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

Allow ranges in spec #1232

Closed
wants to merge 2 commits into from
Closed

Allow ranges in spec #1232

wants to merge 2 commits into from

Conversation

goretkin
Copy link
Contributor

Attempt at addressing #843


```toml
[compat]
PkgA = "0.1 - 0.2.3" # [0.1.0, 0.2.3)
Copy link
Member

Choose a reason for hiding this comment

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

This is not what a range should mean: the second end-point should be inclusive, and any unspecified trailing numbers should be considered to be wildcards. I.e.

  • 0.1-0.2.3 means [0.1, 0.2.3]
  • 0.1-0.2 means [0.1, 0.3)
  • 0.1-0 means [0.1, 1.0)

This seems to be what you've implemented, but it would be good to have some tests and, of course, the comment here should be fixed.

Copy link
Contributor Author

@goretkin goretkin Jun 18, 2019

Choose a reason for hiding this comment

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

Thanks for taking a look. I didn't notice the ) vs the ]. I just copied and pasted from the doc above.

Did you see the tests already in the commit? I am not really familiar with semver, so if you could recommend any additional tests, that would be helpful.

@goretkin
Copy link
Contributor Author

Hi @fredrikekre, would you mind clarifying whether you are thumbs-downing the feature, or the implementation?

@fredrikekre
Copy link
Member

Is it really so annoying to list all breaking versions you are compatible with?

@StefanKarpinski
Copy link
Member

@fredrikekre, why the thumbs down? If I have to guess it's that you don't want to support range syntax in compat sections. The current situation where we have non-overlapping syntaxes for version specs in the registry and in the compat section is a bit of a mess and this fixes at least half of that. We should have a single syntax for specifying version sets that works everywhere. We can encourage/discourage using certain parts of the syntax in different places, but having different syntaxes in different places seems awful to me.

@goretkin
Copy link
Contributor Author

@fredrikekre I don't know if it's annoying. So that I can get a better idea of what you mean, can you tell me how I would specify: 0.5.3-0.10 currently?
(Actual example, EnhancedGJK breaks with StaticArrays 0.11
https://github.com/JuliaRobotics/EnhancedGJK.jl/blob/f6a6553806a2efc56b0ac5bfd942708492877f06/Project.toml#L18)

@fredrikekre
Copy link
Member

can you tell me how I would specify: 0.5.3-0.10 currently?

[compat]
StaticArrays = "0.5.3, 0.6, 0.7, 0.8, 0.9, 0.10"

In this case it becomes a bit long, but it should be really unusual to support more than one, maybe two breaking versions of a dependency, so the list will normally don't be that long.

@goretkin
Copy link
Contributor Author

goretkin commented Jun 18, 2019

I suppose I don't really understand why there are two syntaxes, either. Why isn't there exactly one way to specify a set of versions? Is it because semver on its own can't specify arbitrary sets of versions?
(I'm looking at https://devhints.io/semver and I don't see any comma-separated anything.)

So then, if that's the case, then there's some other syntax required for specifying arbitrary sets of versions. Pkg looks like it uses "," as a union operator of sets to be, uhm, "semver-complete".
If that's the case, then I don't understand why we would impose a limitation on the atoms that you're taking the union of.

I think I'd understand if this feature required introducing new syntax. But it doesn't. It uses the same parsing code that's used elsewhere.

@StefanKarpinski
Copy link
Member

Why isn't there exactly one way to specify a set of versions?

The "semver syntax"—which isn't actually specified in the semver standard at all—has many different redundant syntaxes, like ~1.2 and ^1.2 and 1.2 so if we are going to support that at all, then we would inherently already have redundant syntaxes for specifying version sets. That's part of why I originally only implemented version range syntax: it seemed like the most intuitive, least redundant way to specify any set of versions (I think). On the other hand, the ~1.2, ^1.2, 1.2 set of syntaxes is less general: you cannot, without ranges, specify a range like 0.7-1.3 at all since there are an infinite number of 0.x versions in that range. It's arguable that this is a bad thing, of course, but the syntax is strictly more general.

However, people wanted the normal semver syntax that other languages use for writing compatibility constraints, so that was implemented—without supporting the range syntax, which is a less common syntax in other languages. So that's how we have ended up here: the registry format only supports version range syntax, while the compat section only supports tilde, carrot and equality specifications of version sets. We cannot break things, so that precludes removing support either syntax in order to unify things, which leaves supporting all of these syntaxes in all places as the only way to unify.

@fredrikekre
Copy link
Member

I suppose I don't really understand why there are two syntaxes, either.

There are just one user-facing syntax. The range syntax used in the Registry is an implementation detail.

@StefanKarpinski
Copy link
Member

It’s a very public “implementation detail”.

@goretkin
Copy link
Contributor Author

I thought I had tested my last commit, but it was broken. This should be correctly tested now.

That said, I think I'm still missing some concepts. I would be happy to add some documentation in a separate PR if I understood.

Pkg.Types.VersionSpec and Pkg.Types.VersionRange are both sets (conceptually) of Base.VersionNumbers. A VersionRange must be a contiguous interval of consecutive versions. And a VersionSpec is a ___ ?

And then there's semver_spec which seems to be a different kind of spec:

julia> Pkg.Types.semver_spec("1.2.3")
VersionSpec("1.2.3-1")

julia> Pkg.Types.VersionSpec("1.2.3")
VersionSpec("1.2.3")

@StefanKarpinski
Copy link
Member

I don't understand the new force_range thing. Can you explain what that's about?

@goretkin
Copy link
Contributor Author

I don't understand the new force_range thing. Can you explain what that's about?

VersionRange is happy to parse "v1.2.3" as lower=upper=Pkg.Types.VersionBound((0x00000001, 0x00000002, 0x00000003), 3) unless the force_range flag is set, in which case it only parses things that look like "v1.2.3-v4.5.6"

I don't know what Pkg.Types.semver_spec("v1.2.3") == VersionSpec("1.2.3-1") means, but that was the existing behavior. I'm not sure if the following transcript helps:

julia> Pkg.Types.semver_spec("v1.2.3") # maintain existing behavior of semver_spec
VersionSpec("1.2.3-1")

julia> Pkg.Types.VersionRange("v1.2.3")
VersionRange("1.2.3")

julia> Pkg.Types.VersionSpec(Pkg.Types.VersionRange("v1.2.3")) # this would be the wrong thing to return from semver_spec("v1.2.3")
VersionSpec("1.2.3")

julia> Pkg.Types.VersionRange("v1.2.3"; force_range=true)
ERROR: ArgumentError: degenerate version range: "v1.2.3"

@StefanKarpinski
Copy link
Member

Ah, ok. This is related to the fact that by default semver_spec interprets 1.2.3 as meaning ^1.2.3. That is truly unfortunate in retrospect because 1.2.3 by itself means something quite different in registry files. I always had a feeling we shouldn't have chosen ^ as a default even though it's usually what you want. I'm not sure how we can best converge these formats now. @KristofferC, any ideas? Can we deprecate 1.2.3 in compat sections for a while and then make = the default later?

@tkf
Copy link
Member

tkf commented Sep 16, 2019

This would be a great feature to have. What is blocking from it to be merged?

Can we deprecate 1.2.3 in compat sections for a while and then make = the default later?

Don't you need to still support it during Julia 1.x? If that's the case, isn't the decision to deprecate it or not orthogonal to this PR?

@KristofferC
Copy link
Member

KristofferC commented Sep 18, 2019

It’s a very public “implementation detail”.

In what way? A bot writes it, Pkg consumes it. It should never be touched by humans because getting the compression correct by hand is very hard. If people need to change bounds of existing versions in the registry we should have an API along the lines of change_compat(pkg, versionrange, dep, dep_compat) so you can say change_compat("DataFrames", (v"0.17", v"0.18"), "Tables", "0.15") which changes DataFrames v0.17.0, v0.17.1... to have compat with Tables.jl as one would have written "Tables = 0.15" in [compat].

Trying to unify the compressed format of the registry and the compat specification will in my opinion only lead to confusion.

Ah, ok. This is related to the fact that by default semver_spec interprets 1.2.3 as meaning ^1.2.3. That is truly unfortunate in retrospect because 1.2.3 by itself means something quite different in registry files

It was very much intentional that the simplest way to write a compa entry that is semver compatible with 1.2.3 should be to just write 1.2.3. The registry format (which is compressed and not supposed to be written by humans) is orthogonal to this.

Can we deprecate 1.2.3 in compat sections for a while and then make = the default later?

I oppose this to the extremest degree :)

@KristofferC
Copy link
Member

And then there's semver_spec which seems to be a different kind of spec:

semver_spec is what takes the string in the [compat] entry and transforms it to a VersionRange as documented in https://julialang.github.io/Pkg.jl/v1/compatibility/.

@KristofferC
Copy link
Member

KristofferC commented Sep 18, 2019

Regarding this PR, I don't think it should change the VersionRange constructor at all. Instead, another entry in

Pkg.jl/src/versions.jl

Lines 340 to 345 in dfb341a

const version = "v?([0-9]+?)(?:\\.([0-9]+?))?(?:\\.([0-9]+?))?"
const ver_regs =
[
Regex("^([~^]?)?$version\$") => semver_interval, # 0.5 ^0.4 ~0.3.2
Regex("^((?:≥)|(?:>=)|(?:=)|(?:<)|(?:=))v?$version\$") => inequality_interval,# < 0.2 >= 0.5,2
]

should be added that matches the hyphen syntax and creates the correct VersionRange.

Something like adding an entry

Regex("$version - $version\$" => hyphen_interval

and the function hyphen_interval.

@KristofferC
Copy link
Member

Obsoleted by #1410

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

Successfully merging this pull request may close these issues.

6 participants