-
Notifications
You must be signed in to change notification settings - Fork 505
Bump rules_cc to 0.1.4
#3535
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: main
Are you sure you want to change the base?
Bump rules_cc to 0.1.4
#3535
Conversation
Looks like macos doesn't work after updating to
@keith does that ring any bells? I saw you made a ton of changes to |
🤔 any idea which file this is referring to? is this using the latest apple_support too? |
I first saw this in #3534 where I was bumping everything. And isn't this referring to https://github.com/bazelbuild/rules_cc/blob/0769eb3cec5ae57b02c1f34709643f7cc872af21/cc/private/toolchain/osx_cc_wrapper.sh.tpl#L1 ? |
@keith By the time
Thus, the |
i think some folks did a pass to avoid /bin/bash for portability bazelbuild/rules_cc@d749150 should that action be setting use_default_shell_env=True? |
I would rather avoid inconsistencies from things injected into PATH on various users machines for these actions. As far as I know that flag is convenient but otherwise degrades caching. If there's some magic I'm not aware of to avoid the consequences I'd be happy to turn it on. |
that respects --incompatible_strict_action_env so if the user cares about hermiticity they will still cover the risks there |
I opened bazelbuild/rules_cc#448 which resolves should resolve this issue. |
fwiw i think it's fair for actions to require that PATH is set to something reasonable, so i imagine there will be some pushback on that |
In practice, maybe? But wouldn't the ideal be the action has everything it needs to get the job done? And if so, why would PATH be needed? If this is an intentional change to require PATH then it'd be nice to have an incompatibility flag for an easier transition. |
Using The issue seems to be that the CI environment just not provides a path to |
@hzeller Is the issue is the not lack of |
@hzeller I think the direction to require |
If things can be converted to If you use |
I think there are 2 issues going against the grain here.
|
Interacting with PATH at all feels like an anti-pattern that only occurs out of convenience. I think a more common and frustrating error in using I do think So TLDR my opinion is:
|
Yea but the fix for this is top flip use_default_shell_env=True. It doesn't pull in all env, just some AFAIUI
But there is still a PATH and you still rely on it, not for bash if you do the change back to |
Bringing a couple of notes over from #3556 that I can't see having been discussed:
|
No description provided.