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 OS type "none" to platforms. #7560

Closed
wants to merge 2 commits into from

Conversation

wildarch
Copy link

This pull request introduces the OS type "none" used in target triples for bare metal/embedded devices, so things like rules_rust can target freestanding hardware 😄.

@irengrig irengrig added the team-Configurability platforms, toolchains, cquery, select(), config transitions label Mar 1, 2019
@katre
Copy link
Member

katre commented Mar 1, 2019

Will every rust toolchain that targets bare metal/no OS devices be functionally identical, or will each individual target device need a separate toolchain?

I'd prefer to see projects defining their own constraint values and platforms for this to denote their own individual needs, rather than trying to add everything to a single "no-os" constraint value.

So, as example, if I am following https://os.phil-opp.com/ and trying to adapt it for Bazel (I have tried!), I would want to define my own platforms:

constraint_value(name = "katre_os", constraint_setting = "@bazel_tools//platforms:os")
platform(
  name = "dev_vm",
  constraint_values = [
    "@bazel_tools//platforms:x86_64",
    ":katre_os",
  ],
)

And then I can set up my cross-compiling toolchains to also use this target constraint.

@wildarch
Copy link
Author

wildarch commented Mar 7, 2019

For development on bare metal (and even when something like FreeRTOS is used) a toolchain for the none OS is nearly always used, because there is no underlying OS that provides an abstraction over the hardware.

I think that if you would want to build your katre_os with bazel, you would build the kernel for OS none, and any userspace programs would be built with the katre_os constraint. The toolchain for kernel space will be different from the userspace toolchain, because in userspace you will have an environment available that is provided by the katre_os kernel.

So while I do agree that we would want people that build applications for their own OS to define their own platform, I think that we can add a built-in none os type for the bare metal and kernel developers.

@katre
Copy link
Member

katre commented Mar 8, 2019

This makes sense for platforms, but I'm not clear what will happen when we add values to the OS enum. Adding @jmmv to see if he has any ideas what else would need to be updated.

@jmmv
Copy link
Contributor

jmmv commented Mar 8, 2019

I find it quite confusing that the OS class, which I always assumed was an internal class to Bazel to represent the platform Bazel runs on, leaks through constraints. Is that intentional or an accident of our implementation?

I am also confused about "none". I think you want to say that binaries built against the real hardware have no OS underneath, hence this value, right? In that case, maybe "standalone" would make more sense?

@katre
Copy link
Member

katre commented Mar 8, 2019

@jmmv: We defined constraints matching the OS enum values because those are the OSes that Bazel supports. There's no real need to go the other way, however: every constraint value for @bazel_tools//platforms:os doesn't need to be an enum value.

@wildarch
Copy link
Author

wildarch commented Mar 8, 2019

I find it quite confusing that the OS class, which I always assumed was an internal class to Bazel to represent the platform Bazel runs on, leaks through constraints. Is that intentional or an accident of our implementation?

I am also confused about "none". I think you want to say that binaries built against the real hardware have no OS underneath, hence this value, right? In that case, maybe "standalone" would make more sense?

Toolchains for C, C++ and Rust usually denote the no OS platform as none, that would be a reason to also use this naming in bazel. I know that GCC has a -ffreestanding flag for programs that cannot rely on a standard library or startup routine, so we could also opt to call the platform freestanding. I think standalone is quite a descriptive name, but for coherence I would personally prefer none or freestanding.

@wildarch
Copy link
Author

@katre so if I understand you correctly this PR would not need to include the change to OS.java, as we are only cross compiling to none, but never using it as a host platform?

@katre
Copy link
Member

katre commented Mar 12, 2019

Yes, I think that's correct: adding the enum value has no effect, since nothing will ever set it anyway.

@wildarch
Copy link
Author

The latest commit reverses the addition of the NONE case to the OS enum, so this PR now only adds none as a constraint value for os.

@jmmv
Copy link
Contributor

jmmv commented Mar 13, 2019

This seems fine to me now (not modifying OS.java and using the none name), but I'll leave it to @katre to judge the platforms changes.

I'd probably extend the commit message to describe why none was chosen as opposed to standalone or freestanding for example, given the rationale above.

The `none` os constraint value remains as a standard platform.

While names such as 'freestanding' or 'standalone' have been proposed,
the name remains 'none' to be consistent with the naming of targets
triples such as 'arm-none-eabi' that will satisfy this constaint.
@katre
Copy link
Member

katre commented Mar 13, 2019

The visibility of the "os" constraint setting is public, so you should be able to add the new ":none" constraint_values in your own project. If Bazel isn't using these, and the default cc toolchains aren't creating cc_toolchains for them, are they needed in Bazel core?

@wildarch
Copy link
Author

The visibility of the "os" constraint setting is public, so you should be able to add the new ":none" constraint_values in your own project. If Bazel isn't using these, and the default cc toolchains aren't creating cc_toolchains for them, are they needed in Bazel core?

The current design of rules_rust requires the none os type to be defined in Bazel before this type of toolchains can be used. I'm not a Bazel expert, so I would not know if it would make more sense for rules_rust to define this constraint value by itself or if it should be part of Bazel. My guess would be that having it in Bazel core would make it easier to link cc_librarys to rust_librarys and vice versa?

@katre
Copy link
Member

katre commented Mar 13, 2019

Why does rules_rust require the constraint to be present in the core Bazel repo? I would expect the following to work (once cc rules use toolchain resolution, which is in progress, see #7260):

constraint_value(name = "bare", constraint_setting = "@bazel_tools//platforms:os")
platform(name = "test_vm",
  constraint_values = [
    "@bazel_tools//platforms:x86_64",
    ":bare",
  ],
)

cc_toolchain(
  name = "bare-cc-toolchain-impl",
  # other attributes as required
)
toolchain(
  name = "bare-cc-toolchain",
  toolchain_type = "@bazel_tools//tools/cpp:toolchain_type",
  toolchain = ":bare-cc-toolchain-impl",
  exec_coompatible_with = [
    "@bazel_tools//platforms:x86_64",
    "@bazel_tools//platforms:linux", # assuming the compiler runs on linux
  ],
  target_compatible_with = [
    "@bazel_tools//platforms:x86_64",
    ":bare",
  ],
)

rust_toolchain(
  name = "bare-rust-toolchain",
  # other attributes as required
)
toolchain(
  name = "bare-rust-toolchain",
  toolchain_type = "@io_bazel_rules_rust//:toolchain_type",
  toolchain = ":bare-rust-toolchain-impl",
  exec_coompatible_with = [
    "@bazel_tools//platforms:x86_64",
    "@bazel_tools//platforms:linux", # assuming the compiler runs on linux
  ],
  target_compatible_with = [
    "@bazel_tools//platforms:x86_64",
    ":bare",
  ],
)

Then invoke something like:

bazel build --platforms=//:test_vm --extra_toolchains=//:bare-cc-toolchain,//:bare-rust-toolchain //:my_binary

This allows Bazel to know that

  1. There is a CC toolchain for the platform with no os.
  2. There is a Rust toolchain ditto.
  3. It should use them when trying to build your binary for the platform for no os.

None of this needs to be defined in either Bazel core, or even in rules_rust, just in the project of whoever is configuring the compilers for the no-os compilation.

@wildarch
Copy link
Author

@katre the thing is that rules_rust defines the toolchains for you based on the target triples specified in the workspace, which means that the workspace can't really define these constraints. But maybe this is in fact a deficiency in the approach of rules_rust, I will float this issue there for further discussion.

@wildarch
Copy link
Author

After some discussion and thinking I agree with @katre that bazel core is indeed not the right place to add this. I'm closing this PR and looking into how we can adapt rules_rust instead.

@wildarch wildarch closed this Mar 25, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes team-Configurability platforms, toolchains, cquery, select(), config transitions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants