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

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 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
5 of 10 tasks complete
@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
@doronbehar
Copy link
Contributor

doronbehar commented May 22, 2020

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.

@doronbehar
Copy link
Contributor

doronbehar commented May 22, 2020

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

doronbehar commented May 22, 2020

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

doronbehar commented May 22, 2020

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 bbigras force-pushed the bbigras:zenith-aarch64 branch from c12a3e4 to b47bc18 May 23, 2020
@bbigras
Copy link
Contributor Author

bbigras commented May 23, 2020

I rebased the PR.

@GrahamcOfBorg build zenith

@nixos-discourse
Copy link

nixos-discourse commented May 24, 2020

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 force-pushed the bbigras:zenith-aarch64 branch from 861ea38 to ff2e833 May 26, 2020
@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

nixos-discourse commented May 26, 2020

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 bbigras:zenith-aarch64 branch May 28, 2020
@@ -15,12 +15,15 @@ rustPlatform.buildRustPackage rec {

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

preBuild = ''
export CARGO_TARGET_AARCH64_UNKNOWN_LINUX_GNU_LINKER=gcc

This comment has been minimized.

Copy link
@Thra11

Thra11 Jun 7, 2020

Contributor

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
Contributor

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! 👍

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

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.