Conversation
keith
requested review from
armandomontanez,
c-mita,
comius,
pzembrod and
trybka
as code owners
|
I'd really prefer we don't carry this one forward, and instead fix any instances that rely on |
|
I agree with that in theory. But given bazel's long deprecation timelines it seems to me that the cost of us carrying it along for a while with a change like this, is very much worth it compared to the migration cost to make sure your transitive closure avoids needing this. In our moderately sized C++ project at least grpc: grpc/grpc#38319 and abseil: https://github.com/abseil/abseil-cpp/pull/1797/files all use this mechanism. So my thinking is that even if we snapped our fingers and fixed all of today's deprecated uses, it's non-trivial to upgrade these dependencies in any projects that potentially want to use this API. I'm also quite worried about moving bazel off of this API, here are a few places I found that have been using it forever:
|
This is currently usable in select statements (which is deprecated but won't be flipped for 1 year) and also is used internally by bazel for some paths like `_solib_k8`. We can derive this from platform configs
keith
force-pushed
the
ks/set-target_cpu-in-toolchain
branch
from
691d5bc to
42829a3
Compare
|
@armandomontanez ptal |
armandomontanez
left a comment
armandomontanez
left a comment
There was a problem hiding this comment.
Since this doesn't modify the public API, I'm inclined to just move forward with this.
| ], | ||
| ) | ||
|
|
||
| config_setting( |
There was a problem hiding this comment.
These should definitely be private. we don't want people to start getting these from rules_cc.
There was a problem hiding this comment.
I'm realizing now that it's possible making these private will inherently not work since they're referenced under a macro. If that's the case, we should probably put them in impl/ to at least signal that they shouldn't be referenced externally.
There was a problem hiding this comment.
Another way to solve this is to just split the attrs into OS and CPU so you don't need the config_setting, and then do the conversion at the starlark level. Not the prettiest but it prevents misuse.
There was a problem hiding this comment.
yea the macro was the issue, i moved them to impl. should be pretty clear that they're private, im not sure if buildifier lints against impl or just internal though
| known_features = known_features, | ||
| enabled_features = enabled_features, | ||
| compiler = compiler, | ||
| cpu = select({ |
There was a problem hiding this comment.
Shouldn't this have a fallback case for other platforms? Also, what about Windows? It does have "well-known" legacy CPU values.
There was a problem hiding this comment.
yea I wasn't sure what other ones we thought this supported today. happy to add whichever here
There was a problem hiding this comment.
"//conditions:default" : "" is important so that this doesn't throw an error if one of these branches isn't followed. IMO it's fine to incrementally iterate on this list as we uncover more magic.
For windows, protobuf seems to check "win32" and "win64". That's probably the closest we'll find to a "source of truth" for that one.
There was a problem hiding this comment.
added the windows ones.
I used local as the default because that appears to be what the legacy toolchain used. but we can do an empty string too if we want? maintaining compat on that one doesn't feel as important to me
There was a problem hiding this comment.
I'm not seeing anywhere that relies on "local", so in theory the value is arbitrary.
| known_features = known_features, | ||
| enabled_features = enabled_features, | ||
| compiler = compiler, | ||
| cpu = select({ |
There was a problem hiding this comment.
I'm not seeing anywhere that relies on "local", so in theory the value is arbitrary.
Refers to @rules_cc labels via Label rather than @rules_cc.
3 participants
This is currently usable in select statements (which is deprecated but
won't be flipped for 1 year) and also is used internally by bazel for
some paths like
_solib_k8. We can derive this from platform configs