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

stdenv Rust fixes #268168

Merged
merged 2 commits into from
Nov 24, 2023
Merged

stdenv Rust fixes #268168

merged 2 commits into from
Nov 24, 2023

Conversation

alyssais
Copy link
Member

Description of changes

See commit messages. Fixes a couple of regressions from #233628.

wasm32-rustc is still not ideal. As followup, I'd like to investigate whether we could just always include the wasm32-unknown-unknown target in rustc builds, since it doesn't require any libc dependency or anything.

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/)
  • 23.11 Release Notes (or backporting 23.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.

@alyssais alyssais added 6.topic: rust 6.topic: stdenv Standard environment labels Nov 17, 2023
@alyssais alyssais requested a review from a user November 17, 2023 20:01
@github-actions github-actions bot added 6.topic: lib The Nixpkgs function library and removed 6.topic: rust 6.topic: stdenv Standard environment labels Nov 17, 2023
@alyssais alyssais changed the title [staging-next] stdenv Rust fixes stdenv Rust fixes Nov 17, 2023
@alyssais alyssais changed the base branch from staging-next to master November 17, 2023 20:03
@alyssais alyssais removed the request for review from RaitoBezarius November 17, 2023 20:03
Copy link
Member

@Ericson2314 Ericson2314 left a comment

Choose a reason for hiding this comment

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

Yeah agreed this seems strictly better.

@delroth delroth added the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Nov 17, 2023
@andresilva
Copy link
Member

FWIW this works fine with a package that I maintain that depends on rustc-wasm32. The wasi config threw me off at first until I read the commit message, maybe adding a small comment there is warranted.

@delroth delroth removed the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Nov 17, 2023
@pbsds
Copy link
Member

pbsds commented Nov 17, 2023

took a while to build, but pagefind functions as expected

@delroth delroth added the 12.approvals: 2 This PR was reviewed and approved by two reputable people label Nov 17, 2023
@alyssais
Copy link
Member Author

@Ericson2314 can I get your ack for dropping the assertion that at most one of "rust" and "rustc" is given? Caused eval to fail; reason now explained in the first commit message.

@delroth delroth removed the 12.approvals: 2 This PR was reviewed and approved by two reputable people label Nov 18, 2023
# https://github.com/rust-lang/rust/blob/2e44c17c12cec45b6a682b1e53a04ac5b5fcc9d2/src/bootstrap/config.rs#L415-L421
isNoStdTarget =
builtins.any (t: lib.hasInfix t final.rust.rustcTarget) ["-none" "nvptx" "switch" "-uefi"];
};
Copy link

Choose a reason for hiding this comment

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

Suggested change
};
} // (args.rust or {});

Otherwise people have no way to override any of this stuff without editing this file.

Previously // args was the last thing in the //-chain, which provided an escape hatch.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think that works. If you pass { rust.platform = { arch = …; os = …; }; }, target-family is supposed to be filled in automatically. If we did a merge like this at the end, the whole platform attrset would be overridden with just the values you passed, so target-family wouldn't end up being there.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm also not confident that this needs to be supported, at least not right now. It was never supported when these were in rust.lib, except by using an overlay, which you could also use in this situation. It would be a bit ugly, but making workarounds for things be ugly when they're fixable bugs in the underlying code is not the worst thing, because it encourages fixing the thing rather than working around it.

@ghost
Copy link

ghost commented Nov 19, 2023

@ofborg eval

Usually, attributes passed explicitly to elaborate take precedence
over the elaborated ones, but since we also elaborate the nested
"rust" attrset, we need to push that one level down, so the rest of
"rust" is still filled in if you just pass
{ rust = { config = ... } }.

I've had to drop the assertion that checked that at most one of "rust"
and "rustc" was part of the un-elaborated system, because doing this
broke passing an elaborated system in, which should be idempotent.

For the same reason, I've also had to make it possible for
rust.rustcTargetSpec to be passed in.  Otherwise, on the second call,
since platform was filled in by the first, the custom target file
would be constructed.  The only other way to avoid this would be to
compare the platform attrs to all built in Rust targets to check it
wasn't one of those, and that isn't feasible.

Fixes: e3e57b8 ("lib.systems: elaborate Rust metadata")
The previous version stopped working when we started elaborating Rust
metadata.  Here, I've made it a bit nicer by actually setting
targetPlatform to an elaborated system.  Setting the config to wasi to
get elaborate to understand it is a bit of a hack, but I think it's
less of a hack than what we had before.

The only actual difference this makes to the rustc-wasm32 derivation
compared to the previous working version, is that now crt-static is
set.  This is probably the right thing anyway.

Fixes: e3e57b8 ("lib.systems: elaborate Rust metadata")
@alyssais
Copy link
Member Author

@Ericson2314 can I get your ack for dropping the assertion that at most one of "rust" and "rustc" is given? Caused eval to fail; reason now explained in the first commit message.

Also had to make it possible to pass in rustcTargetSpec for similar reasons, also now explained in commit message.

@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 10.rebuild-darwin: 1-10 and removed 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin labels Nov 19, 2023
@emilylange emilylange linked an issue Nov 19, 2023 that may be closed by this pull request
@delroth delroth added the 12.approvals: 3+ This PR was reviewed and approved by three or more reputable people label Nov 19, 2023
@emilylange
Copy link
Member

Pardon the question, but what is left to get this merged?
Given this neatly resolves the currently present breakage of pagefind, polkadot and lldap :)

@alyssais
Copy link
Member Author

I would still like @Ericson2314's ack for the changes I mentioned.

Copy link
Contributor

@bendlas bendlas left a comment

Choose a reason for hiding this comment

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

I'm currently running this patchset to fix lldap for my deployments.

@andresilva
Copy link
Member

andresilva commented Nov 23, 2023

IMO we should merge this as-is. It's strictly better than the current situation, since there are broken packages depending on this fix, and if @Ericson2314 has any concerns they can be addressed in a follow-up PR.

@EricTheMagician

This comment was marked as off-topic.

@ibizaman

This comment was marked as off-topic.

@alyssais alyssais merged commit c9b74e4 into NixOS:master Nov 24, 2023
28 checks passed
@alyssais alyssais deleted the rustc-wasm32 branch November 24, 2023 11:21
Copy link
Contributor

Successfully created backport PR for release-23.11:

@misuzu
Copy link
Contributor

misuzu commented Dec 2, 2023

This change broke rustc on armv7l-linux and riscv64-linux when evaluating with NixOS system:

62f7a6dcc1a2baf50e757a6a0bce1083692e0e56 is the first bad commit
commit 62f7a6dcc1a2baf50e757a6a0bce1083692e0e56
Author: Alyssa Ross <[email protected]>
Date:   Fri Nov 17 20:16:22 2023 +0100

    lib.systems.elaborate: fix passing `rust`

    Usually, attributes passed explicitly to elaborate take precedence
    over the elaborated ones, but since we also elaborate the nested
    "rust" attrset, we need to push that one level down, so the rest of
    "rust" is still filled in if you just pass
    { rust = { config = ... } }.

    I've had to drop the assertion that checked that at most one of "rust"
    and "rustc" was part of the un-elaborated system, because doing this
    broke passing an elaborated system in, which should be idempotent.

    For the same reason, I've also had to make it possible for
    rust.rustcTargetSpec to be passed in.  Otherwise, on the second call,
    since platform was filled in by the first, the custom target file
    would be constructed.  The only other way to avoid this would be to
    compare the platform attrs to all built in Rust targets to check it
    wasn't one of those, and that isn't feasible.

    Fixes: e3e57b8f1885 ("lib.systems: elaborate Rust metadata")

 lib/systems/default.nix | 184 ++++++++++++++++++++++++------------------------
 1 file changed, 92 insertions(+), 92 deletions(-)

Used commands:

git bisect start
git bisect good 19cbff5
git bisect bad e92039b55bcd
git bisect run nix-build nixos -A pkgs.rustc --argstr system riscv64-linux -I nixos-config=test.nix --dry-run

test.nix looks like this:

{
  boot.loader.grub.device = "/dev/sda";
  fileSystems."/".device = "/dev/sda1";
  services.sshd.enable = true;
}

See #271000 for more info

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.topic: lib The Nixpkgs function library 10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10 12.approvals: 3+ This PR was reviewed and approved by three or more reputable people
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Build failure: lldap
10 participants