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

Newlib fixes. #1278

Merged
merged 2 commits into from
Feb 24, 2019
Merged

Newlib fixes. #1278

merged 2 commits into from
Feb 24, 2019

Conversation

ischeinkman
Copy link
Contributor

No description provided.

@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 @gnzlbg (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.

@gnzlbg
Copy link
Contributor

gnzlbg commented Feb 23, 2019

@ischeinkman thank you for the PR !

One question, how are you building Rust for newlib? We are building libc for a lot of targets in CI to make sure that it at least compiles (check the ci/build.sh file). Would it be possible to target libc there? xargo is already set up for some targets that require it, it might be as easy as adding a newlib target that rustc knows, or adding a target-specification file that it can pick up.

(EDIT: the same applies to the switch target, would be great to set it up also for it)

@ischeinkman
Copy link
Contributor Author

I am personally using the libnx-rs toolchain, which relies on a custom target spec, custom rustflags, and even a patched gcc provided by DevkitPro. However I do have a docker image in there that should get all of that set up by itself, provided that the CI infrastructure can support something like that. However, @roblabla might be able to provide something a bit more friendly, since their toolchain is completely self-contained.

As for the Switch target, the entire situation is strange due to the 2 different toolchains having different values for target_os, so I can't be much help there in terms of CI; libnx-rs uses target_os="horizon", which is the internal name of the OS itself (which is also shared another Nintendo console, the 3ds).

@gnzlbg
Copy link
Contributor

gnzlbg commented Feb 23, 2019

So the first step would be to just test that libc builds without errors for these targets, a custom json spec + xargo might be enough to test that. Could you try doing a xargo build of libc with that custom spec? If that works, it would be as easy as adding it to the ci/.. folder, and adding this target to the linux nightly list in the ci/build.sh.

@ischeinkman
Copy link
Contributor Author

So it would seem that not only did the standard xargo build with the target spec I'm using work, but it also discovered another case of a repeated definition.

@ischeinkman ischeinkman changed the title Removed newlib struct duplicates. Newlib fixes. Feb 24, 2019
@gnzlbg
Copy link
Contributor

gnzlbg commented Feb 24, 2019

Once the build is green, could you try adding the spec file to the ci folder and adding the target to the build.sh and see if that works? That way we can make sure that we won't break this target with future changes.

@gnzlbg
Copy link
Contributor

gnzlbg commented Feb 24, 2019

@bors: r+

@bors
Copy link
Contributor

bors commented Feb 24, 2019

📌 Commit 372ae79 has been approved by gnzlbg

@bors
Copy link
Contributor

bors commented Feb 24, 2019

⌛ Testing commit 372ae79 with merge a5c20d6...

bors added a commit that referenced this pull request Feb 24, 2019
@gnzlbg
Copy link
Contributor

gnzlbg commented Feb 24, 2019

So I'm merging this, but without a way to make sure that this target compiles, nothing will prevent it from breaking again.

@bors
Copy link
Contributor

bors commented Feb 24, 2019

☀️ Test successful - checks-cirrus, checks-travis, status-appveyor
Approved by: gnzlbg
Pushing a5c20d6 to master...

@bors bors merged commit 372ae79 into rust-lang:master Feb 24, 2019
@leo60228 leo60228 mentioned this pull request Mar 4, 2019
bors added a commit that referenced this pull request Mar 5, 2019
Bump to 0.2.50

I'm using libnx-rs too, and it'd be nice to have #1278 in a release.
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