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.

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

rustc: 1.67.1 -> 1.68.1 #220373

Merged
merged 4 commits into from
Mar 26, 2023
Merged

rustc: 1.67.1 -> 1.68.1 #220373

merged 4 commits into from
Mar 26, 2023

Conversation

RaitoBezarius
Copy link
Member

@RaitoBezarius RaitoBezarius commented Mar 9, 2023

Description of changes

https://github.com/rust-lang/rust/blob/master/RELEASES.md

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.

Copy link
Member

@figsoda figsoda left a comment

Choose a reason for hiding this comment

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

@RaitoBezarius RaitoBezarius marked this pull request as draft March 9, 2023 19:14
@RaitoBezarius
Copy link
Member Author

Some hashes would probably need to be updated, see #217084

Cargo will introduce a change that will break our FODs for packages that use Git dependencies.

Making a draft until I figure out all these FODs.

@figsoda
Copy link
Member

figsoda commented Mar 9, 2023

There was a treewide hash change like this and @danieldk had a script, maybe this will help speeding up the process

We can also merge #217084 before this and convert all the hashes to cargoLock, but this will require people to come to a consensus, which is not the case right now: #217084 (comment)

@yu-re-ka
Copy link
Contributor

yu-re-ka commented Mar 9, 2023

If we can somehow extract the Cargo.lock files from each package, we can determine if it needs updating based on if it has any git dependencies

@winterqt
Copy link
Member

winterqt commented Mar 9, 2023

If we can somehow extract the Cargo.lock files from each package, we can determine if it needs updating based on if it has any git dependencies

Steal my logic from #217084 for that, it should work for 99% of packages.

@RaitoBezarius
Copy link
Member Author

I will not have time to perform the treewide before maybe this weekend or more days, so anyone who really want to have this change can go ahead. I'm also in favor of waiting a proper resolution for #217084 (e.g. a merge).

@oxalica
Copy link
Contributor

oxalica commented Mar 17, 2023

Could we enable fresh stabilized sparse-registry for fetchCargoTarball? This can dramatically improve the fetching speed if we need a treewide hash update anyway.

@RaitoBezarius
Copy link
Member Author

Could we enable fresh stabilized sparse-registry for fetchCargoTarball? This can dramatically improve the fetching speed if we need a treewide hash update anyway.

Do you see this in this PR?

@oxalica
Copy link
Contributor

oxalica commented Mar 17, 2023

Could we enable fresh stabilized sparse-registry for fetchCargoTarball? This can dramatically improve the fetching speed if we need a treewide hash update anyway.

Do you see this in this PR?

No? I mean:

diff --git a/pkgs/build-support/rust/fetch-cargo-tarball/default.nix b/pkgs/build-support/rust/fetch-cargo-tarball/default.nix
index 36ab9316974..6c4f8e3053a 100644
--- a/pkgs/build-support/rust/fetch-cargo-tarball/default.nix
+++ b/pkgs/build-support/rust/fetch-cargo-tarball/default.nix
@@ -62,8 +62,13 @@ in stdenv.mkDerivation ({
     export CARGO_HOME=$(mktemp -d cargo-home.XXX)
     CARGO_CONFIG=$(mktemp cargo-config.XXXX)
 
-    if [[ -n "$NIX_CRATES_INDEX" ]]; then
     cat >$CARGO_HOME/config.toml <<EOF
+    [registries.crates-io]
+    protocol = 'sparse'
+    EOF
+
+    if [[ -n "$NIX_CRATES_INDEX" ]]; then
+    cat >>$CARGO_HOME/config.toml <<EOF
     [source.crates-io]
     replace-with = 'mirror'
     [source.mirror]

For nix build -f . cargo-asm.cargoDeps -vL --rebuild, sparse registry reduces building (fetching) time from 119.5s to 14.3s for me, which is about 8x speed.

@figsoda
Copy link
Member

figsoda commented Mar 17, 2023

or we can set the environment variable CARGO_REGISTRIES_CRATES_IO_PROTOCOL

@zowoq
Copy link
Contributor

zowoq commented Mar 22, 2023

or we can set the environment variable CARGO_REGISTRIES_CRATES_IO_PROTOCOL

Opened a PR for this: #222444

@figsoda
Copy link
Member

figsoda commented Mar 23, 2023

1.68.1 is released

@RaitoBezarius
Copy link
Member Author

Upgrading.

@RaitoBezarius
Copy link
Member Author

RaitoBezarius commented Mar 23, 2023

Done and tested.

@RaitoBezarius RaitoBezarius changed the title rustc: 1.67.1 -> 1.68.0 rustc: 1.67.1 -> 1.68.1 Mar 23, 2023
@yu-re-ka
Copy link
Contributor

Built firefox on x86_64-linux

@yu-re-ka
Copy link
Contributor

@ofborg build fd

@yu-re-ka
Copy link
Contributor

Built thunderbird on aarch64-linux

@ofborg ofborg bot requested review from basvandijk and globin March 26, 2023 12:26
@zowoq
Copy link
Contributor

zowoq commented Mar 26, 2023

Build passthru.tests on aarch64-darwin, x86-64_darwin.

@RaitoBezarius RaitoBezarius marked this pull request as ready for review March 26, 2023 13:25
@RaitoBezarius
Copy link
Member Author

Should we add release notes for this upgrade?

@RaitoBezarius
Copy link
Member Author

Could we enable fresh stabilized sparse-registry for fetchCargoTarball? This can dramatically improve the fetching speed if we need a treewide hash update anyway.

Do you see this in this PR?

No? I mean:

diff --git a/pkgs/build-support/rust/fetch-cargo-tarball/default.nix b/pkgs/build-support/rust/fetch-cargo-tarball/default.nix
index 36ab9316974..6c4f8e3053a 100644
--- a/pkgs/build-support/rust/fetch-cargo-tarball/default.nix
+++ b/pkgs/build-support/rust/fetch-cargo-tarball/default.nix
@@ -62,8 +62,13 @@ in stdenv.mkDerivation ({
     export CARGO_HOME=$(mktemp -d cargo-home.XXX)
     CARGO_CONFIG=$(mktemp cargo-config.XXXX)
 
-    if [[ -n "$NIX_CRATES_INDEX" ]]; then
     cat >$CARGO_HOME/config.toml <<EOF
+    [registries.crates-io]
+    protocol = 'sparse'
+    EOF
+
+    if [[ -n "$NIX_CRATES_INDEX" ]]; then
+    cat >>$CARGO_HOME/config.toml <<EOF
     [source.crates-io]
     replace-with = 'mirror'
     [source.mirror]

For nix build -f . cargo-asm.cargoDeps -vL --rebuild, sparse registry reduces building (fetching) time from 119.5s to 14.3s for me, which is about 8x speed.

Apologies, I meant: do you think I should do this change in this PR? I apologize if it has been taken aggressively. (English language barrier yadayada).

@oxalica
Copy link
Contributor

oxalica commented Mar 26, 2023

I meant: do you think I should do this change in this PR?

I mean, yes. You can cherry pick the smaller env var patch from #222444. It also causes no rebuild since it affects only FODs.

@RaitoBezarius
Copy link
Member Author

I meant: do you think I should do this change in this PR?

I mean, yes. You can cherry pick the smaller env var patch from #222444. It also causes no rebuild since it affects only FODs.

Done, thank you and sorry again for realizing too late I misunderstood your message.

@zowoq zowoq merged commit 9861cf4 into NixOS:staging Mar 26, 2023
@vcunat
Copy link
Member

vcunat commented Apr 11, 2023

Reproducible ICE is interesting? https://hydra.nixos.org/build/214913529

@figsoda
Copy link
Member

figsoda commented Apr 11, 2023

turns out rust-lang/rust#107688 was not in 1.68, opened #225778 to fix that

@RaitoBezarius
Copy link
Member Author

Ugh, sorry for that.

@ghost
Copy link

ghost commented Apr 17, 2023

Edit: I think I found the problem. Testing a fix. Fix here: #226593

rustc can no longer be cross compiled (build!=host) as of this commit according to git bisect.

When building the `rustc`'s vendored libraries, they are unable to find `std::`, which I find quite baffling. Any ideas?

Below is a sampling of the error messages (there are hundreds like this) from nix build -f . -L pkgsCross.aarch64-multiplatform.rustc:

rustc-aarch64-unknown-linux-gnu> error[E0463]: can't find crate for `std`
rustc-aarch64-unknown-linux-gnu>  --> /build/rustc-1.68.0-src/vendor/serde_json/build.rs:1:5
rustc-aarch64-unknown-linux-gnu>   |
rustc-aarch64-unknown-linux-gnu> 1 | use std::env;
rustc-aarch64-unknown-linux-gnu>   |     ^^^ can't find crate
rustc-aarch64-unknown-linux-gnu> error[E0463]: can't find crate for `std`
rustc-aarch64-unknown-linux-gnu>  --> /build/rustc-1.68.0-src/vendor/serde_json/build.rs:2:5
rustc-aarch64-unknown-linux-gnu>   |
rustc-aarch64-unknown-linux-gnu> 2 | use std::process::Command;
rustc-aarch64-unknown-linux-gnu>   |     ^^^ can't find crate
rustc-aarch64-unknown-linux-gnu> error[E0463]: can't find crate for `std`
rustc-aarch64-unknown-linux-gnu>  --> /build/rustc-1.68.0-src/vendor/serde_json/build.rs:3:5
rustc-aarch64-unknown-linux-gnu>   |
rustc-aarch64-unknown-linux-gnu> 3 | use std::str::{self, FromStr};
rustc-aarch64-unknown-linux-gnu>   |     ^^^ can't find crate
rustc-aarch64-unknown-linux-gnu> error: cannot find macro `println` in this scope
rustc-aarch64-unknown-linux-gnu>   --> /build/rustc-1.68.0-src/vendor/serde_json/build.rs:38:9
rustc-aarch64-unknown-linux-gnu>    |
rustc-aarch64-unknown-linux-gnu> 38 |         println!("cargo:rustc-cfg=no_btreemap_retain");
rustc-aarch64-unknown-linux-gnu>    |         ^^^^^^^
rustc-aarch64-unknown-linux-gnu> error: cannot find macro `println` in this scope
rustc-aarch64-unknown-linux-gnu>   --> /build/rustc-1.68.0-src/vendor/serde_json/build.rs:32:9
rustc-aarch64-unknown-linux-gnu>    |
rustc-aarch64-unknown-linux-gnu> 32 |         println!("cargo:rustc-cfg=no_btreemap_remove_entry");
rustc-aarch64-unknown-linux-gnu>    |         ^^^^^^^
rustc-aarch64-unknown-linux-gnu> error: cannot find macro `println` in this scope
rustc-aarch64-unknown-linux-gnu>   --> /build/rustc-1.68.0-src/vendor/serde_json/build.rs:26:9
rustc-aarch64-unknown-linux-gnu>    |
rustc-aarch64-unknown-linux-gnu> 26 |         println!("cargo:rustc-cfg=no_btreemap_get_key_value");
rustc-aarch64-unknown-linux-gnu>    |         ^^^^^^^
rustc-aarch64-unknown-linux-gnu> error: cannot find macro `println` in this scope
rustc-aarch64-unknown-linux-gnu>   --> /build/rustc-1.68.0-src/vendor/serde_json/build.rs:14:13
rustc-aarch64-unknown-linux-gnu>    |
rustc-aarch64-unknown-linux-gnu> 14 |             println!("cargo:rustc-cfg=limb_width_32");
rustc-aarch64-unknown-linux-gnu>    |             ^^^^^^^
rustc-aarch64-unknown-linux-gnu> error: cannot find macro `println` in this scope
rustc-aarch64-unknown-linux-gnu>   --> /build/rustc-1.68.0-src/vendor/serde_json/build.rs:11:13
rustc-aarch64-unknown-linux-gnu>    |
rustc-aarch64-unknown-linux-gnu> 11 |             println!("cargo:rustc-cfg=limb_width_64");
rustc-aarch64-unknown-linux-gnu>    |             ^^^^^^^
rustc-aarch64-unknown-linux-gnu> error[E0531]: cannot find tuple struct or tuple variant `Some` in this scope
rustc-aarch64-unknown-linux-gnu>   --> /build/rustc-1.68.0-src/vendor/serde_json/build.rs:19:9
rustc-aarch64-unknown-linux-gnu>    |
rustc-aarch64-unknown-linux-gnu> 19 |         Some(minor) => minor,
rustc-aarch64-unknown-linux-gnu>    |         ^^^^ not found in this scope

@ghost ghost mentioned this pull request Apr 17, 2023
2 tasks
@vcunat vcunat mentioned this pull request Apr 19, 2023
3 tasks
@lblasc
Copy link
Contributor

lblasc commented Apr 19, 2023

Binaries generated by crate2nix (buildRustCrate) are much bigger after the last staging iteration, most likely do to rust bump (still checking), debug and release are affected.

I've tested on some internal crate.

Commit before merge bf839a4b2031557eb43e77d39d343b7ecc84c692:

150M    result/bin

After merge:

1.2G    result/bin

@RaitoBezarius
Copy link
Member Author

Can you try by disabling auditable? (cargo-auditable)

@figsoda
Copy link
Member

figsoda commented Apr 19, 2023

buildRustCrate doesn't have an auditable option

@lblasc
Copy link
Contributor

lblasc commented Apr 20, 2023

It is not connected with rust version bump, after reverting to a 1.67.1 on master binaries are still much bigger.

@RaitoBezarius
Copy link
Member Author

It is not connected with rust version bump, after reverting to a 1.67.1 on master binaries are still much bigger.

Can you do a bisect?

@ghost
Copy link

ghost commented Apr 20, 2023

@lblasc, unless you are 100% sure that it was this PR that caused your problem, you need to open a new issue for your problem (you can reference it here). Otherwise the discussion keeps getting moved from one PR to another, pinging the wrong people.

I've tested on some internal crate.

Please post a complete reproducer in the new issue when you open it.

Note: before #210843 buildRustCrate was unconditionally stripping all builds, including debug builds. That was fixed by #210843. Maybe you're doing a debug build? If so, either switch to doing a release build or else strip manually.

@dzmitry-lahoda
Copy link

Interesting, it worked all with git deps before this PR. What was reason?
All reasonably big projects work with git deps. And why to break what was warking before? would be awesome there would be warning before breaking people using working nix.

@winterqt
Copy link
Member

@dzmitry-lahoda You're about 6 months late to this discussion, but we talked about this issue (as well as the best way to handle it) for months. This isn't a breakage on our part, but Cargo's. See #217084, #221716, and the minimal discussion in these comments (starting here) for more information.

If you're working with a local project that doesn't have the limitations of Nixpkgs, just use importCargoLock or another solution that parses your Cargo.lock, like you probably should have been doing anyways.

I'd appreciate it if you did a tad bit of research (such as even reading the comments on this PR) before assuming that we didn't put in the work to solve this issue, or that we never noticed it.

@dzmitry-lahoda
Copy link

sorry, thank you for hard work

https://opencollective.com/nixos/contributions/678610

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants