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

Fix for rust-lang/rust issue #50583 #1172

Merged
merged 1 commit into from
Dec 11, 2018

Conversation

piersfinlayson
Copy link
Contributor

Rationale for this fix is in the issue - see rust-lang/rust#50583.

To test I've created a container with the new Dockerfile and verified that the libc.a doesn't have the illegal instruction reported in that issue. Not sure how to do a more comprehensive test of the CI that presumably produces a liblibc.rlib with this libc.a in it though.

@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! Did this mean to go to rust-lang/rust though?

@piersfinlayson
Copy link
Contributor Author

Sorry if I've followed the wrong process here - this is my first PR for a submodule. Should I submit the pull request via rust-lang/rust by updating src/liblibc to point to my forked version of libc?

@gnzlbg
Copy link
Contributor

gnzlbg commented Dec 11, 2018

What do you expect to achieve with this PR? This will change how libc is tested here, but that won't affect how the libc that's shipped via rust-lang/rust is compiled.

Also, nothing was failing here on CI, so the fix is unlikely to catch whatever error it attempts to catch. It might be worth it to add a test that catches the error such that CI here fails, and then the fix verifying that it actually fixes the issue.

@piersfinlayson
Copy link
Contributor Author

@gnzlbg could you or someone else share how the liblibc.rlib - that gets delivered as part of the relevant target when installing via rustup - gets built? I assumed it was via this container (as this is the only thing I've found in the rust source which generates a libc.a - which the delivered musl liblibc.rlib is statically linked with), but don't quite understand how the rust CI pipeline and deliver fits together.

@gnzlbg
Copy link
Contributor

gnzlbg commented Dec 11, 2018

When building rust-lang/rust, the git submodules are fetched, which includes fetching some commit from this repository. Then that commit of libc is built there as part of building the rust standard library (libstd).

You'd have to ask @alexcrichton about which service and containers are used to actually build the release artifacts that are distributed via rustup, but I'd suppose that's done on Travis-CI, using the docker containers in the rust-lang/rust repo: https://github.com/rust-lang/rust/tree/master/src/ci/docker

It does make sense to keep the containers here and there in sync, so it might make sense to keep this PR open until that's resolved, but doing a change here does not change anything about how the libc shipped to users via rustup is compiled.

@piersfinlayson
Copy link
Contributor Author

piersfinlayson commented Dec 11, 2018

When building rust-lang/rust from source I sadly can't seem to persuade the liblibc.rlib to statically link with musl libc.a using the appropriate arm target. But it's mostly the release artifacts I'm interested in getting corrected so I don't even need to build rust from source.

Pretty sure that the rust (as opposed to libc) CI docker containers don't deal with the non-x86 musl targets which is why I've surmised it is the libc one which is the culprit for armv6 musl.

@alexcrichton
Copy link
Member

Oh sure yeah I can help you there! @gnzlbg is right in that the docker files in this repository exclusively affect this crate's CI, not the binaries we distribute or the rust-lang/rust repository.

For rust-lang/rust the src/ci/docker folder is what you're looking for, specifically the dist-various-1 container and probably a line somewhere in here.

The libc.a built for musl is included later in liblibc.rlib, so changing the original musl build will affect those later artifacts

@piersfinlayson
Copy link
Contributor Author

Great thanks. I'll leave this PR open, as I think it would be sensible to take the same fix to this repo for consistency, and I'll submit a PR to the rust repo.

@alexcrichton alexcrichton merged commit c66221d into rust-lang:master Dec 11, 2018
@alexcrichton
Copy link
Member

Ok!

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.

4 participants