Skip to content

Default experimental_toolchain_generated_sysroot to True #2277

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 4 commits into from
Nov 25, 2023
Merged

Default experimental_toolchain_generated_sysroot to True #2277

merged 4 commits into from
Nov 25, 2023

Conversation

daivinhtran
Copy link
Contributor

@daivinhtran daivinhtran commented Nov 21, 2023 •
edited
Loading

#2223 only sets --@rules_rust//rust/settings:experimental_toolchain_generated_sysroot in a few targets on CI. This PR defaults the feature flag to True.

This ensures the feature flag works at a larger scope because we remove it.

@daivinhtran daivinhtran force-pushed the set-sysroot-by-default branch 2 times, most recently from 2824e05 to 0620e50 Compare November 21, 2023 18:19
@daivinhtran daivinhtran force-pushed the set-sysroot-by-default branch from 0620e50 to ac64412 Compare November 21, 2023 18:49
@daivinhtran daivinhtran marked this pull request as ready for review November 21, 2023 19:05
@daivinhtran
Copy link
Contributor Author

@UebelAndre PTAL

@UebelAndre
Copy link
Collaborator

Thanks! Why not flip the flag though?

@daivinhtran daivinhtran changed the title Enable setting --sysroot to rustc commands in this repo Remove experimental_toolchain_generated_sysroot feature flag Nov 21, 2023
@daivinhtran daivinhtran force-pushed the set-sysroot-by-default branch from 8969ea3 to 4c3f445 Compare November 21, 2023 20:41
@daivinhtran daivinhtran force-pushed the set-sysroot-by-default branch from 4c3f445 to 5269036 Compare November 21, 2023 20:42
@daivinhtran
Copy link
Contributor Author

@UebelAndre Yes. Let's just flip the flag. I changed the PR. PTAL.

@UebelAndre
Copy link
Collaborator

I don’t think you should remove the flag just yet. Can you only flip the default?

@daivinhtran daivinhtran changed the title Remove experimental_toolchain_generated_sysroot feature flag Default experimental_toolchain_generated_sysroot to True Nov 21, 2023
@daivinhtran
Copy link
Contributor Author

@UebelAndre Done. PTAL.

UebelAndre
UebelAndre approved these changes Nov 25, 2023
View reviewed changes
Copy link
Collaborator

@UebelAndre UebelAndre left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@UebelAndre UebelAndre merged commit a09c6d9 into bazelbuild:main Nov 25, 2023
@gferon
Copy link
Contributor

gferon commented Dec 1, 2023

I'm not 100% sure, but this might have introduced an issue with the clippy aspect. I'm getting:

Use --sandbox_debug to see verbose messages from the sandbox and retain the sandbox build root for debugging
error: Option 'sysroot' given more than once

when trying to upgrade rules_rust. Any thoughts?

@UebelAndre
Copy link
Collaborator

I'm not 100% sure, but this might have introduced an issue with the clippy aspect. I'm getting:

Use --sandbox_debug to see verbose messages from the sandbox and retain the sandbox build root for debugging
error: Option 'sysroot' given more than once

when trying to upgrade rules_rust. Any thoughts?

Interesting, I'm curious why this wasn't caught by existing CI. Can you file a bug with a small repro?

@UebelAndre UebelAndre mentioned this pull request Dec 1, 2023
Closed
@neilisaac
Copy link
Contributor

neilisaac commented Dec 22, 2023 •
edited
Loading

@UebelAndre same issue here. I had to set build --@rules_rust//rust/settings:experimental_toolchain_generated_sysroot=false to work around this.

clippy-driver will automatically add --sysroot based on SYSROOT if it doesn't detect a --sysroot command line flag. Is the @bazel-out/k8-opt/bin/.../test-NNN/my_test.clippy.ok-0.params parameter passed literally to clippy-driver, or is it expanded by process_wrapper? If it's passed literally, that would explain why it appears twice: once in the clippy.ok-0.params file, and once by clippy-driver.

Should an "experimental" flag really be enabled by default? I overlooked this initially because I assumed the experimental flag would be disabled by default.

@daivinhtran daivinhtran mentioned this pull request Jan 12, 2024
Closed
This was referenced Jan 23, 2024
Merged
Closed
@daivinhtran
Copy link
Contributor Author

The fix to this issue should be done upstream in rust-lang/rust-clippy#12203.

nmattia added a commit to dfinity/ic that referenced this pull request Jun 13, 2024
@nmattia
chore(IDX): remove clippy sysroot workaround
9fa76dc 
This removes a workaround that was necessary in the past to work around
an issue in clippy. The issue is now fixed in clippy, and our clippy
version is recent enough to include the fix.

See also:
* bazelbuild/rules_rust#2277
* rust-lang/rust-clippy#12203
@nmattia nmattia mentioned this pull request Jun 13, 2024
Merged
github-merge-queue bot pushed a commit to dfinity/ic that referenced this pull request Jun 13, 2024
@nmattia
chore(IDX): remove clippy sysroot workaround (#214)
50aa238 
This removes a workaround that was necessary in the past to work around
an issue in clippy. The issue is now fixed in clippy, and our clippy
version is recent enough to include the fix.

See also:
* bazelbuild/rules_rust#2277
* rust-lang/rust-clippy#12203
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@UebelAndre UebelAndre UebelAndre approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@daivinhtran @UebelAndre @gferon @neilisaac