Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add FAQ section for current bazel/buck2 users #1160

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

blakehatch
Copy link
Member

@blakehatch blakehatch commented Jul 15, 2024

Description

Adds a new FAQ page on the docs targeting users already on supported build systems but are unfamiliar with RBE systems such as NativeLink.


This change is Reviewable

Copy link
Member

@aaronmondal aaronmondal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With these incremental improvements that add small-ish/manageable files, please make sure that they pass warning-level vale as outlined in #1150

Reviewable status: 0 of 1 LGTMs obtained, and 1 discussions need to be resolved


docs/src/content/docs/faq/why-nativelink.mdx line 11 at r1 (raw file):

NativeLink's features, you stand to gain significant benefits with minimal effort.

By just integrating remote caching via our **free** hosted cloud,

Copy link
Collaborator

@MarcusSorealheis MarcusSorealheis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this doc doesn't really answer the question fully. You need to talk about how it is a server implementation. Bazel and Buck2 by themselves are clients.

Reviewable status: 0 of 1 LGTMs obtained, and 2 discussions need to be resolved


docs/src/content/docs/faq/why-nativelink.mdx line 10 at r1 (raw file):

perhaps due to monorepo requirements, and aren't yet familiar with
NativeLink's features, you stand to gain significant benefits with minimal effort.

this doc doesn't really answer the question fully. You need to talk about how NativeLink is a server implementation. Bazel and Buck2 by themselves are clients.

Copy link
Member Author

@blakehatch blakehatch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will do @aaronmondal

Reviewable status: 0 of 1 LGTMs obtained, and pending CI: pre-commit-checks, and 2 discussions need to be resolved


docs/src/content/docs/faq/why-nativelink.mdx line 10 at r1 (raw file):

Previously, MarcusSorealheis (Marcus Eagan) wrote…

this doc doesn't really answer the question fully. You need to talk about how NativeLink is a server implementation. Bazel and Buck2 by themselves are clients.

Done.


docs/src/content/docs/faq/why-nativelink.mdx line 11 at r1 (raw file):

NativeLink's features, you stand to gain significant benefits with minimal effort.

By just integrating remote caching via our **free** hosted cloud,

Done.

Copy link
Member Author

@blakehatch blakehatch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Passed pre-commit run -a

Reviewable status: 0 of 1 LGTMs obtained, and pending CI: pre-commit-checks, and 2 discussions need to be resolved

Copy link
Member

@aaronmondal aaronmondal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't pass with warning level. If I change this to warning:

MinAlertLevel = error

And run vale <thefile> I get something like this:

0 errors, 10 warnings and 0 suggestions in 1 file.

Reviewable status: 0 of 1 LGTMs obtained, and 1 discussions need to be resolved

Copy link
Member Author

@blakehatch blakehatch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the result of pre-commit run -a with the .vale.ini shown below. Let me know if there's a step I'm missing

mixed-line-ending........................................................Passed
rustfmt..................................................................Passed
statix...................................................................Passed
trailing-whitespace......................................................Passed
vale.....................................................................Passed


Blakes-MacBook-Pro:docs blakehatch$ cat ../.vale.ini
StylesPath = .github/styles

# TODO(aaronmondal): Set this to `warning`, then to `suggestion` once Vale
#                    stabilizes in CI.
MinAlertLevel = warning

Vocab = TraceMachina

Packages = alex, Microsoft, write-good

Reviewable status: 0 of 1 LGTMs obtained, and 1 discussions need to be resolved

@aaronmondal
Copy link
Member

Ah yeah this is super confusing and we should document this better. Triggering a "warning" in vale doesn't return with a non-zero exit code. This causes the pre-commit hooks to ignore warnings even with the setting.

There are two ways to see the warnings:

  1. Add a bogus word that triggers an actual "error". Then the pre-commit hooks will fail and you'll see the warnings in the logs as well.
  2. Run vale manually with "vale path/to/file.mdx". This should also print the warnings.

I'm aware that this is horrible ux, and I'm really sorry that this is so complicated. Last time I looked into improving this ux it seemed like the only way to treat warnings as errors is via adjustments to our vale GitHub action workflows where we can use another tool to expose warnings more easily. I can add such an improvement as soon as we can "globally" enable the warning level in the codebase. Then it'll be much easier to see warnings and prevent new ones. But for this to work we have to fix all existing warnings first. There are already hundreds of them, which is why I'm trying so hard to prevent new ones.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants