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

Delete Cargo.lock from this repo #4623

Merged
merged 1 commit into from
Oct 19, 2017
Merged

Conversation

alexcrichton
Copy link
Member

There's now a lock file upstream in rust-lang/rust so the one here isn't
actually used, and otherwise this crate is used as a dependency so the lock file
isn't respected anyway!

@rust-highfive
Copy link

r? @matklad

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

@bors
Copy link
Contributor

bors commented Oct 15, 2017

☔ The latest upstream changes (presumably #4624) made this pull request unmergeable. Please resolve the merge conflicts.

@matklad
Copy link
Member

matklad commented Oct 16, 2017

Ok, so this is how it works now, right?

  1. In rust-lang/rust repo, cargo is included as a submodule under tools/cargo.
  2. It is also included as a workspace member, which gives it the actual lockfile.
  3. When filing a PR against rust-lang/rust, Cargo test suite is executed. Also, RLS, which depends on Cargo, is tested as well.
  4. When filing a PR against rust-lang/cargo, RLS is not tested. Only when the Cargo submodule is updated we'll learn if it actually works.

I am a bit worried about two things:

  1. Looks like CI for Cargo now is not actually complete, because ideally we'd want to test with rust-lang/rust lockfile.

  2. Including Cargo as a workspace member does not seem entirely right for me because in theory Cargo should be a complete separate piece of software, with its own lockfile, and it could be a workspace itself even.

Not that I think there will be any real significant problems because of this, so r+ from me! (but there's a merge conflict).

@alexcrichton
Copy link
Member Author

@matklad you're correct on all counts! It's true that Cargo's CI is less comprehensive than Rust's, and it's true that different versions of dependencies are used. That being said though I think it's an undesriable goal for the two to exactly match up. The Rust CI is very comprehensive and trying to match it on Cargo likely mostly just amount to a waste of resources.

It's true that from time to time we'll have a bug sneak in that will make updating the submodule in rust-lang/rust more difficult (especially with the RLS) but so far it hasn't been too much of a problem in practice and I'd probably expect that to stay mostly the same over time.

@alexcrichton
Copy link
Member Author

Oh also note that we use a workspace with Cargo to ensure that we only build Cargo once with one the same version of all deps between the RLS and Cargo itself.

There's now a lock file upstream in rust-lang/rust so the one here isn't
actually used, and otherwise this crate is used as a dependency so the lock file
isn't respected anyway!
@alexcrichton
Copy link
Member Author

@bors: r=matklad

@bors
Copy link
Contributor

bors commented Oct 18, 2017

📌 Commit 5c9665f has been approved by matklad

@bors
Copy link
Contributor

bors commented Oct 18, 2017

⌛ Testing commit 5c9665f with merge b73cc00e704f04ddcdf180da71898e9f7de1f5a1...

This was referenced Oct 18, 2017
@bors
Copy link
Contributor

bors commented Oct 19, 2017

💥 Test timed out

@matklad
Copy link
Member

matklad commented Oct 19, 2017

@bors r+

@bors
Copy link
Contributor

bors commented Oct 19, 2017

💡 This pull request was already approved, no need to approve it again.

  • This pull request previously failed. You should add more commits to fix the bug, or use retry to trigger a build again.

@bors
Copy link
Contributor

bors commented Oct 19, 2017

📌 Commit 5c9665f has been approved by matklad

@bors
Copy link
Contributor

bors commented Oct 19, 2017

⌛ Testing commit 5c9665f with merge 92a3a4e...

bors added a commit that referenced this pull request Oct 19, 2017
Delete Cargo.lock from this repo

There's now a lock file upstream in rust-lang/rust so the one here isn't
actually used, and otherwise this crate is used as a dependency so the lock file
isn't respected anyway!
@bors
Copy link
Contributor

bors commented Oct 19, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: matklad
Pushing 92a3a4e to master...

@bors bors merged commit 5c9665f into rust-lang:master Oct 19, 2017
@alexcrichton alexcrichton deleted the remove-lock branch December 18, 2017 19:47
@ehuss ehuss added this to the 1.23.0 milestone Feb 6, 2022
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.

9 participants