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 more info on custom toolchains #3396

Merged
merged 2 commits into from
Apr 11, 2022
Merged

Conversation

mari-crl
Copy link
Contributor

@mari-crl mari-crl commented Apr 7, 2022

Expand notes on custom toolchains in the toolchains documentation page

PR Checklist

Please check if your PR fulfills the following requirements:

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature (please, look at the "Scope of the project" section in the README.md file)
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Documentation content changes
  • Other... Please describe:

What is the current behavior?

@knz wanted some extra clarity on how to configure toolchains in this PR's comments
Issue Number: #2991

What is the new behavior?

More notes to expand on why things are the way they are

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

Expand notes on custom toolchains in the toolchains documentation page
@google-cla
Copy link

google-cla bot commented Apr 7, 2022

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

For more information, open the CLA check for this pull request.

@mari-crl
Copy link
Contributor Author

mari-crl commented Apr 7, 2022

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

For more information, open the CLA check for this pull request.

CLA bot, you don't recognize me? It's me, Mari! You know, mstaib@!

Ohhhh, fine, I guess I changed accounts and email addresses (and last names). You're lucky you're cute!

4) A call to [the `register_toolchains` function](https://bazel.build/rules/lib/globals#register_toolchains) in your `WORKSPACE`
that refers to the `toolchain` rule defined in step 3.

Examples of steps 2-4 can be found in the [documentation for `node_toolchain`](https://bazelbuild.github.io/rules_nodejs/Core.html#node_toolchain).
Copy link
Collaborator

Choose a reason for hiding this comment

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

could you keep the link relative instead of pointing to the github docs site? that way it still works when you're in the GH UI looking at an older tag, for example

Copy link
Collaborator

@alexeagle alexeagle left a comment

Choose a reason for hiding this comment

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

thanks!
CI failure is just asking that you bazel run //docs:update

Copy link

@knz knz left a comment

Choose a reason for hiding this comment

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

I'm happy about these changes but if i am a newcomer to a project that currently uses Bazel but I am not expert in Bazel, this doc changes is not telling me step-by-step what I need to do to achieve a custom yarn/npm use.

The phrasing of this doc change assumes the reader is already a bazel exprt.


If necessary, you can substitute building the node binary as part of the build with using a locally installed version by skipping step 1 and replacing step 2 with:

2) A `node_toolchain` rule which has the path of the system binary as its `target_tool_path`
Copy link

Choose a reason for hiding this comment

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

Can you add an example of this. I can't find this documented elsewhere.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I agree an example would be nice, and if Cockroach wants to fund it then we could make the time to write one.

@mari-crl
Copy link
Contributor Author

mari-crl commented Apr 8, 2022

I'm happy about these changes but if i am a newcomer to a project that currently uses Bazel but I am not expert in Bazel, this doc changes is not telling me step-by-step what I need to do to achieve a custom yarn/npm use.

The phrasing of this doc change assumes the reader is already a bazel exprt.

Is there something in particular that you feel is not sufficiently explained?

@knz
Copy link

knz commented Apr 8, 2022

Is there something in particular that you feel is not sufficiently explained?

Well for example, if I arrive at the cockroachdb repository, I'd be at a loss about how to use this doc to actually solve the problem at hand (using a system-wide yarn/npm).

@mari-crl
Copy link
Contributor Author

mari-crl commented Apr 8, 2022

Well for example, if I arrive at the cockroachdb repository, I'd be at a loss about how to use this doc to actually solve the problem at hand (using a system-wide yarn/npm).

Can you be more specific? Where do you become stuck, and what confuses you about that/those step(s)?

@alexeagle
Copy link
Collaborator

I like the sentiment, but Bazel is an expert tool to configure in a repo, and custom tool chains is an advanced use case. I doubt there's much we could do to solve that here.

@knz
Copy link

knz commented Apr 9, 2022 via email

@knz
Copy link

knz commented Apr 9, 2022 via email

@alexeagle
Copy link
Collaborator

Yeah I agree, Bazel is hard to use and documentation is out-of-date. There's no longer any tech writing team at Google on the project, last I heard.
CC and CXX env variables is a relevant example, I believe those are deprecated and you now use the same toolchains feature we're describing here to select your C/C++ compiler.
It requires a lot more deep understanding than "add a couple of lines of code in a file," sadly. We try to make the defaults easy to use, like with the @bazel/create npm package, but this is advanced usage and I think rules_nodejs is consistent with the rest of the Bazel ecosystem in terms of the prerequisite level of understanding to configure for the advanced usage.

I really do appreciate your feedback and want to help, but I also want to reset your expectations: Bazel requires some expert configuration. At most companies that's a small number of experts required for hundreds of developer/users, and often only periodically.

@alexeagle alexeagle merged commit 8b5d274 into bazel-contrib:stable Apr 11, 2022
@mari-crl
Copy link
Contributor Author

There used to be a time where one could override tool paths using environment variables, and these were even standardized across build chains (CC, CXX etc). This made it easy for folk who didn't understand the build tools to still be able to run experiments on different versions etc. It's Bazel that's the odd one sticking out here.

I see what you mean - yes, that's a model that definitely makes it a lot easier to change the tools that your tools use!

Unfortunately, in order to gain the benefits that Bazel seeks to bring, it couldn't use this model. Precise reproducibility - achieved in large part by ignoring the environment outside the build tree as much as possible, including outside system paths and environment variables - is a huge part of what Bazel strives for, so merely setting a local system path for an experiment is not something Bazel supports trivially. That wasn't a goal for earlier build systems, so easy experimentation is something they can support.

In the same way, for example, you usually can't easily edit database files on disk, and have to go through that database's sometimes-standard, sometimes-arcane procedures to extract or modify the data. Because human-readability/hand-editability is usually not a goal for databases, and they instead optimize for speed and size of storage.

I presume my job is to add a couple of lines of code in a file.

Correct. Though you'll need to modify at least two files here.

Where i become stuck is that i do not know which file that should be and which lines i should write in it and where in the file these lines should be written.

This will require some knowledge of the fundamentals of Bazel. There are four types of files used to configure Bazel:

  • BUILD.bazel files, which are distributed throughout the repository. These indicate that their containing directory is a package by their presence. These contain rules which describe how to build the code contained within that package. With few exceptions (the package rule has to be at the top of the file), the order of rules in a BUILD.bazel file does not matter.
  • Starlark files, with the extension .bzl. These describe the definitions of rules. BUILD.bazel files load them in order to use their definitions.
  • The WORKSPACE file, which is at the top level of the repository - in fact, its presence indicates that this is a Bazel repository. This contains special workspace rules which describe what external repositories should be brought in and how.
  • The .bazelrc file, which has a few possible locations and contains command-line flags to be automatically specified.

With these fundamentals in place, you know that the rules mentioned by steps 1-3 are meant to go in a BUILD.bazel file (which one? where in that file? That depends on your project - Bazel doesn't care, so it's a style matter, only of interest to humans.) Step 4 explicitly mentions the WORKSPACE file, so you know that's what you have to change with that.

These aren't expert-level matters; this is just the basic high-level overview of how Bazel works.

I am expecting a file section containing a list of tool paths with a clear comment that explains this is the purpose of that section, and i expect the ability to inductively imitate the code that's already there to reach my needs. I don't find such a section, and i don't find a pattern i can inductively follow.

This simply isn't the way Bazel works, unfortunately. There is no list of tools used; each rule definition defines what tools it wants and how to get those tools. Some rules provide extension points, like rules_nodejs does, and this teaches you how to use those extension points.

And the Bazel docs are not forthcoming with step-by-step tutorials either. (And yes, i checked, the cmake and automake docs do explain clearly what one has to do to override tool paths. So it's not like what I'm requesting is unreasonable)

More tutorials would absolutely be nice!

And just to be clear, I wasn't able to make heads nor tails of the cmake and automake documentation to find what you're talking about on a brief glance. To be able to make the change you're describing, I'd have to spend more time with them to get the background knowledge sufficient to know where to even look and how to understand what I'm looking at - precisely what you'll likely have to do with Bazel.

This is probably not something that you wanted to learn, and that's unfortunate. But as Alex pointed out, you're taking on a task that, in Bazel, is an expert-level task. And the project you're working on has decided to use Bazel. Fortunately, though, you're on the early-adopter train, so you have some time to get used to it before it becomes the default!

@rickystewart rickystewart mentioned this pull request Apr 15, 2022
12 tasks
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