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

Specifying dependencies: add exact version section #9961

Closed
wants to merge 8 commits into from

Conversation

marcospb19
Copy link

@marcospb19 marcospb19 commented Oct 5, 2021

Specifying exact versions is extremely useful, I gave it a dedicated section separating from the "Comparison requirements" one, even tho it can be mixed up with comparison requirements.

Specify an exact version together with other comparison requirements seems to be redundant:

time = "= 0.3.3, > 0.2.0"
rand = "= 0.8.3, >= 0.8.3, < 0.8.4"

It's redundant because it can only solve to "0.3.3" and "0.8.3".

Questions:

  1. Is it ok to separate these sections?
  2. Should I add a note in the Exact version requirement section saying that it can be chained with other Comparison requirements? (even if it's useless)

I would suggest separating it and adding a new warning for when this occurs, saying that it is redundant (I also would like to implement it).

In [dependencies] a "=" can be used to specify an exact version
@rust-highfive
Copy link

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @alexcrichton (or someone else) soon.

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 5, 2021
@alexcrichton
Copy link
Member

Thanks for the PR! I think it's fine to split this out to its own section, although strict version requirements like this are generally an anti-pattern on crates.io itself so we tend to advise against it, so could the section be placed at the end instead of the front? (if you're up for it calling out the hazards of publishing a crate with strict version requirements would also be great). Otherwise though I think it might be good to say that multiple requirements can be listed (I thought they needed to be separated with , but maybe not?), but I don't think there's any need to call out specifically that for =foo requirements specifying other things may not make much sense.

Rewrote sections about specifying dependencies versions in Cargo.toml
@marcospb19
Copy link
Author

I thought they needed to be separated with , but maybe not?

Oh yes, they do, sorry, I edited the original message

rand = "= 0.8.3, >= 0.8.3, < 0.8.4"

@marcospb19 marcospb19 closed this Oct 7, 2021
@marcospb19 marcospb19 deleted the patch-1 branch October 7, 2021 10:40
@marcospb19 marcospb19 restored the patch-1 branch October 7, 2021 10:40
@marcospb19 marcospb19 reopened this Oct 7, 2021
@marcospb19
Copy link
Author

marcospb19 commented Oct 7, 2021

Oops, I tried renaming the branch, that closed the PR unintentionally.

@marcospb19
Copy link
Author

@alexcrichton I tried to rewrite all the version requirements sections, they were untouched for more than 4 years.

I said that using strict version requirements would be redundant with other requirements, however, there is this counter-example:

time = "=0.3, < 0.3.5"
# which is equivalent to
time = ">=0.3, < 0.3.5"
# and
time = ">=0.3.0, < 0.3.5"

Comment on lines 163 to 165
> **Note**: Using strict requirements with exact versions are a bad practice and
> should be avoided, as it disables the ability of receiving important patches
> of security, soundness and bugfixes.
Copy link
Author

@marcospb19 marcospb19 Oct 7, 2021

Choose a reason for hiding this comment

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

Here is the advice you suggested, about the hazard of using strict requirements.

Copy link
Contributor

Choose a reason for hiding this comment

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

I would not word this so strongly. This goes against the advice given in https://doc.rust-lang.org/cargo/reference/resolver.html#recommendations where exact requirements do have some use cases (for example, serde has an exact semver requirement on serde_derive because the two are very tightly coupled and not intended to be used independently.

There are other use cases, such as a temporary workaround to lock to an exact version. I think we've done that with cargo itself before.

Copy link
Author

Choose a reason for hiding this comment

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

Changed it to:

> **Note**: When using strict requirements with exact versions, no updates can
> be applied, including bugfixes and security updates.

Any other recommendations?

Comment on lines 26 to 42
[semver]: https://github.com/steveklabnik/semver#requirements
[semver]: https://github.com/dtolnay/semver
Copy link
Author

Choose a reason for hiding this comment

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

Unresolved question: where was this pointing to? The previous repository now leads to this new one I inserted, but there is no section "#requirements".

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it used to summarize the rules for requirements, but that has been removed and instead points to this page (somewhat circular).

I notice this page doesn't link to or describe what SemVer is anywhere. Perhaps it could link to https://semver.org/ and/or https://doc.rust-lang.org/cargo/reference/resolver.html#semver-compatibility?

Copy link
Author

Choose a reason for hiding this comment

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

Changed it to https://doc.rust-lang.org/cargo/reference/resolver.html#semver-compatibility, because it already points to https://semver.org/ in the first line.

So it's like we're pointing to both.

Copy link
Contributor

@ehuss ehuss left a comment

Choose a reason for hiding this comment

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

Thanks for the PR!

Lots of comments, but only because I think this text can be really important to be as clear as possible.

Comment on lines 54 to 55
If we specify the version string as `"1.0.4"`, compatible versions are, `1.0.x`
where `x` is greater than 4.
Copy link
Contributor

Choose a reason for hiding this comment

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

I don' tthink this is correct. Specifying "1.0.4" means that it will pick any version 1.x.y greater than 1.0.4.

Copy link
Author

@marcospb19 marcospb19 Dec 17, 2021

Choose a reason for hiding this comment

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

I find this to be a bit hard to explain without being confusing and verbose.

I tried organizing this in another way, here's what I came up with:

If we specify the version string as `"1.4.8"`, compatible versions are:
- `"1.4.x"` with `x ≥ 8`.
- `"1.x.y"` with `x > 2`, `y` can be anything.

What do you think?

separated with a comma, e.g., `>= 1.2, < 1.5`.
As shown in the examples above, it is possible to combine any requirements to
restrict the version even further, however, this is only necessary with
[comparison requirements](#Comparison-requirements).
Copy link
Contributor

Choose a reason for hiding this comment

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

Section fragments should be in lower case.

Suggested change
[comparison requirements](#Comparison-requirements).
[comparison requirements](#comparison-requirements).

Also, I would maybe include what it means to combine requirements, both the syntax and the semantics. That is, saying that you can combine multiple requirements with commas, and that it means that all requirements must be met.

Copy link
Author

@marcospb19 marcospb19 Dec 17, 2021

Choose a reason for hiding this comment

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

Fixed the section upper case and added this text to the multiple requirements part:

The list of requirements should be separated by commas, spaces are optional.

A version is considered to be compatible with the list only if it is compatible
with all elements of the list.

Comment on lines 163 to 165
> **Note**: Using strict requirements with exact versions are a bad practice and
> should be avoided, as it disables the ability of receiving important patches
> of security, soundness and bugfixes.
Copy link
Contributor

Choose a reason for hiding this comment

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

I would not word this so strongly. This goes against the advice given in https://doc.rust-lang.org/cargo/reference/resolver.html#recommendations where exact requirements do have some use cases (for example, serde has an exact semver requirement on serde_derive because the two are very tightly coupled and not intended to be used independently.

There are other use cases, such as a temporary workaround to lock to an exact version. I think we've done that with cargo itself before.

Comment on lines 10 to 11
You can specify different dependencies for:
- Development and release profiles.
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure what is meant is meant here, but it seems to imply you can independently define dependencies for different profiles (which can't be done). Can you say why this is mentioning profiles here?

Copy link
Author

@marcospb19 marcospb19 Dec 17, 2021

Choose a reason for hiding this comment

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

I was mistaken in this one, glad you got my mistake in the review.

Rewritten to:

Dependencies can be specified for [crate features](features.md) and for
different [architectures and operating systems](#platform-specific-dependencies).

Comment on lines 26 to 42
[semver]: https://github.com/steveklabnik/semver#requirements
[semver]: https://github.com/dtolnay/semver
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it used to summarize the rules for requirements, but that has been removed and instead points to this page (somewhat circular).

I notice this page doesn't link to or describe what SemVer is anywhere. Perhaps it could link to https://semver.org/ and/or https://doc.rust-lang.org/cargo/reference/resolver.html#semver-compatibility?

Comment on lines 46 to 48
**Caret requirements** allow SemVer compatible updates, it's the only
requirement type that uses specific rules for versions with and without the zero
digits.
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm a bit confused by this statement about the zero digits.

Usually I think it is best to have a definition up front when introducing a term, and I think it would be good to lead with the sentences that define its left-most non-zero behavior. I think the wording here is very important since this is introducing a potentially confusing topic, and this is one of the most important parts of cargo. If possible, it would be best to try to approach it assuming you don't know what semver is, or what a version requirement is and what it means. The original text tried to do that by introducing a topic by example from the perspective of what someone does with these dependencies. I'm not saying it has to use that style, just mentioning that is one way to introduce a new concept.

Copy link
Author

Choose a reason for hiding this comment

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

Ok, I rewrote this section and now the definition comes first, needs another review.

src/doc/src/reference/specifying-dependencies.md Outdated Show resolved Hide resolved

### Strict version requirements

**Strict version requirements** allow you to lock the numbers for major, minor
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm a little uneasy introducing this as a separately defined kind of requirement. For example =1 is not what I would characterize as "strict". Is there a particular reason for adding it? It seems like it fits with the comparison section, as = is a comparison operator.

Copy link
Author

Choose a reason for hiding this comment

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

My reasoning was that strict requirements only unique purpose is specifying exact versions without having to create a list of multiple requirements.

For example, =1.2.3 is a shorthand for >= 1.2.3, < 1.2.4.

And the other comparison operators all leave open ranges to be combined with more operators, = on the other hand, works very similarly to *, specifying a part of the version, and having the missing parts be "anything".

@ehuss ehuss added S-waiting-on-author Status: The marked PR is awaiting some action (such as code changes) from the PR author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 25, 2021
@bors
Copy link
Collaborator

bors commented Dec 5, 2021

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

@ehuss
Copy link
Contributor

ehuss commented Dec 14, 2021

Ping @marcospb19 Just checking in to see if you are still interested in working on this, or if you had any questions.

@marcospb19
Copy link
Author

I'm still interested!

Sorry for the delay, I've been struggling finding time between multiple projects.

Now seems like a better time for trying finishing it, I'll see if I can take a look.

Thanks for the ping :D .

@marcospb19
Copy link
Author

Working on the conflict now.

@marcospb19
Copy link
Author

About the conflicts, some smaller work were done in the second section of this document in order to improve some of the explanations, I reused the new texts added there, but kept the new section organization that was made in this PR, containing the table of all operators and keeping the Caret Requirement explanation inside of it's dedicated section.

src/doc/src/reference/specifying-dependencies.md Outdated Show resolved Hide resolved

> **Note**: This is the only type of version requirement that has different
> rules for versions that start with `0`, and the version `"0.x.y"` is not
> compatible with any other versions.
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure I understand this last clause about 0.x.y not being compatible with anything else, isn't 0.x.(y+1) compatible with that?

Copy link
Author

Choose a reason for hiding this comment

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

Yes, if x > 0.

Before the review this was stating that 0.0.x was not compatible with other versions, @ehuss said that 0.0.x was not a common version, so it wouldn't be as useful in this explanation, so I changed to 0.x.y without noticing that it is now incorrect.

I'm not sure how to proceed here, is this information even useful? I do often get confused about this, maybe I should just leave the definition.

Or remove completely the "and" part?

, and the version "0.x.y" is not compatible with any other versions.

Copy link
Member

Choose a reason for hiding this comment

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

What is this note trying to convey? It's correct that 0.0.x is not that common but it's incorrect to say that 0.x.y is not compatible with any other versions.

Comment on lines +72 to +79
^1 := >=1.0.0, <2.0.0
^1.2 := >=1.2.0, <2.0.0
^1.2.3 := >=1.2.3, <2.0.0
^0 := >=0.0.0, <1.0.0
^0.0 := >=0.0.0, <0.1.0
^0.0.3 := >=0.0.3, <0.0.4 (exact)
^0.2 := >=0.2.0, <0.3.0
^0.2.3 := >=0.2.3, <0.3.0
Copy link
Member

Choose a reason for hiding this comment

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

IIUC #10158 tried not to mention much about the ^ syntax on requirement to reduce confusion. Do we want to follow the rule here and throughout this PR?

Copy link
Author

Choose a reason for hiding this comment

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

How could I proceed here?

  1. Rename caret requirement to SemVer requirement.
  2. Inside of the caret requirements section, remove all carets, and add a note explaining that "1.4.8" == "^1.4.8".
  3. Try to move all caret requirements out of it's own section, and put it on the start of the file.

I tried approaching 3 like #10158 but I found it to be weird with the new table with all the operators.

Copy link
Member

Choose a reason for hiding this comment

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

Hmm... maybe keep calling it caret but only mention it as a alternative syntax at the end of the "Caret requirements" section? Such like

Note that Cargo favors specifying caret requirements without a real caret ^. You can still use the alternative syntax. For example, ^1.2.3 is exactly equivalent to 1.2.3.

Though it still seems a bit awkward...

Copy link
Member

Choose a reason for hiding this comment

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

I like @weihanglo's idea here to not use ^ in this section where possible, and at the end there can be a small code-block that says that ^1.2.3 := 1.2.3 or similar

@alexcrichton alexcrichton removed their assignment Mar 8, 2022
@ehuss
Copy link
Contributor

ehuss commented Mar 22, 2022

I'm going to close due to inactivity. If you or anyone is interested in continuing this, feel free to open a new PR with the above comments addressed.

@ehuss ehuss closed this Mar 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: The marked PR is awaiting some action (such as code changes) from the PR author.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants