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

Cargo update (almost) all the things! #75555

Merged
merged 5 commits into from Aug 19, 2020

Conversation

workingjubilee
Copy link
Contributor

@workingjubilee workingjubilee commented Aug 15, 2020

This runs cargo update on our dependency tree, bumping numerous crates. See details in the first commit of this PR.

Several updates were held back intentionally; version_check in particular landed a change in behavior in 0.9.2 (arguably a bug fix, but still will be split into a separate PR).

@rust-highfive
Copy link
Collaborator

r? @Mark-Simulacrum

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 15, 2020
@Mark-Simulacrum
Copy link
Member

This is unfortunately basically not going to be tenable. I try to review cargo updates by glancing over the diffs between the crate versions, and with so many crates updated that's going to be quite hard, especially if I need to go track down all of the diffs myself.

Is there a specific reason for these updates? I recall seeing a few PRs in my queue with regards to this -- I think something about bumping yanked dependencies? Is there a reason we've gone to bumping everything from that?

@workingjubilee
Copy link
Contributor Author

workingjubilee commented Aug 17, 2020

Many deps of rustc that are on older versions are also depending on older versions of other crates which predate various important patches, where newer versions do not satisfy Cargo's default dependency algorithm. After reviewing several crates I came to the conclusion that a one-by-one audit would be quite tedious as there are... many crates, as mentioned. On discussing this with other contributors, it seemed like it might be wise to PR an update of Many, especially as it's almost entirely patch versions, so I went ahead with it.

If I should reevaluate and use a different approach then I am happy to do so.
A couple of those other PRs are going to be more or less empty space because I learned things as I opened them.

@Mark-Simulacrum
Copy link
Member

I pushed up an update done locally (with a list embedded into the commit message). That is failing CI on the parallel-compiler build, though, and I'm not quite certain yet what is causing that. Trying to investigate now.

@workingjubilee
Copy link
Contributor Author

? I thought I included a list but I suppose I should have made it more finely itemized.

@workingjubilee
Copy link
Contributor Author

Oh, yes, one of the things I learned in my flailing with other PRs is that this error happened when I updated once_cell 1.1.0 -> 1.4.0, that's why I left it out of the initial commit here.

@Mark-Simulacrum
Copy link
Member

@matklad -- looks like something in matklad/once_cell@v1.3.1...v1.4.0 is a breaking change.

@matklad
Copy link
Member

matklad commented Aug 17, 2020

@Mark-Simulacrum oh wow, do you happen to have a link to broken build by any chance?

@workingjubilee
Copy link
Contributor Author

@matklad The broken build is here, the Queries struct uses OnceCell, this appears to err only on the mingw (parallel compiler?) build. https://github.com/rust-lang/rust/runs/994274395?check_suite_focus=true#step:23:486

@Mark-Simulacrum
Copy link
Member

@matklad managed to reproduce and has a MCVE -- we'll be only upgrading to 1.3.1 for now though.

@Mark-Simulacrum
Copy link
Member

Okay, it looks like the only problem after some manual checking is tinyvec by @Lokathor which is licensed as Zlib. It's mentioned by @Lokathor as "should be allowed" here. @joshtriplett, can you comment on that?

@workingjubilee
Copy link
Contributor Author

Pulling unicode-normalization back a patch would allow it to use the latest patch of smallvec instead if that is a problem, but does seem like just deferring the question if it can be resolved in-the-moment.

@Lokathor
Copy link
Contributor

Lokathor commented Aug 17, 2020

Hello.

Please note that not only should Zlib code be allowed, but also that tinyvec uses Zlib OR Apache-2.0 OR MIT, thus even if Zlib isn't permitted at this time it's still eligible for use here because it's an OR list.

@workingjubilee
Copy link
Contributor Author

It looks like the dependency is on 0.3.3, before the "official" (as far as Cargo.toml cares) change to Zlib OR Apache-2.0 OR MIT in 0.4.0.

@Mark-Simulacrum
Copy link
Member

@Lokathor Could you release a 0.3.4 with the license updated perhaps? Then we can bump here and be 100% confident it's acceptable.

@Lokathor
Copy link
Contributor

Oh that's very unfortunate.

And tinyvec is borderline going to be 1.0 actually. I was going to release the 1.0 on the next stable rust release. I could do it sooner if you'd like tinyvec to just be 1.0 now and then you can move unicode-normalization to that all in one jump. The breaks between 0.3, 0.4, and 1.0 are mostly edge case breaks only, so the unicode-normalization update should be easy either way.

Let me know what you want to do, I could put out the 1.0 within say 12 hours if you want to go with that route.

@Lokathor
Copy link
Contributor

whoops, posted at the same time.

@Mark-Simulacrum I can probably put out a 0.3.4 later today, yes.

matklad added a commit to matklad/once_cell that referenced this pull request Aug 17, 2020
This fixes a regression from 1.3.1 to 1.4.0.

See
rust-lang/rust#75555 (comment)
and
https://doc.rust-lang.org/nomicon/dropck.html

Arrrrrrrrrrrrrrrrrrrrrr!
@Lokathor
Copy link
Contributor

tinyvec-0.3.4 should be up now.

@matklad
Copy link
Member

matklad commented Aug 17, 2020

I've released the new version of OnceCell that fixes a bug. I've also submitted #75648 to fix the same bug in stdlib (where the fix is more principled). I think, after that PR is merged, we should switch rustc to use standard lazy.

matklad added a commit to matklad/rust that referenced this pull request Aug 17, 2020
See the failed build in

rust-lang#75555 (comment)

for an example where we need this in real life
cmake-rs@8141f0e changed the logic for handling asm compiler flags.
This change was pulled in with the cmake 0.1.42 -> 0.1.44 update.

This introduced two new flags to the LLVM build, breaking it:
"-DCMAKE_ASM_FLAGS= -ffunction-sections -fdata-sections -fPIC -m64"
"-DCMAKE_ASM_COMPILER=/usr/bin/cc"

This patch should resolve the breakage by handling it in bootstrap.
@workingjubilee
Copy link
Contributor Author

I was using a system LLVM to spare my processor from melting. 😅

Thank you. If that corrects it then we are good to go, if it breaks still then it is probably best to revert the cmake update and split it out into a new debugging process.

@Mark-Simulacrum
Copy link
Member

Let's try it out. @bors r+

@bors
Copy link
Contributor

bors commented Aug 18, 2020

📌 Commit 8c40426 has been approved by Mark-Simulacrum

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 18, 2020
@workingjubilee
Copy link
Contributor Author

Seems to work OK on local.

@bors
Copy link
Contributor

bors commented Aug 19, 2020

⌛ Testing commit 8c40426 with merge 1656582...

@bors
Copy link
Contributor

bors commented Aug 19, 2020

☀️ Test successful - checks-actions, checks-azure
Approved by: Mark-Simulacrum
Pushing 1656582 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Aug 19, 2020
@bors bors merged commit 1656582 into rust-lang:master Aug 19, 2020
@workingjubilee workingjubilee deleted the update-everything branch August 19, 2020 03:24
@jyn514
Copy link
Member

jyn514 commented Aug 19, 2020

This resulted in an extra 9 dependencies for rustdoc; before it was 68, now it's 77. Is that an expected change?

@jyn514
Copy link
Member

jyn514 commented Aug 19, 2020

There seem to be more for the compiler itself too but I don't remember what it used to be. It's 216 crates now (including librustc_*).

@Mark-Simulacrum
Copy link
Member

I mean "no" but also it doesn't seem like dependency count is a problem in and of itself? We certainly added a few crates here and there, I think most of which are really not needed for the compiler but rather provide std-equivalent functionality for older Rust versions.

I am happy to see follow-up PRs to the ecosystem and compiler if there's things that can be dropped, of course, by replacing with std or so. (Obviously ecosystem PRs I likely can't approve :)

@jyn514
Copy link
Member

jyn514 commented Aug 19, 2020

I'll open a separate issue for removing duplicate dependencies (hopefully it won't be open as long as rust-lang/docs.rs#459 😆).

@jyn514
Copy link
Member

jyn514 commented Aug 19, 2020

#75704

tmandry added a commit to tmandry/rust that referenced this pull request Aug 19, 2020
Make OnceCell<T> transparent to dropck

See the failed build in

rust-lang#75555 (comment)

for an example where we need this in real life

r? @ghost
tmandry added a commit to tmandry/rust that referenced this pull request Aug 19, 2020
Make OnceCell<T> transparent to dropck

See the failed build in

rust-lang#75555 (comment)

for an example where we need this in real life

r? @ghost
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Aug 22, 2020
Per rust-lang#75555 (comment)
Zlib license might be OK. "OR Apache-2.0 OR MIT" definitely is.
unicode-normalization depends on this and rustc_parse, clippy,
and many other things depend on unicode-normalization.
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Aug 22, 2020
See the failed build in

rust-lang#75555 (comment)

for an example where we need this in real life
@ecstatic-morse
Copy link
Contributor

As expected, this PR caused a small regression in instruction counts, mostly concentrated in the deep-vector and unicode-normalization benchmarks, with up to a 1% regression on full check builds of unicode-normalization.

Ideally, we would do small batches of updates more frequently instead of a huge cargo update so we could better bisect bugs and/or perf regressions. Splitting those up seems like a pretty thankless task, however.

@workingjubilee
Copy link
Contributor Author

Hm. I was going to mention that unicode-normalization changed a key dependency in this PR, but then I noticed the perf runs are against a vendored version, so that doesn't matter I guess?

I'm still mildly surprised that deep-vector experienced any regressions whatsoever. Feels unintuitive.

It would probably be nice if rustc had a "dependabot"-esque automation for its key dependencies, except slightly more... leisurely than the behavior of dependabot. That would make splitting up dependency updates easier... cargo update, compile test, PR, once in a while? At least once every six weeks.

@Mark-Simulacrum
Copy link
Member

I think we want to update roughly once a cycle at least, which we haven't been doing, but probably should. https://forge.rust-lang.org/release/process.html#update-dependencies-t1-day-friday technically exists but I never actually do it (and I think @pietroalbini has not either, historically).

@workingjubilee
Copy link
Contributor Author

"Day after release" is going to be a weird position to put a task since it's emotionally a resolved issue the previous day, ね? This needs to be something that either happens before the release or as someone else's job.

@jyn514
Copy link
Member

jyn514 commented Aug 25, 2020

If it happens just before the release, you're suddenly changing hundreds (possibly thousands) of lines of code without giving it time to bake on nightly. I don't think that's a great idea.

@jyn514
Copy link
Member

jyn514 commented Aug 25, 2020

It also doesn't give you time to resolve any possible issues, because there's a time crunch to get the release out.

@workingjubilee
Copy link
Contributor Author

Oh yeah, it would have to be at least a week before the release, but also a week after the release is probably more plausible (since you're no longer saying "right after you're done, you're not done", you're saying "so as you start the next cycle...")

@Mark-Simulacrum
Copy link
Member

IMO there is no worry about making it happen within the release week or so, I don't think there's any feeling in at least my mind that after we've done a release we're done (e.g., we need to enqueue crater run for beta immediately, ideally).

If we can start doing this and get into the habit of doing so I think it will work just fine. We just need to form that habit. It is plausible that the release team will try to select someone not in charge of releases to do this.

I have added this to the release team meeting agenda for this Friday to discuss -- feel free to open up a thread on Zulip if you have preliminary discussion or points you'd like to see addressed.

@cuviper cuviper added this to the 1.47.0 milestone May 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet