Skip to content

Be forward compat with rootless lock files #3023

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

Closed
wants to merge 1 commit into from

Conversation

matklad
Copy link
Member

@matklad matklad commented Aug 20, 2016

Hi! So what is the plan regarding the [root] key in the lock file?

With workspaces it is meaningless, but not immediately harmful.

I think it should be possible to remove it eventually if we teach Cargo to read rootless lockfiles now and switch to them several cycles later. Reading lockfiles with [root] should be supported forever of course.

This PR adds ability to read rootless lockfiles, but they are then replaced by the current format in write_pkg_lockfile. Should be easy to fix if we do want to evolve lockfile format.

@rust-highfive
Copy link

r? @alexcrichton

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

@alexcrichton
Copy link
Member

I like this approach! My plan was to just not do anything, but this seems like the correct way to approach this to me. I would indeed like to one day phase them out (as they don't make sense).

Looks like tests are failing though?

@matklad matklad force-pushed the rootless-lockfile branch from ac3213b to 051fb17 Compare August 20, 2016 21:42
@matklad
Copy link
Member Author

matklad commented Aug 20, 2016

Looks like tests are failing though?

Yep, the empty file is now a valid Cargo.toml.

if orig.starts_with("[[package]]") {
assert!(out.starts_with("[root]"));
out = format!("[[package]]{}", &out["[root]".len()..]);
}
Copy link
Member Author

Choose a reason for hiding this comment

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

This is interesting.

This is the case where we've read a Cargo.toml without [root] from the future. Presumably we want to preserve this rootless format.

But this approach may change the ordering of packages in the lockfile! This is not the best behavior. Hm, I think I can actually solve this...

@matklad matklad force-pushed the rootless-lockfile branch from 051fb17 to 5e644df Compare August 20, 2016 22:32
@matklad
Copy link
Member Author

matklad commented Aug 20, 2016

Should be ready now

@alexcrichton
Copy link
Member

Hm I think that an empty lock file should still be an error? If a package isn't listed as [root] it should be in [[package]]?

@matklad
Copy link
Member Author

matklad commented Aug 21, 2016

Hm I think that an empty lock file should still be an error?

I tend to think that empty lockfile is valid, albeit makes little sense. Lockfile can specify less packages than there are members in workspace, if it is not up to date. An empty lockfile specifies zero packages. Previously at least one package was required because of the mandatory root.

We can specifically bail for lockfile without packages, but I doubt that it will be useful in practice. Empty file is just a special case of a lockfile that needs updating.

@alexcrichton
Copy link
Member

To confirm, though:

  • cargo new followed by cargo build today emits [root]
  • cargo new followed by cargo build tomorrow would emit one [[package]]

right?

@alexcrichton
Copy link
Member

"tomorrow" being "when we actually decide to remove [root]

@matklad
Copy link
Member Author

matklad commented Aug 22, 2016

right?

Yep, though looks like there is no tests which verifies current lockfile format specifically.

@matklad
Copy link
Member Author

matklad commented Aug 23, 2016

Added a test for ^^^ here, if it is necessary: #3031

@alexcrichton
Copy link
Member

Ok, I'm gonna close in favor of #3031, thanks!

@matklad matklad deleted the rootless-lockfile branch December 14, 2016 15:27
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.

3 participants