bazelembedded / modular_cc_toolchains

MIT License
3 stars 1 forks source link

Decide on action/feature naming convention #5

Open silvergasp opened 1 year ago

silvergasp commented 1 year ago

By moving action/feature configs into the build files we create a new problem when it comes to naming schemas for toolchain configuration. In particular how to we map, target names e.g.

# file: //foo:BUILD
cc_feature(
  name = "foo",
  #...
)
# file: //conflicting_foo:BUILD
cc_feature(
  name = "foo",
  #...
)

Here we have two features in different directories, but with the same name. If we where to include both in our toolchain we may end up with some unexpected behaviour if the target name directly maps to the action/feature name.

Potential solutions

  1. We use the fully qualified label in the action name this would disambiguate between features with the same name but in different directories
  2. We check if there are duplicate named features in the resulting toolchain and raise an error.
tpudlik commented 1 year ago

As I mentioned in the doc, my intuition is that (1) is preferable. The pro is that configurations that look correct indeed are; and treating objects with distinct labels as distinct is the bazel way.

The con, however, is that the CLI syntax (--features=...) becomes cumbersome. I think this is maybe not a huge deal: you should probably not be setting your compiler configuration on the command line, anyway, and rely on specifying it entirely in BUILD files. So we're just guiding people away from a pattern that's problematic anyway.

Another potential con: are there any "well-known" features that folks would be confused to not have short names? (E.g., --features=external_include_paths, https://github.com/bazelbuild/bazel/pull/13107)

silvergasp commented 1 year ago

Yeah, I agree. I'll start with 2. then. These issues match up with TODO's for the next PR I'm working on.

silvergasp commented 1 year ago

A bit of an update on this, it turns out that Bazel currently limits feature names to lowercase ascii so having fully qualified labels is not possible. This is what happens if you try to use a label string;

A feature's name must consist solely of lowercase ASCII letters, digits, '.', '_', '+', and '-', got '@//cc:garbage_collect_sections'
silvergasp commented 1 year ago

While I do like the fully qualified names, it might require upstream changes to bazel.

tpudlik commented 1 year ago

OK, sounds like this might be blocked on such Bazel changes, then. Too bad!

@trybka FYI