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

No warning/error/abort if version does not exist/is yanked #113

Closed
somehowchris opened this issue Apr 26, 2022 · 9 comments · Fixed by #116 or #151
Closed

No warning/error/abort if version does not exist/is yanked #113

somehowchris opened this issue Apr 26, 2022 · 9 comments · Fixed by #116 or #151
Labels
Report: bug Something isn't working Report: feature request New feature request

Comments

@somehowchris
Copy link
Contributor

somehowchris commented Apr 26, 2022

I just started out trying to create scripts to download a specific version of a package. During testing, I found out that the following command will succeed and install another version than specified.

cargo binstall cargo-watch --no-confirm --version 8.1.0
11:41:07 [INFO] Installing package: 'cargo-watch'
11:41:11 [INFO] Checking for package at: 'https://github.com/watchexec/cargo-watch/releases/download/v8.1.1/cargo-watch-v8.1.1-x86_64-apple-darwin.tar.xz'
11:41:12 [INFO] The package will be downloaded from github.com
11:41:12 [INFO] Downloading package from: 'https://github.com/watchexec/cargo-watch/releases/download/v8.1.1/cargo-watch-v8.1.1-x86_64-apple-darwin.tar.xz'
11:41:13 [INFO] This will install the following binaries:
11:41:13 [INFO]   - cargo-watch (cargo-watch -> /Users/chris/.cargo/bin/cargo-watch-v8.1.1)
11:41:13 [INFO] And create (or update) the following symlinks:
11:41:13 [INFO]   - cargo-watch (/Users/chris/.cargo/bin/cargo-watch-v8.1.1 -> /Users/chris/.cargo/bin/cargo-watch)
11:41:13 [INFO] Installing binaries...
11:41:13 [INFO] Installation complete!

I understand that it's looking for ^8.1.0 but my 2 cents:

  • Either it should warn the user and offer other versions as options similar to npx or yarn add or in case of --no-confirm abort
  • Or clarify how its searching and also add that to the help page and keep error messages such as Error: No matching version for requirement: '=8.0.1' in line with others

I know I'm a bit picky on versioning and there is a reason semver versioning exists but there is also a reason why they gave it multiple digits to use. If I'm searching for some milk in a supermarket, I do not expect to leave with a yogurt just cause they are similar

Happy to hear opinions and RTFMs

@passcod
Copy link
Member

passcod commented Apr 26, 2022

That definitely is a missing check!

@passcod passcod added Report: bug Something isn't working Report: feature request New feature request labels Apr 26, 2022
passcod added a commit to passcod/cargo-binstall that referenced this issue Apr 27, 2022
@somehowchris
Copy link
Contributor Author

Well, left a comment a bit too late as you've already merged the branch

@passcod
Copy link
Member

passcod commented Apr 29, 2022

What comment? I merged with your suggestion of using semver, if you were referring to that

@somehowchris
Copy link
Contributor Author

Can't seem to create a link to the comment, but it was about the return of Ok(()); after printing the warn message. https://github.com/ryankurte/cargo-binstall/blob/main/src/main.rs#L117

My question was if that means that the exit code of cargo-binstall would be 0? If so this would mean scripts or installs such as Dockerfiles would ignore the error as they do not see it as a failure

@passcod
Copy link
Member

passcod commented Apr 29, 2022

That's only if --no-confirm is not given, and it should be in scripts.

@passcod
Copy link
Member

passcod commented Apr 29, 2022

I guess the second part of this issue hasn't been fixed: if the crate is yanked, it won't warn or do anything else but will install. That is:

  • cargo binstall cargo-watch --version =8.1.0 happily works and installs 8.1.0,
  • cargo binstall cargo-watch --version 8.1.0 --no-confirm warns, but still installs version 8.1.1,
  • cargo binstall cargo-watch --version 8.1.2 errors, as that crate is not published at that version. (technically the release exists in github, but the entirety of cargo-watch 8.x.y is a mistake)

@somehowchris
Copy link
Contributor Author

FYI about my thing above

gitpod /workspace/cargo-binstall (feature/remove-git-index) $ cargo binstall cargo-watch --version 8.1.0 --dry-run
12:26:51 [INFO] Installing package: 'cargo-watch'
12:26:52 [WARN] You specified `--version 8.1.0` but the package resolved that to '8.1.1', use `=8.1.0` if you want an exact match
12:26:52 [WARN] Installation cancelled
gitpod /workspace/cargo-binstall (feature/remove-git-index) $ echo $?
0

@somehowchris
Copy link
Contributor Author

cargo binstall cargo-watch --version 8.1.2 errors, as that crate is not published at that version. (technically the release exists in github, but the entirety of cargo-watch 8.x.y is a mistake)

I dont know about this one but the others should be fixed in the meantime

@passcod
Copy link
Member

passcod commented May 2, 2022

I guess I should have been clearer but all those examples I gave were behaviour I expected as correct and were not things to be fixed in my mind. That is, the remaining issue was:

if the crate is yanked, it won't warn or do anything else but will install.

So cargo binstall cargo-watch --version =8.1.1 should tell you to go away specifically because the crate is yanked, which you've fixed in #122.

See #125 for more discussion on the rest of it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Report: bug Something isn't working Report: feature request New feature request
Projects
None yet
2 participants