Skip to content

[v2] rust: Write to .cargo/config.toml instead of .cargo/config#331167

Merged
emilazy merged 1 commit into
NixOS:stagingfrom
mmlb:rust-write-config.toml-not-config
Sep 12, 2024
Merged

[v2] rust: Write to .cargo/config.toml instead of .cargo/config#331167
emilazy merged 1 commit into
NixOS:stagingfrom
mmlb:rust-write-config.toml-not-config

Conversation

@mmlb

@mmlb mmlb commented Jul 30, 2024

Copy link
Copy Markdown
Member

Description of changes

Changes the rust infra to write cargo config to .cargo/config.toml insted of .cargo/config since the latter is deprecated (since 1.38?) and is now warning on stderr.

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • 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/)
  • 24.11 Release Notes (or backporting 23.11 and 24.05 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.

Add a 👍 reaction to pull requests you find important.


Changes since v1:

  • Avoid changing cargoHash for FOD
  • Build/fixup more packages

@github-actions github-actions Bot added 6.topic: rust General-purpose programming language emphasizing performance, type safety, and concurrency. 6.topic: ruby A dynamic, open source programming language with a focus on simplicity and productivity. labels Jul 30, 2024
@mmlb

mmlb commented Jul 30, 2024

Copy link
Copy Markdown
Member Author

This time I've verified that no cargoHash changes to FOD happens (note, this was from when I based on master, waiting on building-most-of-the-world now that I rebased on staging):

[18:14:12]-[~/c/g/n/nixpkgs]-[manny@c3mv18nix]
git status
On branch rust-write-config.toml-not-config
nothing to commit, working tree clean
[18:14:18]-[~/c/g/n/nixpkgs]-[manny@c3mv18nix]
git rev-parse HEAD
c935d4b1ba9800c06629b7d3c39f156d52c32c9d
[18:14:20]-[~/c/g/n/nixpkgs]-[manny@c3mv18nix]
git grep cargoHash pkgs/by-name/co/commitmsgfmt/package.nix
pkgs/by-name/co/commitmsgfmt/package.nix:  cargoHash = "sha256-jTRB9ogFQGVC4C9xpGxsJYV3cnWydAJLMcjhzUPULTE=";
[18:14:24]-[~/c/g/n/nixpkgs]-[manny@c3mv18nix]
sed -i '/cargoHash/ s|".*"|""|' pkgs/by-name/co/commitmsgfmt/package.nix
[18:14:53]-[~/c/g/n/nixpkgs]-[manny@c3mv18nix]
nix-build -A commitmsgfmt 2>&1 | tail -n 5
calling 'postFixup' function hook '_multioutPropagateDev'
error: hash mismatch in fixed-output derivation '/nix/store/k0sfjvbjx1binzpm5b36n02a030rzdy9-commitmsgfmt-1.6.0-vendor.tar.gz.drv':
         specified: sha256-AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA=
            got:    sha256-jTRB9ogFQGVC4C9xpGxsJYV3cnWydAJLMcjhzUPULTE=
error: 1 dependencies of derivation '/nix/store/x925hf14jii50vzlniamja7yb2726dzd-commitmsgfmt-1.6.0.drv' failed to build
[--:--:--]-[~/c/g/n/nixpkgs]-[manny@c3mv18nix]

@mmlb mmlb changed the base branch from master to staging July 30, 2024 22:16
@github-actions github-actions Bot added 6.topic: python Python is a high-level, general-purpose programming language. 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 6.topic: GNOME GNOME desktop environment and its underlying platform 6.topic: haskell General-purpose, statically typed, purely functional programming language 6.topic: qt/kde Object-oriented framework for GUI creation 6.topic: kernel The Linux kernel 8.has: documentation This PR adds or changes documentation 8.has: changelog This PR adds or changes release notes 8.has: module (update) This PR changes an existing module in `nixos/` 6.topic: emacs Text editor 6.topic: printing Drivers, CUPS & Co. 6.topic: vim Advanced text editor 6.topic: ocaml OCaml is a general-purpose, high-level, multi-paradigm programming language. 6.topic: nodejs Node.js is a free, open-source, cross-platform JavaScript runtime environment 6.topic: lua Lua is a powerful, efficient, lightweight, embeddable scripting language. 6.topic: testing Tooling for automated testing of packages and modules 6.topic: systemd Software suite that provides an array of system components for Linux operating systems. 6.topic: vscode A free and versatile code editor that supports almost every major programming language. 6.topic: jupyter Interactive computing tooling: kernels, notebook, jupyterlab 6.topic: php PHP is a general-purpose scripting language geared towards web development. 8.has: maintainer-list (update) This PR changes `maintainers/maintainer-list.nix` 6.topic: llvm/clang Issues related to llvmPackages, clangStdenv and related labels Jul 30, 2024
@mmlb mmlb force-pushed the rust-write-config.toml-not-config branch from c935d4b to f2da47c Compare July 30, 2024 22:16
@github-actions github-actions Bot removed 6.topic: python Python is a high-level, general-purpose programming language. 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 6.topic: GNOME GNOME desktop environment and its underlying platform labels Jul 30, 2024
@mmlb

mmlb commented Jul 30, 2024

Copy link
Copy Markdown
Member Author

This should be the proper fix to close #320294

@mmlb

mmlb commented Jul 31, 2024

Copy link
Copy Markdown
Member Author

And same steps to verify no change to cargoHash now off of staging:

[20:23:02]-[~/c/g/n/nixpkgs]-[manny@c3mv18nix]
git status
On branch rust-write-config.toml-not-config
nothing to commit, working tree clean
[20:23:04]-[~/c/g/n/nixpkgs]-[manny@c3mv18nix]
git rev-parse HEAD
4d384b1cf0afb65a5b1c3e7ecb4754a292662bcb
[20:23:08]-[~/c/g/n/nixpkgs]-[manny@c3mv18nix]
git grep cargoHash pkgs/by-name/co/commitmsgfmt/package.nix
pkgs/by-name/co/commitmsgfmt/package.nix:  cargoHash = "sha256-jTRB9ogFQGVC4C9xpGxsJYV3cnWydAJLMcjhzUPULTE=";
[20:23:10]-[~/c/g/n/nixpkgs]-[manny@c3mv18nix]
sed -i '/cargoHash/ s|".*"|""|' pkgs/by-name/co/commitmsgfmt/package.nix
[20:23:14]-[~/c/g/n/nixpkgs]-[manny@c3mv18nix]
nix-build -A commitmsgfmt 2>&1 | tail -n 5
calling 'postFixup' function hook '_multioutPropagateDev'
error: hash mismatch in fixed-output derivation '/nix/store/y58pyqfr0ad4pwwkwn4mlnr1bvn7xzsf-commitmsgfmt-1.6.0-vendor.tar.gz.drv':
         specified: sha256-AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA=
            got:    sha256-jTRB9ogFQGVC4C9xpGxsJYV3cnWydAJLMcjhzUPULTE=
error: 1 dependencies of derivation '/nix/store/vxhj2yakd59ciqip8my6zz39pahj83vd-commitmsgfmt-1.6.0.drv' failed to build

@ofborg ofborg Bot requested a review from mbalatsko July 31, 2024 03:16
@ofborg ofborg Bot added 10.rebuild-darwin: 501+ This PR causes many rebuilds on Darwin and should normally target the staging branches. 10.rebuild-darwin: 5001+ This PR causes many rebuilds on Darwin and must target the staging branches. 10.rebuild-linux: 501+ This PR causes many rebuilds on Linux and should normally target the staging branches. 10.rebuild-linux: 5001+ This PR causes many rebuilds on Linux and must target the staging branches. labels Jul 31, 2024
@winterqt

winterqt commented Aug 9, 2024

Copy link
Copy Markdown
Member

I'm not sure if this will just time out or not, but let's see.

@ofborg build fd

@mmlb

mmlb commented Aug 16, 2024

Copy link
Copy Markdown
Member Author

@winterqt looks like it succeeded (FOD being FOD, its not much a signal though... right?). Any other worries? Also ping ping @zowoq @figsoda @Mic92 @mbalatsko

@emilazy emilazy left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM in principle.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Non‐blocking nit: If we’re changing this hook anyway we could get rid of this unnecessary ;.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

can do

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

updated/removed

@emilazy

emilazy commented Sep 3, 2024

Copy link
Copy Markdown
Member

This is an improvement over the status quo either way, but: this doesn’t adjust the fetchCargoTarball behaviour, which is good, but that means it’s still writing to the deprecated location. Doesn’t that mean we’ll still get warnings if we don’t mv or ln it in the build hooks? Do we not use those files at all somehow?

@VuiMuich

VuiMuich commented Sep 4, 2024

Copy link
Copy Markdown
Contributor

Would we want to symlink ln -s .cargo/config.toml .cargo/config for compatibility to cargo 1.38 and earlier proactively, or should we wait for some occurrence?

Closes #334857

@emilazy

emilazy commented Sep 4, 2024

Copy link
Copy Markdown
Member

From discussion on Matrix I think we decided that we don’t need to support 5+ year old compilers in Nixpkgs.

@VuiMuich

VuiMuich commented Sep 4, 2024

Copy link
Copy Markdown
Contributor

From discussion on Matrix I think we decided that we don’t need to support 5+ year old compilers in Nixpkgs.

Fair enough. And if there would be a package that could absolutely not be built on a newer rust, they could create the symlink in their derivation anyways.

@mmlb

mmlb commented Sep 5, 2024

Copy link
Copy Markdown
Member Author

This is an improvement over the status quo either way, but: this doesn’t adjust the fetchCargoTarball behaviour, which is good, but that means it’s still writing to the deprecated location. Doesn’t that mean we’ll still get warnings if we don’t mv or ln it in the build hooks? Do we not use those files at all somehow?

code comments say this only applies when depending on a git repo. I'm not sure if I built one of those explicitly, let me check (currently rebuilding everything as I ran a gc and did not cache any of this remotely ...)

Honestly I'm not sure why its not warning any more even without the mv or ln, as you suggest. I suspect it has to do with difference between $CARGO_HOME/config vs $CARGO_HOME/config.toml and where fetchCargoTarball writes the config to $name/.cargo/config. Looks like cargo is not warning about the latter case. I have a hunch that will change one day and then I'm not sure what we can do w/o causing the FOD hashes to be wrong.

@mmlb mmlb force-pushed the rust-write-config.toml-not-config branch from 4d384b1 to 5b36343 Compare September 5, 2024 18:42
@wegank wegank added the 2.status: merge conflict This PR has merge conflicts with the target branch label Sep 10, 2024
Seeing the following new warnings pop up on stderr when cargo was bumped
to 1.78:

```
warning: `/build/.cargo/config` is deprecated in favor of `config.toml`
note: if you need to support cargo 1.38 or earlier, you can symlink `config` to `config.toml`
```

which happens to break commitmsgfmt builds in nix (NixOS#320294).

closes NixOS#320294
@mmlb

mmlb commented Sep 12, 2024

Copy link
Copy Markdown
Member Author

Rebased on latest staging to handle the conflict, looks like I'm rebuilding the world ...

@mmlb mmlb force-pushed the rust-write-config.toml-not-config branch from 5b36343 to 17b3df2 Compare September 12, 2024 02:26
@mmlb

mmlb commented Sep 12, 2024

Copy link
Copy Markdown
Member Author

commitmsgfmt FOD hash checks out as no changes after rebase.

@mmlb mmlb requested a review from emilazy September 12, 2024 02:28

@emilazy emilazy left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think let’s land this and worry about if the path in fetchCargoTarball affects anything another time. Thanks!

@emilazy emilazy merged commit bce3d51 into NixOS:staging Sep 12, 2024
@ofborg ofborg Bot removed the 2.status: merge conflict This PR has merge conflicts with the target branch label Sep 12, 2024
@mmlb mmlb deleted the rust-write-config.toml-not-config branch September 13, 2024 15:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

6.topic: ruby A dynamic, open source programming language with a focus on simplicity and productivity. 6.topic: rust General-purpose programming language emphasizing performance, type safety, and concurrency. 10.rebuild-darwin: 501+ This PR causes many rebuilds on Darwin and should normally target the staging branches. 10.rebuild-darwin: 5001+ This PR causes many rebuilds on Darwin and must target the staging branches. 10.rebuild-linux: 501+ This PR causes many rebuilds on Linux and should normally target the staging branches. 10.rebuild-linux: 5001+ This PR causes many rebuilds on Linux and must target the staging branches.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants