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

rustc: add support for armv7l targets #72480

Closed
wants to merge 1 commit into from
Closed

rustc: add support for armv7l targets #72480

wants to merge 1 commit into from

Conversation

urbas
Copy link
Contributor

@urbas urbas commented Nov 1, 2019 •
edited
Loading

Motivation for this change

rustc and Rust packages would not compile on Raspbian on a Raspberry Pi. It turns out that Rust's ARMv7 targets are slightly different from Nix's.

Addresses issue #72473

Things done

This change adds a function that maps armv7a and armv7l architectures to armv7. The former are used in Raspbian on Raspberry Pi but are unknown to rustc. Rust knows only armv7.

In addition, this change also disables the m_hugefile test in e2fsprogs as it was failing on ARMv7 Raspbian on the Raspberry Pi.

  • 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 nix-review --run "nix-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.
Notify maintainers

cc @edolstra @madjar @cstrahan @globin @Havvy

@urbas urbas requested review from LnL7 and Mic92 as code owners November 1, 2019 20:32
@urbas urbas force-pushed the feature/armv7l-raspbian-support-for-rust branch 2 times, most recently from b5b9ee4 to 09d24d1 Compare November 1, 2019 20:50
@urbas urbas requested a review from Mic92 November 1, 2019 20:53
Mic92
Mic92 approved these changes Nov 1, 2019
View reviewed changes
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.

Have not tested, but code seems fine.

@ofborg ofborg bot added the 6.topic: rust label Nov 1, 2019
@urbas urbas mentioned this pull request Nov 2, 2019
Closed
Gerschtli
Gerschtli approved these changes Nov 3, 2019
View reviewed changes
Copy link
Contributor

@Gerschtli Gerschtli left a comment

Choose a reason for hiding this comment

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

Haven't tested it either, but diff LGTM.

@doronbehar
Copy link
Contributor

BTW Rust 1.39.0 was released today so it might worth updating it here as well.

@edolstra
Copy link
Member

edolstra commented Nov 7, 2019

That's already done in #72980.

@Mic92
Copy link
Member

Mic92 commented Nov 8, 2019

@GrahamcOfBorg build rustc

lopsided98
lopsided98 suggested changes Nov 9, 2019
View reviewed changes
Copy link
Contributor

@lopsided98 lopsided98 left a comment

Choose a reason for hiding this comment

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

I wrote a similar patch a while ago to add support for armv7l and armv6l, see: lopsided98@d6d2c1c. My patch fixes the target in a few more places that you missed, but I missed the one in build-crate.nix.

pkgs/development/compilers/rust/default.nix Outdated
x86_64-pc-mingw32 = "x86_64-pc-windows-gnu";
}.${nixTarget} or (
let
armMatch = builtins.match "armv7[al]-(.*)" nixTarget;
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems like you could make this simpler if you passed in the entire platform instead of just the config string. The platform elaboration logic has already parsed out all the components you need, so you don't need to do it again. You could then make your function more like the one previously used in build-crate.nix

Copy link
Contributor

@lopsided98 lopsided98 Nov 9, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Something like this: lopsided98@249d11c#diff-4100057d032c0d790107c2739448233cR8

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My fixes look extremely similar to yours. I also see they well predate mine. I am sorry, I wasn't aware of them.

Btw, I also agree with your suggestions. I can fix this PR. However, I can also retract this pull request so you can go ahead and create yours. Let me know which way you'd prefer.

Copy link
Member

Choose a reason for hiding this comment

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

How will you continue?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll apply the fixes @lopsided98 recommended.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I now applied the fixes.

pkgs/tools/filesystems/e2fsprogs/default.nix Outdated Show resolved Hide resolved
@urbas urbas force-pushed the feature/armv7l-raspbian-support-for-rust branch from 09d24d1 to 16a73c0 Compare November 10, 2019 11:59
@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux and removed 10.rebuild-darwin: 11-100 10.rebuild-linux: 501+ labels Nov 10, 2019
@urbas urbas force-pushed the feature/armv7l-raspbian-support-for-rust branch 2 times, most recently from f4b514d to a3cc991 Compare November 15, 2019 08:46
@urbas
rustc: add support for armv7l targets
cfc9800 
Addresses issue NixOS#72473

`rustc` and Rust packages would not compile on Raspbian on a Raspberry Pi. It turns out that Rust's ARMv7 targets are slightly different from Nix's.

This change adds a function that maps `armv7a` and `armv7l` architectures to `armv7`. The former are used in Raspbian on Raspberry Pi but are unknown to rustc. Rust knows only `armv7`.
@urbas urbas force-pushed the feature/armv7l-raspbian-support-for-rust branch from a3cc991 to cfc9800 Compare November 15, 2019 19:09
@ofborg ofborg bot requested a review from Gerschtli November 15, 2019 19:26
@ofborg ofborg bot added 10.rebuild-darwin: 501+ 10.rebuild-darwin: 501-1000 and removed 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin labels Nov 15, 2019
@lopsided98
Copy link
Contributor

Sorry for not responding, but I can submit my own version of the PR if you want. I see that you integrated most of it already, but I noticed a couple of things still missing (in .cargo/config and bootstrapping hashes).

Also, we put our mapping functions inside different attrsets (rust for me vs. rustPlatform for you). This doesn't much functional difference, but it might be good to get the opinion of a Rust maintainer about where it belongs.

@lopsided98 lopsided98 mentioned this pull request Nov 15, 2019
Merged
10 tasks
@lopsided98
Copy link
Contributor

I submitted it: #73472

@urbas
Copy link
Contributor Author

urbas commented Nov 15, 2019 via email

@lopsided98
Copy link
Contributor

This PR has the same problem, ofborg just doesn't catch it. On x86_64 macOS, the toRustTarget function returns x86_64-apple-darwin-unknown, which will probably cause the rustc build to fail because that is not a target triple Rust knows about.

@lopsided98
Copy link
Contributor

Here is my updated mapping function: https://github.com/NixOS/nixpkgs/pull/73472/files#diff-4100057d032c0d790107c2739448233cR14

It is not the most elegant, but it works for all the platforms that we have bootstrap tarballs for, as well as MinGW (which actually did not need a special case) and armv7a.

@urbas
Copy link
Contributor Author

urbas commented Nov 16, 2019

@lopsided98: Sounds much better.

I'm throwing this PR out in favour of #73472

@urbas urbas closed this Nov 16, 2019
@urbas urbas deleted the feature/armv7l-raspbian-support-for-rust branch December 29, 2020 18:42
@Janik-Haag Janik-Haag added the 12. first-time contribution This PR is the author's first one; please be gentle! label Jun 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@FRidh FRidh FRidh left review comments

@lopsided98 lopsided98 lopsided98 requested changes

@matthewbauer matthewbauer matthewbauer approved these changes

@Mic92 Mic92 Mic92 approved these changes

@Gerschtli Gerschtli Gerschtli approved these changes

@LnL7 LnL7 Awaiting requested review from LnL7

@globin globin Awaiting requested review from globin

@cstrahan cstrahan Awaiting requested review from cstrahan

@madjar madjar Awaiting requested review from madjar

@edolstra edolstra Awaiting requested review from edolstra

Assignees
No one assigned
Labels
6.topic: rust 10.rebuild-darwin: 501-1000 10.rebuild-darwin: 501+ 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux 12. first-time contribution This PR is the author's first one; please be gentle!
Projects
Staging
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@urbas @doronbehar @edolstra @Mic92 @lopsided98 @matthewbauer @FRidh @Gerschtli @Janik-Haag