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

mesa: fix cross compilation to x86_64 #233628

Merged
merged 3 commits into from
Nov 9, 2023
Merged

Conversation

alyssais
Copy link
Member

Description of changes

In particular, fix cross compilation of rusticl. Previously, Nixpkgs has not been equipped to handle non-Cargo Rust cross compilation. That's getting increasingly important, now that we're building Mesa with Rust, and soon the kernel, and potentially systemd in not too long.

This is a bigger PR than I usually like to make, but it's difficult to explain each of these changes out of context, so I've bundled them into a single PR. I'd definitely recommend reviewing commit by commit though. The commit messages contain the details of each change.

Spiritual successor to #159064.

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.

@alyssais alyssais added 6.topic: rust 6.topic: cross-compilation Building packages on a different sort platform than than they will be run on labels May 23, 2023
@github-actions github-actions bot added 6.topic: GNOME GNOME desktop environment and its underlying platform 6.topic: stdenv Standard environment labels May 23, 2023
@RaitoBezarius

This comment was marked as resolved.

@alyssais
Copy link
Member Author

@ofborg eval

@RaitoBezarius

This comment was marked as outdated.

@alyssais
Copy link
Member Author

alyssais commented Nov 4, 2023

I've rebased to fix the merge conflict, and I've also:

  • Simplified the logic for looking up values from args.rust falling back to args.rustc falling back to a default.
  • Added an assert so that it's not possible to pass args.rust (the new name) and args.rustc (the old name) at the same time.
  • Fixed rustcTargetSpec referring to a non-existent rustcPlatform that was left over from a previous iteration.

All suggested by @Ericson2314.

ghost
ghost previously requested changes Nov 4, 2023
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

We need lib.warn backwards-compatibility placeholders on the toFooBar attributes; crate2nix uses them (in their current location).

Otherwise looks good.

Building:

  • pkgs.tests.cross.sanity

lib/systems/default.nix Show resolved Hide resolved
pkgs/build-support/rust/lib/default.nix Show resolved Hide resolved
@ghost ghost dismissed their stale review November 5, 2023 09:40

main issue fixed; will approve once builds complete

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Rust changes look great.

Built:

@ghost ghost mentioned this pull request Nov 6, 2023
13 tasks
Copy link
Member

@Mic92 Mic92 left a comment

Choose a reason for hiding this comment

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

Looks clean.

lib/systems/default.nix Outdated Show resolved Hide resolved
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.

Just one little thing, and then it looks good to. This is very important change let's get it in 23.05 23.11!

(I didn't really look at all the usage site, but I think those are very low risk)

We need this stuff to be available in lib so make-derivation.nix can
access it to construct the Meson cross file.

This has a couple of other advantages:

 - It makes Rust less special.  Now figuring out what Rust calls a
   platform is the same as figuring out what Linux or QEMU call it.

 - We can unify the schema used to define Rust targets, and the schema
   used to access those values later.  Just like you can set "config"
   or "system" in a platform definition, and then access those same
   keys on the elaborated platform, you can now set "rustcTarget" in
   your crossSystem, and then access "stdenv.hostPlatform.rustcTarget"
   in your code.

"rustcTarget", "rustcTargetSpec", "cargoShortTarget", and
"cargoEnvVarTarget" have the "rustc" and "cargo" prefixes because
these are not exposed to code by the compiler, and are not
standardized.  The arch/os/etc. variables are all named to match the
forms in the Rust target spec JSON.

The new rust.target-family only takes a list, since we don't need to
worry about backwards compatibility when that name is used.

The old APIs are all still functional with no warning for now, so that
it's possible for external code to use a single API on both 23.05 and
23.11.  We can introduce the warnings once 23.05 is EOL, and make them
hard errors when 23.11 is EOL.
In particular, fix cross compilation of rusticl.
@ghost
Copy link

ghost commented Nov 7, 2023

I agree, this should be merged before the branch-off. However,

This is very important change let's get it in 23.05!

I think that ship has sailed 😆 unless nix-shell -p time-machine

@alyssais
Copy link
Member Author

alyssais commented Nov 7, 2023

The sphinx build failure on x86_64-linux seems to be intermittent / load-related. Let's try again:

@ofborg build mesa mesa.passthru.tests stdenv stdenv.passthru.tests

@ghost
Copy link

ghost commented Nov 9, 2023

The sphinx build failure on x86_64-linux seems to be intermittent / load-related. Let's try again:

I've been getting sphinx test failures on my machines too. It's annoying. I disabled the tests for that package. This has happened before btw.

(and I don't think it is caused by this PR)

We should consider disabling the tests for the sphinx package. It's just a documentation generator.

@alyssais
Copy link
Member Author

alyssais commented Nov 9, 2023

OfBorg hasn't given me the aarch64 builds for days, so I'm just going to merge. I've tested locally on x86_64-linux and aarch64-linux, and IIRC mesa is broken on Darwin anyway. In the unlikely event there are failures, we'll catch them in staging.

@alyssais alyssais merged commit 6247bd0 into NixOS:staging Nov 9, 2023
20 of 22 checks passed
@alyssais alyssais deleted the mesa-cross branch November 9, 2023 09:02
@andresilva
Copy link
Member

Seems like this PR broke all packages that use rustc-wasm32, namely: lldap, pagefind and polkadot.

@andresilva
Copy link
Member

andresilva commented Nov 17, 2023

Nevermind, I found #267876 which should fix this.

@andresilva andresilva mentioned this pull request Nov 17, 2023
13 tasks
@alyssais alyssais mentioned this pull request Nov 17, 2023
13 tasks
@alyssais
Copy link
Member Author

Fix is #268168.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.topic: cross-compilation Building packages on a different sort platform than than they will be run on 6.topic: GNOME GNOME desktop environment and its underlying platform 6.topic: lib The Nixpkgs function library 6.topic: python 6.topic: rust 6.topic: stdenv Standard environment 10.rebuild-darwin: 501-1000 10.rebuild-darwin: 501+ 10.rebuild-linux: 501+ 10.rebuild-linux: 1001-2500 12.approvals: 1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants