Skip to content
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

rustfmt: fix on darwin #231078

Merged
merged 1 commit into from
May 12, 2023
Merged

rustfmt: fix on darwin #231078

merged 1 commit into from
May 12, 2023

Conversation

PuercoPop
Copy link
Contributor

Description of changes

Without this patch rustfmt installs successfully but upon running it it throws the following error

  $ rustfmt
  dyld[68147]: Library not loaded: @rpath/librustc_driver-c577224f5690b349.dylib
    Referenced from: <no uuid> /nix/store/w46inc6cad6xg2kw09v9n629iqb7aqrw-rustfmt-1.69.0/bin/rustfmt
    Reason: tried: '/nix/store/ih18sy1kcbl5kdgq2l8l1lm70ai6aybj-apple-framework-CoreFoundation-11.0.0/Library/Frameworks/librustc_driver-c577224f5690b349.dylib' (no such file), '/System/Volumes/Preboot/Cryptexes/OS/nix/store/ih18sy1kcbl5kdgq2l8l1lm70ai6aybj-apple-framework-CoreFoundation-11.0.0/Library/Frameworks/librustc_driver-c577224f5690b349.dylib' (no such file), '/nix/store/ih18sy1kcbl5kdgq2l8l1lm70ai6aybj-apple-framework-CoreFoundation-11.0.0/Library/Frameworks/librustc_driver-c577224f5690b349.dylib' (no such file), '/System/Volumes/Preboot/Cryptexes/OS/nix/store/ih18sy1kcbl5kdgq2l8l1lm70ai6aybj-apple-framework-CoreFoundation-11.0.0/Library/Frameworks/librustc_driver-c577224f5690b349.dylib' (no such file), '/usr/local/lib/librustc_driver-c577224f5690b349.dylib' (no such file), '/usr/lib/librustc_driver-c577224f5690b349.dylib' (no such file, not in dyld cache)
  Abort trap: 6

Reading this rust-issue it seems that we have to link against rustc_driver. The clippy.nix expression already does something similar

Of the 4 executables found in the result of building rustfmt only rustfmt and git-rustfmt needed to be linked. The other worked without linking to rustc_driver.

Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 23.05 Release Notes (or backporting 22.11 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

@PuercoPop
Copy link
Contributor Author

The editorconfig check is failing, I can't find the error message in the web UI. I tried reproducing the failure locally but it doesn't seem to complain:

cd ~/src/nixpkgs
$ nix-shell -p editorconfig-checker

[nix-shell:~/src/nixpkgs]$ editorconfig-checker -disable-indent-size pkgs/development/compilers/rust/rustfmt.nix 

[nix-shell:~/src/nixpkgs]$ echo $?
0

@winterqt
Copy link
Member

Blame GitHub -- I re-ran it, and it passed.

winterqt
winterqt reviewed May 10, 2023
View reviewed changes
pkgs/development/compilers/rust/rustfmt.nix
@@ -15,6 +15,11 @@ rustPlatform.buildRustPackage rec {
rustPlatform.rust.rustc.llvm
] ++ lib.optional stdenv.isDarwin Security;

preFixup = lib.optionalString stdenv.isDarwin ''
Copy link
Member

Choose a reason for hiding this comment

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

Might be worth adding my comment from Clippy's derivation here, or just reference it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree that it should be commented. I was unsure on what was the best way to do so. I've copied the comment verbatim.

winterqt
winterqt requested changes May 10, 2023
View reviewed changes
Copy link
Member

@winterqt winterqt left a comment

Choose a reason for hiding this comment

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

Commit message needs to be in proper form, e.g. rustfmt: fix on darwin.

@winterqt
Copy link
Member

I'd also argue that the commit message can be shortened significantly, as we already explain why this workaround is needed in the Clippy derivation/commits, but maybe I'm nitpicking too much :)

@ofborg ofborg bot added the 6.topic: darwin Running or building packages on Darwin label May 10, 2023
@ofborg ofborg bot requested review from basvandijk and globin May 10, 2023 14:38
@PuercoPop PuercoPop force-pushed the fix-rustfmt-darwin branch 2 times, most recently from 469544f to 580f3f3 Compare May 10, 2023 15:01
@PuercoPop
Copy link
Contributor Author

PuercoPop commented May 10, 2023 •
edited
Loading

Thanks for the swift review. I've updated the commit header line as requested and removed some of the specific details of the commit message. Let me know what you think

@PuercoPop @winterqt
rustfmt: fix on darwin
4e00120 
Reading this [rust-issue] it seems that we have to link against
rustc_driver. The [clippy.nix] expression already does something similar

Of the 4 executables found in the result of building rustfmt only
rustfmt and git-rustfmt needed to be linked. The other worked without
linking to rustc_driver.

[rust-issue]: rust-lang/rust#105609
[clippy.nix]: https://github.com/NixOS/nixpkgs/blob/c8cf570dae9b41f30395b71f9b432418b4ff0ebc/pkgs/development/compilers/rust/clippy.nix#L27-L36
@winterqt winterqt changed the title fix rustfmt in darwin rustfmt: fix on darwin May 12, 2023
@winterqt
Copy link
Member

Just changed the commit message to remove the extraneous Darwin mention.

winterqt
winterqt approved these changes May 12, 2023
View reviewed changes
Copy link
Member

@winterqt winterqt left a comment

Choose a reason for hiding this comment

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

Tested, thanks!

@winterqt
Copy link
Member

Skipping OfBorg because I've already tested this extensively, and it seems to be very backed up right now.

@winterqt winterqt merged commit dd584d8 into NixOS:master May 12, 2023
@figsoda figsoda mentioned this pull request May 12, 2023
Merged
12 tasks
@jfly jfly mentioned this pull request Dec 13, 2023
Closed
@PuercoPop PuercoPop mentioned this pull request May 3, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@winterqt winterqt winterqt approved these changes

@Mic92 Mic92 Awaiting requested review from Mic92 Mic92 is a code owner

@zowoq zowoq Awaiting requested review from zowoq zowoq is a code owner

@figsoda figsoda Awaiting requested review from figsoda figsoda is a code owner

@basvandijk basvandijk Awaiting requested review from basvandijk

@globin globin Awaiting requested review from globin

Assignees
No one assigned
Labels
6.topic: darwin Running or building packages on Darwin 6.topic: rust 10.rebuild-darwin: 11-100 10.rebuild-linux: 501+ 10.rebuild-linux: 1001-2500 12. first-time contribution
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@PuercoPop @winterqt @Janik-Haag