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

Toolchains should be defined as build rules #7869

Closed
shs96c opened this issue Mar 28, 2019 · 8 comments
Closed

Toolchains should be defined as build rules #7869

shs96c opened this issue Mar 28, 2019 · 8 comments
Assignees
Labels
more data needed team-Configurability platforms, toolchains, cquery, select(), config transitions

Comments

@shs96c
Copy link
Contributor

shs96c commented Mar 28, 2019

There are times when it is desirable to build a new toolchain using Bazel, and then use that build subsequent rules. Consequently, defining all toolchains as part of a Bazel's configs, rather than in build rules, is impractical. It follows that it should be possible to define new toolchains using regular build files and targets.

Examples where this might be helpful are compiler development, or when iterating on new Android development releases.

@jin jin added team-Configurability platforms, toolchains, cquery, select(), config transitions untriaged labels Mar 29, 2019
@katre
Copy link
Member

katre commented Apr 1, 2019

@shs96c Have you read the documentation on toolchains: https://docs.bazel.build/versions/master/toolchains.html?

If so, what is missing from the current work that would be helpful?

@katre
Copy link
Member

katre commented Apr 8, 2019

After 7 days with no response, I am closing this issue. Feel free to re-open it with a detailed description of the functionality that is not currently provided.

@katre katre closed this as completed Apr 8, 2019
@shs96c
Copy link
Contributor Author

shs96c commented Jul 9, 2019

@katre, my apologies for not answering sooner. I should be a little more careful about reading my emails.

The major problem with toolchains as outlined in the doc is that they need to be registered in the WORKSPACE. This precludes one building a toolchain as part of the build process, and then using that in rules that depend on that build chain.

This would make writing self-hosting compilers tricky, but (less esoterically) means that all toolchains must be compiled and present on the system before build time (either downloaded via an http_archive or present on the PATH) Sometimes it's useful for this not be the case.

@laurentlb laurentlb reopened this Jul 9, 2019
@katre
Copy link
Member

katre commented Jul 9, 2019

@shs96c: There is nothing stopping you from compiling a toolchain as part of the build. Consider this example:

my_toolchain(  # my_toolchain rule returns ToolchainInfo provider
  name = "my_toolchain_impl",
  compiler = ":my_compiler",
)

cc_binary(
  name = "my_compiler",
  srcs = [...],
  deps = [...],
)

toolchain_type(name = "my_toolchain_type",

toolchain(
  name = "my_toolchain",
  toolchain_type = ":my_toolchain_type",
  toolchain = ":my_toolchain_impl",
  # No pre-existing execution constraints, will be built for the execution platform as needed.
  exec_compatible_with = [],
  target_compatible_with = [
    ...
  ],
)

Then by registering the :my_toolchain target, any rule which requires the :my_toolchain_type toolchain type will get a dependency on the :my_toolchain_impl. This will then be analyzed and built as normal, with the target platform set to be the execution platform where the compiler will be executed.

The only thing that cannot be expressed easily in this system is a bootstrapping compiler: using a pre-built (possibly minimal) my_compiler to in turn compile a (fuller featured) version of the same compiler. In order to support that case, you would need to define two toolchain types (:my_toolchain_type and :my_bootstrap_toolchain_type) and two sets of rules (my_library and my_bootstrap_library), in order to clearly indicate which code is built with which compiler.

@shs96c
Copy link
Contributor Author

shs96c commented Jul 10, 2019

So this can already be done without needing to register the toolchains in my WORKSPACE? That's contrary to what the docs say (which say that they need to be registered via register_toolchains)

In a large monorepo, the top-level WORKSPACE is already getting cluttered, and I don't really want everyone editing that file, particularly to make use of new toolchains. That's the reason why I filed the issue as "toolchains should be defined build rules" --- from the point of view of a user, it's just another dependency of the target using it.

@katre
Copy link
Member

katre commented Jul 10, 2019

I think we are speaking past each other with the term "toolchains should be defined build rules". The problem from my side is this: toolchains are defined using rules in BUILD files: there's the toolchain rule that defines the constraints for a toolchain, and the custom, language-specific rules like cc_toolchain and go_toolchain that define the actual properties of the toolchain.

The only requirement that is placed on toolchains by being registered in the WORKSPACE file is that they must be statically defined: like every other build target, Bazel needs to have a name for them after the BUILD file is loaded. (This does not preclude using macros to generate toolchains, of course).

As nearly as I can tell, your objection is simply that you don't wish the WORKSPACE to grow larger or be edited? In that case, use the load() statement to include bzl files with toolchain definitions, and allow users to add toolchains there.

There is a technical reason why toolchains (and execution platforms) have to be registered in the WORKSPACE: there isn't anyplace else to do it. Using the example of cc rules: when a new cc_toolchain is added, how should the existing cc_library rules discover it exists? There can't be a direct dependency between each cc_library target and the new cc_toolchain: that would require editing every existing rule to support a new toolchain. Bazel can't automatically discover every cc_toolchain in the repo: that would require that Bazel load every BUILD file, which is a major change to how Bazel operates. Currently Bazel only loads BUILD files as-needed, to improve speed and reduce memory usage.

@shs96c
Copy link
Contributor Author

shs96c commented Jul 10, 2019

Thank you for the thoughtful reply. I see your point about why default toolchains need to be registered in the WORKSPACE, but I'd also argue that passing in the toolchains explicitly to certain rules is fine. There are plenty of valid cases where that makes sense (an example is given in #8735)

In the case where a toolchain is passed in as a rule parameter, the target for the toolchain seems like the appropriate thing to use.

Sadly, calling load in included bzl files is blocked by #1550, which is closed, and so is not a great option.

@katre
Copy link
Member

katre commented Nov 11, 2019

Closing for now since this isn't work we intend to pick up. Feel free to open a discussion on the mailing list if you have a proposal to make this easier for you.

@katre katre closed this as completed Nov 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
more data needed team-Configurability platforms, toolchains, cquery, select(), config transitions
Projects
None yet
Development

No branches or pull requests

5 participants