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

zenith: re-enable aarch64 #88616

Closed
wants to merge 1 commit into from
Closed

zenith: re-enable aarch64 #88616

wants to merge 1 commit into from

Conversation

bbigras
Copy link
Contributor

@bbigras bbigras commented May 22, 2020

This depends on #88185

The only change from that PR is platforms = platforms.all;.

Motivation for this change
Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@bbigras bbigras mentioned this pull request May 22, 2020
Merged
10 tasks
@bbigras bbigras changed the title Zenith aarch64 zenith: re-enable aarch64 May 22, 2020
@bbigras bbigras marked this pull request as ready for review May 22, 2020 17:19
@doronbehar
Copy link
Contributor

No, you need to put in this PR only the commit:

zenith: re-enable aarch64

And if this doesn't work yet, put a [WIP] prefix to the PR title.

@bbigras bbigras changed the title zenith: re-enable aarch64 [WIP] zenith: re-enable aarch64 May 22, 2020
@doronbehar
Copy link
Contributor

In addition to stating in this PR's text that it depends on #88185 .

@bbigras
Copy link
Contributor Author

bbigras commented May 22, 2020

No, you need to put in this PR only the commit:

Can I base this PR on the other one?

0.8.2 won't build with aarch64. Only 0.9.0 has the fix.

And if this doesn't work yet, put a [WIP] prefix to the PR title.

Done. I did a draft PR at first but I thought maybe the bot wouldn't build it.

@bbigras
Copy link
Contributor Author

bbigras commented May 22, 2020

In addition to stating in this PR's text that it depends on #88185 .

Like I already did?

image

@doronbehar
Copy link
Contributor

Like I already did?

Yea yea, I just wanted to make sure you won't remove it LOL.

Can I base this PR on the other one?

Well I guess it is more comfortable to test this PR with that update commit included but just don't forget to remove it once you'll succeed in making this build work. You can try and use this text in commits to test the build:

@GrahamcOfBorg build zenith

Are you using an aarch64 machine?

@bbigras
Copy link
Contributor Author

bbigras commented May 22, 2020

Thanks :)

@GrahamcOfBorg build zenith

I thought I needed some special permissions to use that.

don't forget to remove [the other commit] once you'll succeed in making this build work.

👍

Are you using an aarch64 machine?

I don't have access to an aarch64 machine. Well except my phone or with qemu but that would be slow.

@doronbehar
Copy link
Contributor

Thanks :)

@GrahamcOfBorg build zenith

IIRC that every member of @NixOS/nixpkgs-maintainers is supposed to be able to do that for sandboxed builds (non darwin builds) but from some reason this doesn't work for me now and I'm supposed to be even a bit more privileged according to https://github.com/NixOS/ofborg/blob/released/config.public.json#L36 so IDK...

@bbigras
Copy link
Contributor Author

bbigras commented May 22, 2020

It took a couple of minutes before the bot started to run the stuff for the other PR, maybe it has a lot of work to do.

@bbigras
Copy link
Contributor Author

bbigras commented May 23, 2020

I rebased the PR.

@GrahamcOfBorg build zenith

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/prs-in-distress/3604/22

@bbigras
Copy link
Contributor Author

bbigras commented May 26, 2020

@GrahamcOfBorg build zenith

@bbigras
Copy link
Contributor Author

bbigras commented May 26, 2020

@GrahamcOfBorg build zenith

@bbigras bbigras changed the title [WIP] zenith: re-enable aarch64 zenith: re-enable aarch64 May 26, 2020
@bbigras
Copy link
Contributor Author

bbigras commented May 26, 2020

@GrahamcOfBorg build zenith

@bbigras
Copy link
Contributor Author

bbigras commented May 26, 2020

OK, it seems my "fix" works. I guess this PR is ready for reviews.

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/prs-ready-for-review-may-2019/3032/168

@bbigras
Copy link
Contributor Author

bbigras commented May 28, 2020

I give up due to lack of interest.

@bbigras bbigras closed this May 28, 2020
@bbigras bbigras deleted the zenith-aarch64 branch May 28, 2020 18:05
Thra11
Thra11 reviewed Jun 7, 2020
View reviewed changes
pkgs/tools/system/zenith/default.nix
@@ -15,12 +15,15 @@ rustPlatform.buildRustPackage rec {

buildInputs = stdenv.lib.optionals stdenv.isDarwin [ IOKit ];

preBuild = ''
export CARGO_TARGET_AARCH64_UNKNOWN_LINUX_GNU_LINKER=gcc
Copy link
Member

Choose a reason for hiding this comment

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

Instead of using preBuild, I believe you can simply write

CARGO_TARGET_AARCH64_UNKNOWN_LINUX_GNU_LINKER = "gcc";

and the call to mkDerivation underlying buildRustPackage will interpret it as setting an environment variable for the build process.

@Thra11
Copy link
Member

Thra11 commented Jun 7, 2020

Let's not be too hasty in assuming people aren't interested. I know it can seem like ages when you're waiting for a response, but unfortunately, it can take a while for the right person to see a PR, and might take even longer for them to find the time to review it. This is especially true for less widely used platforms such as aarch64.

I built this on my aarch64 system and ran the result. It appears to work fine.

I'd be interested to understand more about why the CARGO_TARGET_AARCH64_UNKNOWN_LINUX_GNU_LINKER environment variable needs to be set. I'm guessing one of the crates hard-codes something it shouldn't? If so, it would of course be nice to get a fix upstream at some point, but I don't think that should hold up this PR.

Other than the minor change I suggested in how the environment variable is set, this looks good to me if you're willing to reopen it! 👍

@bbigras bbigras mentioned this pull request Sep 13, 2020
Merged
10 tasks
@uri-canva
Copy link
Contributor

This is due to the fact we always pass --target which prevents rust from detecting that we're trying to build for the host and not trying to cross compile: rust-lang/cargo#5754 (comment).

The fix is to delete the cross compilation configuration as in #107237, we configure cross compilation differently in nixpkgs anyway.

@uri-canva
Copy link
Contributor

I will open a separate issue to see if we can generalise the fix.

@uri-canva uri-canva mentioned this pull request Nov 13, 2021
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Thra11 Thra11 Thra11 left review comments

Assignees
No one assigned
Labels
8.has: package (new) 10.rebuild-darwin: 1-10 10.rebuild-darwin: 1 10.rebuild-linux: 1-10 10.rebuild-linux: 1 11.by: package-maintainer
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@bbigras @doronbehar @nixos-discourse @Thra11 @uri-canva