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

Verify / update corresponding version for path dependencies #77

Closed
epage opened this issue Feb 10, 2019 · 12 comments · Fixed by #101
Closed

Verify / update corresponding version for path dependencies #77

epage opened this issue Feb 10, 2019 · 12 comments · Fixed by #101
Assignees
Labels
enhancement Improve the expected

Comments

@epage
Copy link
Collaborator

epage commented Feb 10, 2019

In a workspace, I need to define my dependencies as dep = { path = "../dep", version = "0.1.0" } but I forget (until cargo publish) to update the versions for my path dependencies (and it is really annoying).

I think building this into cargo-release would be a big help.

I view this as a subset of the work for #66. While that is for releasing an entire workspace, this will help with releasing parts of a workspace (and is needed for the entire workspace).

Configuration

  • What to do on path dependency
    • verify
    • bump to latest (has to check crates.io because local path dependency has probably been bumped post-publish)
    • bump to semver compatible version
@epage
Copy link
Collaborator Author

epage commented Feb 10, 2019

I'm willing and interested in implementing this but I feel like I would want to do some refactoring first to make my life easier and I want to make sure that is aligned with you all

  • Use a struct / serde for config and merge it and the flags together to make it easier to pass the state around (making it easier for me to add new configuration)
  • Use toml_edit for modifying Cargo.toml (its the format-preserving parser / generator that cargo-edit is using)

I think there was more but I'm forgetting it at the moment.

@sunng87
Copy link
Collaborator

sunng87 commented Feb 11, 2019

@epage Thanks! You are welcomed to send pull request for a refactoring. Only thing to remind is to send refactoring and feature implementation in separated pull requests. We can go with refactoring first.

@epage
Copy link
Collaborator Author

epage commented Feb 11, 2019

Sounds good and was planning on it. I just wanted to make sure the refactorings were aligned with you.

@epage epage mentioned this issue Mar 7, 2019
6 tasks
@epage epage added the enhancement Improve the expected label Mar 11, 2019
@epage
Copy link
Collaborator Author

epage commented Mar 14, 2019

So I've been thinking on this.

If we update our own dependencies when updating our own version, how do we know what dependency version to update to? Our dependency already has had the post-release version bump.

What might be better is if we update the dependencies of our dependents. We can offer the options

  • ignore
  • warn to remind them which dependents are now out of date
  • minimum version (1.0 -> 2.0 only)
  • maximum version (2.0 -> 2.0.1)

We can then leverage knowing what version we are about to publish when we do this. This also means that the tree is always in a theoretically good state rather than only converging to a good state on the release of a dependent.

@epage
Copy link
Collaborator Author

epage commented Mar 22, 2019

Been working on the code to modify version ranges and there are a lot more angles to this then I expected

We could

  • "Smart" route
    • Offer a mode to calculate the absolute minimal change for any given range predicate to make it compatible
      • This requires a lot of customization per predicate type
    • Offer a mode to update the range predicate to at least the new version
      • This is relatively easy
  • "Dumb route"
    • Offer to modify version range on (1) incompatibility or (2) always
    • No matter what, we always update the range predicate to the new version, preserving the user's precision

I'm leaning towards the dumb route. Its simple / straightforward to predict and implement

Next up, what happens when going from 0.0.x -> 0.1 or 0.x -> 1.0? Do we preserve precision (^0.x becomes ^1 rather than ^1.0) or fill in each part as is? Originally, I was leaning towards preserving precision. The problem is we are guessing the users intent. What if their predicate is ^0.x.y? is the intention to have 2 fields of precision or to have all fields specified? I'm thinking we go with the simple, predictable route for a situation that doesn't come up often.

epage added a commit to epage/cargo-release that referenced this issue Mar 29, 2019
epage added a commit to epage/cargo-release that referenced this issue Mar 29, 2019
epage added a commit to epage/cargo-release that referenced this issue Mar 30, 2019
epage added a commit to epage/cargo-release that referenced this issue Mar 30, 2019
epage added a commit to epage/cargo-release that referenced this issue Apr 4, 2019
@epage epage self-assigned this Apr 4, 2019
@dignifiedquire
Copy link

I am trying out the latest master, but this is still an issue for me, the path dependency does not get replaced, and I get an error from cargo publish

Running cargo publish on storage-proofs
    Updating crates.io index
error: all path dependencies must have a version specified when publishing.
dependency `logging-toolkit` does not specify a version

@epage
Copy link
Collaborator Author

epage commented Jun 4, 2019

Could you provide an example of your Cargo.toml?

It sounds like you only have a path dependency and not path + version dependency which I believe is the norm. For example, look at ripgrep's Cargo.toml

EDIT: What this issue was about and fixed was updating the version within a path+version dependency.

@dignifiedquire
Copy link

dignifiedquire commented Jun 4, 2019

You are right, I only have path = "../<create>" without a version.
When I add version = "0.1" this is the result that I get:

$ cargo release --skip-publish minor
Release version drop_struct_macro_derive 0.2.0? [y/N]
y
Update drop_struct_macro_derive to version 0.2.0
[prepare-release 3c36c50] chore(drop_struct_macro_derive): release 0.2.0
 2 files changed, 2 insertions(+), 2 deletions(-)
Creating git tag drop_struct_macro_derive-0.2.0
Starting drop_struct_macro_derive's next development iteration 0.2.1-alpha.0
Fatal: Invalid TOML file format: error during execution of `cargo metadata`: error: no matching package named `drop_struct_macro_derive` found
location searched: /Users/dignifiedquire/work/filecoin/rust-proofs/drop-struct-macro-derive
perhaps you meant: drop_struct_macro_derive
required by package `filecoin-proofs v0.1.0 (/Users/dignifiedquire/work/filecoin/rust-proofs/filecoin-proofs)`

Any tips on how to overcome this would be appreciated. The code is here: filecoin-project/rust-fil-proofs#680

@dignifiedquire
Copy link

okay, I found the issue, it seems that the following does not lead to a valid selection of dependencies in cargo iteslf

# a/Cargo.toml
name = "a"
version = "0.1.1-alpha.0"

# b/Cargo.toml
name = "b"
version = "0.2"

[dependencies]
a = { version = "^0.1", path = "../a" }

When switching the version of a to a non prerelease version things start working again.

@epage
Copy link
Collaborator Author

epage commented Jun 4, 2019

Weird that the error showed up as it did but that makes sense. I think you have to opt-in to pre-release versions. I don't use pre-release versions myself and so I didn't think to test them. If you think there is a bug with the auto-updating of [dependencies] with pre-release versions, please open a new issue.

@dignifiedquire
Copy link

I have a repro of the issue here: https://github.com/dignifiedquire/cargo-workspace-prerelease-bug

Which I think is a cargo bug, or at least a very surprising behaviour for me. Will switch off prerreleases for now and file an issue with cargo to get more clarity

@dignifiedquire
Copy link

created rust-lang/cargo#7007

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Improve the expected
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants