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

Add support for arm-unknown-linux-musleabi{,hf} targets. #264

Merged
merged 1 commit into from
Apr 18, 2016

Conversation

timonvo
Copy link
Contributor

@timonvo timonvo commented Apr 15, 2016

These targets will be similar to the x86_64-unknown-linux-musl
target, in that they'll use MUSL libc to allow fully static binaries
to be generated. To remain consistent with the naming of existing
ARM targets, as well as with the standard naming of MUSL toolchains,
we'll use musleabi and musleabihf as the target environment
names (analogous to gnueabi and gnueabihf).

Most of these changes just extend the special casing for x86_64 MUSL
targets to the ARM ones as well.

@rust-highfive
Copy link

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @alexcrichton (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@alexcrichton
Copy link
Member

Thanks for the PR! The compiler doesn't currently recognize these triples (and transitively rustdoc doesn't) so this may have to leave out the doc links for now.

I also wonder if we should just define target_env as "musl" for all of these triples and provide the hard/soft float option via a different method... (cc @brson). This is fine to land for now though, it'd work in either situation :)

@timonvo
Copy link
Contributor Author

timonvo commented Apr 16, 2016

Got rid of the doc links for now. This whole separate repositories with interdepencies thing makes it quite difficult to land patches. :) I just figured landing preliminary support in the sub-repos first would be better than landing broken changes in the main repo first.

As for the target_env thing. Yes, I agree that the situation is getting unwieldy. However, in order to get my changes in I'd like to just go ahead anyway, and then improve later on. It's been taking quite some effort already to keep my changes up-to-date with master and I'd be grateful if I could finally get my preliminary changes landed.

These targets will be similar to the x86_64-unknown-linux-musl
target, in that they'll use MUSL libc to allow fully static binaries
to be generated. To remain consistent with the naming of existing
ARM targets, as well as with the standard naming of MUSL toolchains,
we'll use `musleabi` and `musleabihf` as the target environment
names (analogous to `gnueabi` and `gnueabihf`).

Most of these changes just extend the special casing for x86_64 MUSL
targets to the ARM ones as well.
@timonvo
Copy link
Contributor Author

timonvo commented Apr 16, 2016

Also fixed line wrap (80 chars now).

@joerg-krause
Copy link
Contributor

I really appreciate the support for musl on ARM and I know using eabi and eabihf is also used for musl based toolchains, but having three different kinds of target-envs for musl looks somehow weird and redundant.

@timonvo
Copy link
Contributor Author

timonvo commented Apr 16, 2016

It's no different from the three existing different target_envs of gnu, gnueabi, and gnueabihf though, all of which are also backed by the same glibc implementation yet have different target_envs. I agree that this doesn't quite feel right, but I feel like we shouldn't stray away from that existing convention either.

That being said, if this is really a dealbreaker I could:

  • use target_env="musl" instead. Note though that this means we should probably change the existing gnueabi/gnueabihf targets to use gnu as the target_env though, and that may incur some changes in the code as well.
  • add a new target variable (e.g. target_libc) to cover this case, independent of the target_env variable so that it can remain consistent with the toolchain naming conventions.

WDYT?

@alexcrichton
Copy link
Member

@bors: r+ ad72859

Thanks @timonvo! @joerg-krause I agree with the sentiment here, I definitely don't want to see #[cfg(target_env = "musl")] explode everywhere to cover all these as well. I think that @timonvo's diagnosis is spot on and my preferred solution as well is to change gnueabihf and gnueabi to only have "gnu" as the target_env, but I haven't though too too hard about doing this (it's a breaking change, so shouldn't be taken lightly).

In any case this is pretty minor for liblibc itself, we may as well help these targets come online in the first place anyway :). If we end up choosing to just use "musl" everywhere then we can revert this in libc in time.

@bors
Copy link
Contributor

bors commented Apr 17, 2016

⌛ Testing commit ad72859 with merge a56bb28...

bors added a commit that referenced this pull request Apr 17, 2016
Add support for arm-unknown-linux-musleabi{,hf} targets.

These targets will be similar to the x86_64-unknown-linux-musl
target, in that they'll use MUSL libc to allow fully static binaries
to be generated. To remain consistent with the naming of existing
ARM targets, as well as with the standard naming of MUSL toolchains,
we'll use `musleabi` and `musleabihf` as the target environment
names (analogous to `gnueabi` and `gnueabihf`).

Most of these changes just extend the special casing for x86_64 MUSL
targets to the ARM ones as well.
@bors
Copy link
Contributor

bors commented Apr 17, 2016

💔 Test failed - travis

@alexcrichton
Copy link
Member

Hm seems spurious, we'll fix tomorrow

@timonvo
Copy link
Contributor Author

timonvo commented Apr 17, 2016

Thanks for the review Alex!

@alexcrichton
Copy link
Member

@bors: retry

@bors
Copy link
Contributor

bors commented Apr 18, 2016

⌛ Testing commit ad72859 with merge 1e6a354...

bors added a commit that referenced this pull request Apr 18, 2016
Add support for arm-unknown-linux-musleabi{,hf} targets.

These targets will be similar to the x86_64-unknown-linux-musl
target, in that they'll use MUSL libc to allow fully static binaries
to be generated. To remain consistent with the naming of existing
ARM targets, as well as with the standard naming of MUSL toolchains,
we'll use `musleabi` and `musleabihf` as the target environment
names (analogous to `gnueabi` and `gnueabihf`).

Most of these changes just extend the special casing for x86_64 MUSL
targets to the ARM ones as well.
@bors
Copy link
Contributor

bors commented Apr 18, 2016

☀️ Test successful - status-appveyor, travis

@bors bors merged commit ad72859 into rust-lang:master Apr 18, 2016
@timonvo timonvo deleted the armmusleabi branch April 27, 2016 03:56
Susurrus pushed a commit to Susurrus/libc that referenced this pull request Mar 26, 2017
Add introduction/constants/enumeration/uninitialized to CONVENTIONS.

I have added new sections to the file. I would like the usual suspects to read them critically and offer critique.

When have agreed to add certain versions I will add Issues to track our progress in getting the code to follow the conventions. For two of the sections they already exist in rust-lang#254 and rust-lang#264.
Susurrus pushed a commit to Susurrus/libc that referenced this pull request Mar 26, 2017
Use libc constants in sys/signal.rs.

Work toward rust-lang#264.
Susurrus pushed a commit to Susurrus/libc that referenced this pull request Mar 26, 2017
mount: Use bindings from libc instead of our own

Ref rust-lang#264

Changing internal usage of `ffi::mount` and `ffi:umount` to make use of libc.
Susurrus pushed a commit to Susurrus/libc that referenced this pull request Mar 26, 2017
fcntl: Use bindings from libc instead of our own

**Un-finished** and looking for help. As rust-lang#264 was tagged "mentor, good first bug"

Ref rust-lang#264

I couldn't find `F_GET_SEALS`, or `F_ADD_SEALS` in libc.

Is this a case where I should be making a PR over there? I'm not sure where exactly to address it in libc.
danielverkamp pushed a commit to danielverkamp/libc that referenced this pull request Apr 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants