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.

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

Add a new --range argument on cargo install to accept semver ranges #4207

Closed
azerupi opened this issue Jun 22, 2017 · 13 comments
Closed

Add a new --range argument on cargo install to accept semver ranges #4207

azerupi opened this issue Jun 22, 2017 · 13 comments

Comments

@azerupi
Copy link
Contributor

azerupi commented Jun 22, 2017

Discussion started in #3321

By mistake, the cargo install --vers takes a semver range instead of a precise version. This behaviour is now deprecated and issues a warning, however there are legitimate use cases for installing a binary using a semver range.

My personal use case is for mdBook. A lot of projects have set up automatic deployment of their rendered books through CI. The problem is that if they install the latest version, their deployment might break when we release a "breaking version". So they have to pin the installed version in their CI script, requiring manual intervention every time we release a patch version. The ideal solution would be to allow a binary specifying a semver range so that the CI script can install the latest compatible version, only requiring manual update when we release a breaking version.

@alexcrichton proposes to add the --range argument on cargo install to specify a semver range and keeping the --vers argument to specify a strict version.

If this is a desired feature, I would be willing to try and make a PR for this. 😊
@azerupi azerupi mentioned this issue Jun 22, 2017
Closed
@matklad
Copy link
Member

matklad commented Jun 23, 2017

Hm, could we use a singe --vers flag, but allow for ~1.2.3 constraints? That is, interpret 0.1.2 as a precise version, but continue to parse all other VersionReq syntaxes?

@alexcrichton
Copy link
Member

@matklad that's a possibility, yeah, but it runs the risk of being backwards from Cargo.toml and possibly confusing :(

@matklad
Copy link
Member

matklad commented Jun 23, 2017 •
edited
Loading

I'd say that --vers "^1.2.3" is more obvious then --range "1.2.3", and I don't think this'll cause a lot of confusion. For the --vers "1.2.3" flavor, it works exactly as documented, and is the same under both proposals, so the amount of the confusion is the same. And the --vers "~1.2.3" flavor is self documenting.

As you may have noticed, I usually want to avoid adding separate command line flags, if possible. Way too often I need | grep or | less to read --help of various unix utilities :(

@alexcrichton
Copy link
Member

Yeah that makes sense to me. ok, I'm convinced!

@matklad
Copy link
Member

matklad commented Jun 23, 2017

Ok, the only problem is that I am not 100% sure that there isn't some nasty syntactic ambiguity between version and range. Let's ask @steveklabnik about this!

@steveklabnik, we want to interpret stuff like 1.2.3 as a precise sevantic version, and stuff like ^1.2.3 as a SemverRange. Will the following algorithm work without surprises?

  1. If input is empty, return error.
  2. look at the first character, if it is among <, >, =, ^, ~, parse input as range (this may return error)
  3. if the first character is not <, >, =, ^, ~, parse input as version.

@alexcrichton
Copy link
Member

What about:

  • Parse input as a semver version
    • If successful, match exactly that
    • If failure, parse as a semver range
      • If successful, match that
      • If failure print out that error message

@matklad
Copy link
Member

matklad commented Jun 23, 2017

@alexcrichton I think this variant can lead to surprises: --vers "0.1" will be parsed as range, which is not super obvious if you don't know that in rust versions have three components.

@alexcrichton
Copy link
Member

Ah yeah that's a very good point! In that case the steps you proposed above sound pretty good to me

@matklad
Copy link
Member

matklad commented Jun 23, 2017

Yeah, I think we in general need to be wary of patterns like "try A, if that fails, fallback to B", because then fixing a bug in A is a backwards incompatible change (this is exactly the problem with the algorithm for infering binary paths)!

@azerupi
Copy link
Contributor Author

azerupi commented Jun 24, 2017

I think this variant can lead to surprises: --vers "0.1" will be parsed as range

So if I understand correctly, this should result in a warning (or error) because it would be parsed as a precise version, right?

@matklad
Copy link
Member

matklad commented Jun 24, 2017

Yeah, this should say something like "Invalid semantic version string 0.1". If we want to get fancy with error messages, we can then try to parse this as range, and if that succeeds add "if you want to specify semver range, add qualifier explicitelly, like ^0.1?"

@azerupi
Copy link
Contributor Author

azerupi commented Jun 24, 2017

Ok, I think I understand everything that needs to happen now. I will try to implement this and issue a PR when I'm done. :)

@azerupi azerupi mentioned this issue Jun 26, 2017
Merged
bors added a commit that referenced this issue Jun 27, 2017
@bors
Auto merge of #4229 - azerupi:install-version-range, r=matklad
cef6833 
Implement semver ranges for install --vers

This implements the design discussed in #4207
It allows to specify semver ranges on `cargo install ... --vers`

1. The first character of the `--vers` value is checked, if there is none we return an error.
2. If it is one of `<`, `>`, `=`, `^`, `~` we parse the value as a `VersionReq`, otherwise we parse it as a `Version`.
3. If the parsing as a `Version` fails but parsing as `VersionReq` succeeds, we add a note to the warning mentioning that a qualifier should be used to specify a semver range. This catches versions with less than tree digits.

Otherwise, the previous behaviour is preserved with the warning of backwards compatibility.

This means that

- `cargo install ... --vers "^1.2.3"` will be parsed as a range
- `cargo install ... --vers 1.2.3` will be parsed as a version
- `cargo install ... --vers 1.2` will be parsed as a version for backwards compatibility reasons, fail and be passed through as is,**but** we will add a note `if you want to specify semver range, add an explicit qualifier, like ^1.2`
- `cargo install ... --vers blah` will be parsed as a version for backwards compatibility reasons (which is weird because it is not even a valid semver range) and produce an `unknown error` down the line. I have left this behaviour untouched because it worked like that before, but I can easily make it error sooner with a better message.
bors added a commit that referenced this issue Jun 28, 2017
@bors
Auto merge of #4229 - azerupi:install-version-range, r=matklad
64c3217 
Implement semver ranges for install --vers

This implements the design discussed in #4207
It allows to specify semver ranges on `cargo install ... --vers`

1. The first character of the `--vers` value is checked, if there is none we return an error.
2. If it is one of `<`, `>`, `=`, `^`, `~` we parse the value as a `VersionReq`, otherwise we parse it as a `Version`.
3. If the parsing as a `Version` fails but parsing as `VersionReq` succeeds, we add a note to the warning mentioning that a qualifier should be used to specify a semver range. This catches versions with less than tree digits.

Otherwise, the previous behaviour is preserved with the warning of backwards compatibility.

This means that

- `cargo install ... --vers "^1.2.3"` will be parsed as a range
- `cargo install ... --vers 1.2.3` will be parsed as a version
- `cargo install ... --vers 1.2` will be parsed as a version for backwards compatibility reasons, fail and be passed through as is,**but** we will add a note `if you want to specify semver range, add an explicit qualifier, like ^1.2`
- `cargo install ... --vers blah` will be parsed as a version for backwards compatibility reasons (which is weird because it is not even a valid semver range) and produce an `unknown error` down the line. I have left this behaviour untouched because it worked like that before, but I can easily make it error sooner with a better message.
@azerupi
Copy link
Contributor Author

azerupi commented Jul 3, 2017

The PR has been merged, so I'm going to close this issue :)

@azerupi azerupi closed this as completed Jul 3, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@alexcrichton @matklad @azerupi