Skip to content

Remove stable cargo feature default-run #827

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
Merged
bors merged 2 commits into from
Jul 6, 2019
Merged

Remove stable cargo feature default-run #827

bors merged 2 commits into from
Jul 6, 2019

Conversation

@tesuji
Copy link
Contributor

@tesuji tesuji commented Jul 5, 2019

At least on beta.

@RalfJung
Copy link
Member

RalfJung commented Jul 5, 2019

We can only do this once the bootstrap-cargo used by rustc also has this as a stable feature. Is that the case?

@RalfJung
Copy link
Member

RalfJung commented Jul 5, 2019

Looks like that is

$ build/x86_64-unknown-linux-gnu/stage0/bin/cargo --version
cargo 1.37.0-beta (4c1fa54d1 2019-06-24)

When did default-run get stabilized?

@tesuji
Copy link
Contributor Author

tesuji commented Jul 5, 2019 •
edited
Loading

I think it is https://static.rust-lang.org/dist/2019-07-04/rustc-beta-x86_64-unknown-linux-gnu.tar.gz .
I got warning when building rustc.

@RalfJung
Copy link
Member

RalfJung commented Jul 5, 2019

Yeah looks like it is new enough then. Thanks!

@bors r+

@bors
Copy link
Contributor

bors commented Jul 5, 2019

📌 Commit afdd8a3 has been approved by RalfJung

@RalfJung
Copy link
Member

RalfJung commented Jul 5, 2019

Actually, this seems to fail on CI. I think CI uses stable cargo currently.

@bors r-

@tesuji
Copy link
Contributor Author

tesuji commented Jul 5, 2019

Hm, I should update the rust-toolchain file then.

@RalfJung
Copy link
Member

RalfJung commented Jul 5, 2019

You could try changing that to beta. The relevant lines are

miri/.travis.yml

Line 34 in 3525943

- curl https://build.travis-ci.org/files/rustup-init.sh -sSf | sh -s -- -y --default-toolchain stable

miri/.appveyor.yml

Line 22 in 3525943

- rustup-init.exe -y --default-host %TARGET% --default-toolchain stable

In that case please also open an issue so that we remember to change it back to stable when that becomes possible.

@RalfJung
Copy link
Member

RalfJung commented Jul 5, 2019

Hm, I should update the rust-toolchain file then.

No, that has no effect on the cargo we use. It just determines the rustc.

@tesuji
Copy link
Contributor Author

tesuji commented Jul 5, 2019

I think it should work now.

@RalfJung
Copy link
Member

RalfJung commented Jul 5, 2019 •
edited
Loading

No that will definitely not work. Miri needs lots of updating to work with latest rustc. And even then it will still use stable cargo. Please see my messages above. :)

@tesuji
Copy link
Contributor Author

tesuji commented Jul 5, 2019

I would prefer to defer this PR until we sort out issue with latest rustc in
other PRs.

@RalfJung
Copy link
Member

RalfJung commented Jul 5, 2019

But why? This is about updating cargo. The other PRs are about updating rustc. Those two are updated entirely independently for our CI.

Fixing this PR is trivial, and I showed you the two lines you will have to change to make that work. :)

@tesuji tesuji mentioned this pull request Jul 5, 2019
Closed
@tesuji
Copy link
Contributor Author

tesuji commented Jul 5, 2019

Thanks. My assumption is wrong. I opened a tracking issue in #828.

@tesuji
Copy link
Contributor Author

tesuji commented Jul 5, 2019

Hm, CI still uses cargo from master to build.

@RalfJung
Copy link
Member

RalfJung commented Jul 5, 2019

Hm, either that or it didn't actually install beta. The curl https://build.travis-ci.org/files/rustup-init.sh -sSf | sh -s -- -y --default-toolchain beta step was suspiciously fast.

@RalfJung
Copy link
Member

RalfJung commented Jul 5, 2019

Maybe try removing the -c cargo from the rustup-toolchain-install-master line?

@RalfJung
Copy link
Member

RalfJung commented Jul 5, 2019

Looks good in PR CI. Can you squash, then I will r+?

@tesuji
Copy link
Contributor Author

tesuji commented Jul 5, 2019

Done.

RalfJung
RalfJung reviewed Jul 5, 2019
View reviewed changes
.travis.yml Outdated
before_cache:
# Don't cache "master" toolchain, it's a waste
- rustup default stable
- rustup default beta
Copy link
Member

@RalfJung RalfJung Jul 5, 2019

Choose a reason for hiding this comment

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

Good catch!

@RalfJung
Copy link
Member

RalfJung commented Jul 5, 2019

Great!

@bors r+

@bors
Copy link
Contributor

bors commented Jul 5, 2019

📌 Commit 1da9fea has been approved by RalfJung

@bors
Copy link
Contributor

bors commented Jul 5, 2019

⌛ Testing commit 1da9fea with merge afdf678...

bors added a commit that referenced this pull request Jul 5, 2019
@bors
Auto merge of #827 - lzutao:patch-1, r=RalfJung
afdf678 
Remove stable cargo feature `default-run`

At least on beta.
@bors
Copy link
Contributor

bors commented Jul 5, 2019

💔 Test failed - checks-travis

@tesuji
Copy link
Contributor Author

tesuji commented Jul 5, 2019

Need another bors retry to get it work. Previous fail build only cache beta toolchain.

@RalfJung
Copy link
Member

RalfJung commented Jul 5, 2019

I doubt this will work TBH, does the cache get updated when the build fails?

@bors retry

@bors
Copy link
Contributor

bors commented Jul 5, 2019

⌛ Testing commit 1da9fea with merge 9411573...

bors added a commit that referenced this pull request Jul 5, 2019
@bors
Auto merge of #827 - lzutao:patch-1, r=RalfJung
9411573 
Remove stable cargo feature `default-run`

At least on beta.
RalfJung
RalfJung reviewed Jul 5, 2019
View reviewed changes
@bors
Copy link
Contributor

bors commented Jul 5, 2019

💔 Test failed - status-appveyor

@RalfJung
Copy link
Member

RalfJung commented Jul 6, 2019

Turns out I was wrong, switching to beta cargo does not work. It leads to strange failures on Windows: #829.

@RalfJung
Copy link
Member

RalfJung commented Jul 6, 2019

So given that rust-version has been bumped by other PRs, maybe we try your original strategy? I forgot that we did -c cargo to actually install the nightly cargo.

@tesuji
Copy link
Contributor Author

tesuji commented Jul 6, 2019

Rebased on master.

@RalfJung
Copy link
Member

RalfJung commented Jul 6, 2019

Travis looks good!

@bors r+

@bors
Copy link
Contributor

bors commented Jul 6, 2019

📌 Commit 96be924 has been approved by RalfJung

@bors
Copy link
Contributor

bors commented Jul 6, 2019

⌛ Testing commit 96be924 with merge 67fd5a4...

bors added a commit that referenced this pull request Jul 6, 2019
@bors
Auto merge of #827 - lzutao:patch-1, r=RalfJung
67fd5a4 
Remove stable cargo feature `default-run`

At least on beta.
@bors
Copy link
Contributor

bors commented Jul 6, 2019

☀️ Test successful - checks-travis, status-appveyor
Approved by: RalfJung
Pushing 67fd5a4 to master...

@bors bors merged commit 96be924 into rust-lang:master Jul 6, 2019
@tesuji tesuji deleted the patch-1 branch July 6, 2019 10:26
@RalfJung
Copy link
Member

RalfJung commented Jul 6, 2019

Yay, that worked. :D Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@RalfJung RalfJung RalfJung left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@tesuji @RalfJung @bors