Added incompatible_no_rustc_sysroot_env

[UebelAndre]

To remove SYSROOT from Rustc actions

relates to #2429

[UebelAndre]

Ping @illicitonion

[illicitonion]

I'm not sure we want to be uncoditionally removing this from all actions; I can believe clippy and rustc want different envs here... But also, no tests are failing, so hopefully if this breaks something someone can report a failing test case we can commit.

[UebelAndre]

It’s my understanding that this has no impact on rustc and potentially negative impacts with clippy. Its existence feels low value and the intent satisfied by the —sysroot flag. Is that not correct?

[daivinhtran]

Hi @UebelAndre I've ran into issues with --sysroot flag during bootstrapping at Google after checking in #2277. I was going to propose a fix in this space but haven't had time to to get to it.

Could you hold on to merging this PR? I'll add some notes by the end of today. I'm just making sure the fixes don't step on each other.

If you still prefer to merge the PR, could you add some tests that will fail without this PR? Just to protect from further regression.

cc: @krasimirgg

[UebelAndre]

If you still prefer to merge the PR, could you add some tests that will fail without this PR? Just to protect from further regression.

@daivinhtran The issue is I don't know exactly what causes a failure with this environment variable set. I would have expected failures to occur when enabling --sysroot (aka experimental_toolchain_generated_sysroot - #2298) but alas, nothing failed. This PR adds a feature flag with the variable enabled to expedite the integration. I don't see an issue with merging but if you want to take on addressing #2418 then that sounds good to me and I can hold.

[daivinhtran]

SGTM. I'll address #2418 now. Hope to send out some PR by end of today.

[UebelAndre]

It’s my understanding that this has no impact on rustc and potentially negative impacts with clippy. Its existence feels low value and the intent satisfied by the —sysroot flag. Is that not correct?

@daivinhtran would you be able to confirm or deny this? I still think this would be a good change to make if it's true.

[daivinhtran]

It’s my understanding that this has no impact on rustc and potentially negative impacts with clippy. Its existence feels low value and the intent satisfied by the —sysroot flag. Is that not correct?

@daivinhtran would you be able to confirm or deny this? I still think this would be a good change to make if it's true.

If we're sure that we always pass --sysroot, then clippy-driver will recognize and prefer that over SYSROOT. So removing SYSROOT now does feel safe to me.

One thing I'm cautious about is that SYSROOT might be use somewhere else out of our sight. For example, we didn't know SYSROOT is used by clippy-driver but now we know.

If the intent was to fix #2418, the bug is fixed in clippy-driver now. If the intent is now to remove redundancy, that seems fine to me too.

[UebelAndre]

One thing I'm cautious about is that SYSROOT might be use somewhere else out of our sight.

This is why this change in behavior is introduced as a flag so users can report if this causes a breakage. As of now I know of no known use for it and only 1 issue caused by it. So again, I think we should disable it and see what happens and if a user reports an issue we can add a regression test and maintain the behavior after assessing the use case.

To be clear though, the intent is to drop an unnecessary flag. This change allows us to determine this through user feedback.

[daivinhtran]

One thing I'm cautious about is that SYSROOT might be use somewhere else out of our sight.

This is why this change in behavior is introduced as a flag so users can report if this causes a breakage. As of now I know of no known use for it and only 1 issue caused by it. So again, I think we should disable it and see what happens and if a user reports an issue we can add a regression test and maintain the behavior after assessing the use case.

To be clear though, the intent is to drop an unnecessary flag. This change allows us to determine this through user feedback.

SGTM!