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 latest nightly: need to add a patch section #228

Merged
merged 11 commits into from
Dec 15, 2018

Conversation

RalfJung
Copy link
Collaborator

@RalfJung RalfJung commented Dec 13, 2018

Fixes #227

@jethrogb
Copy link
Collaborator

jethrogb commented Dec 14, 2018

I just filed #229 myself, I guess I should've looked through the PRs first :P

Anyway, you should update your PR to say "Fixes #227"

@jethrogb
Copy link
Collaborator

Anyway, this (and #229) don't work (fully). It works if you're using only the rust-src component or XARGO_RUST_SRC. However, if you're specifying the source in Xargo.toml, the dependency on rustc-std-workspace-core will point to the wrong libcore source.

I've come up with 3 different approaches to fix this, not sure which one is best:

  1. If there is a std crate in the blueprint, compute ../tools/rustc-std-workspace-core from that path (or use the same git) and use that as the [patch].

  2. If there is a std crate in the blueprint, find the spec of the core dependency, then generate a rustc-std-workspace-core crate in the tempdir with that core (how would this work if std is a git dependency?). Then, use that temp crate as the [patch].

  3. Use the logic in this PR or Fix build for latest Rust nightly source #229 to generate the [patch] by default. Let users specify their own [patch]es in Xargo.toml

@RalfJung
Copy link
Collaborator Author

I don't have a strong opinion about solving this problem.

However, I think we can merge one of #228 and #229 to fix the immediate problem for everyone using an unchanged libstd/XARGO_RUST_SRC, and then separately have an issue for fixing the other case?

@jethrogb
Copy link
Collaborator

I think this PR doesn't work for compiling core without Xargo.toml so I'd vote #229

@RalfJung
Copy link
Collaborator Author

I think this PR doesn't work for compiling core without Xargo.toml so I'd vote

Not sure hat you mean by that, but fine for me.

However, please make sure xargo still works for old libstd that does not have rustc-std-workspace-core yet.

@RalfJung
Copy link
Collaborator Author

Also I did the proper thing for fixing the deprecation warnings instead of just silencing them, feel free to copy those parts of my patches.

@jethrogb
Copy link
Collaborator

However, please make sure xargo still works for old libstd that does not have rustc-std-workspace-core yet.

When building std from a git source, how will you know whether to add the patch or not?

@RalfJung
Copy link
Collaborator Author

Can't you check whether the rustc-std-workspace-core folder exists? That's what this patch does.

@jethrogb
Copy link
Collaborator

How would that work for a git source? Xargo doesn't have access to the source.

@RalfJung
Copy link
Collaborator Author

I'm afraid I am not familiar with the mode of operation of xargo you are referring to. I have only ever used xargo with the rust-src component, and I think it is reasonable to keep xargo compatible with older Rust nightlies -- it's not much effort.

@japaric
Copy link
Owner

japaric commented Dec 15, 2018

Thanks for the PRs, @RalfJung and @jethrogb! I'm going to land this one since it works with older nightly releases. On that note: @RalfJung, could you add a new builder that runs the test suite on an older nightly? I think the change should boil down to adding these lines to .travis.yml:

matrix:
  include:
    - env: TARGET=x86_64-unknown-linux-gnu
      rust: nightly-2018-12-XX

Feel free to land this with that change.

bors delegate+

@bors
Copy link
Contributor

bors bot commented Dec 15, 2018

✌️ RalfJung can now approve this pull request

@RalfJung
Copy link
Collaborator Author

@japaric there was no CI in this PR at all though, for some reason?

@japaric
Copy link
Owner

japaric commented Dec 15, 2018

@RalfJung CI builds on PRs was disabled; I have re-enabled it.

@RalfJung
Copy link
Collaborator Author

@japaric Hm, still not doing anything.

@japaric
Copy link
Owner

japaric commented Dec 15, 2018

Hmm, I think you'll have to add master to the list of branches in .travis.yml.

I'll kick off a try build for now.

bors try

bors bot added a commit that referenced this pull request Dec 15, 2018
@japaric
Copy link
Owner

japaric commented Dec 15, 2018

Link to try build: https://travis-ci.org/japaric/xargo/builds/468425445

@bors
Copy link
Contributor

bors bot commented Dec 15, 2018

try

Build failed

@RalfJung
Copy link
Collaborator Author

Looks like CI is not in a good state...

E: Unable to locate package gcc-arm-none-eabi

@RalfJung
Copy link
Collaborator Author

ci/script.sh does nothing if the branch is "master"... seems like this is not intended to work for PRs. Odd.

bors try

bors bot added a commit that referenced this pull request Dec 15, 2018
@RalfJung
Copy link
Collaborator Author

bors try

bors bot added a commit that referenced this pull request Dec 15, 2018
@RalfJung
Copy link
Collaborator Author

Why is it not listening now?^^

bors try

@bors
Copy link
Contributor

bors bot commented Dec 15, 2018

try

Not awaiting review

@RalfJung
Copy link
Collaborator Author

Oh it did listen, just GH didn't update. Never mind.

@RalfJung
Copy link
Collaborator Author

Well that looks really green to me!

bors r+

bors bot added a commit that referenced this pull request Dec 15, 2018
228: Fix for latest nightly: need to add a patch section r=RalfJung a=RalfJung

Fixes #227

Co-authored-by: Ralf Jung <[email protected]>
@bors
Copy link
Contributor

bors bot commented Dec 15, 2018

try

Build failed

@RalfJung
Copy link
Collaborator Author

Uh, no, just the nightly-XXX didn't run the tests because it's not exactly "nightly".

bors r-

@bors
Copy link
Contributor

bors bot commented Dec 15, 2018

Canceled

@RalfJung
Copy link
Collaborator Author

bors try

bors bot added a commit that referenced this pull request Dec 15, 2018
@bors
Copy link
Contributor

bors bot commented Dec 15, 2018

try

Build succeeded

@RalfJung
Copy link
Collaborator Author

bors try

bors bot added a commit that referenced this pull request Dec 15, 2018
@bors
Copy link
Contributor

bors bot commented Dec 15, 2018

try

Build succeeded

@RalfJung
Copy link
Collaborator Author

Okay, now it actually tested something and the tests come back green. :)

@RalfJung
Copy link
Collaborator Author

bors r+

bors bot added a commit that referenced this pull request Dec 15, 2018
228: Fix for latest nightly: need to add a patch section r=RalfJung a=RalfJung

Fixes #227

Co-authored-by: Ralf Jung <[email protected]>
@bors
Copy link
Contributor

bors bot commented Dec 15, 2018

Build succeeded

@bors bors bot merged commit 00f4dec into japaric:master Dec 15, 2018
@thepowersgang
Copy link

When would we expect to see this change in a released version on crates.io?

@jethrogb
Copy link
Collaborator

I've filed #230 and #231 for the remaining issues.

@johnthagen johnthagen mentioned this pull request Dec 17, 2018
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