-
Notifications
You must be signed in to change notification settings - Fork 306
fix: Relax exec_compatible_with requirement on dtrace rules. #2759
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
base: master
Are you sure you want to change the base?
Conversation
|
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). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
4dcbfc0 to
7ab34a6
Compare
|
|
|
@brentleyjones, can you explain a bit more about toolchain selection for |
Something like that. We initiate our builds from Linux, to run on macOS executors, and without this change it could select Linux exectors. |
|
ok, so I think using a toolchain (as the updated PR now does) should work for that setup. I did a bit of testing:
I'm happy to toolchain-ify the other rules that have exec_compatible_with set if you'd like me to (I don't think there is benefit to this other than consistency). |
|
Neat!
Only a benefit if we are using a path that we want to override. |
In #2661 the
exec_compatible_withwas added. This breaks some of our Linux-based tests where we use systemtap dtrace. I was hoping we could move the constraint onto the tool itself (that way we can supply one that's Linux-compatible and things keep working).target_compatible_withworks for my usecase, but it's not exactly the same as the previous solution. @brentleyjones, could you have a look and let me know if this works for you?