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

Ranges for release candidates fail to parse #24

Closed
bripkens opened this issue Nov 1, 2015 · 13 comments
Closed

Ranges for release candidates fail to parse #24

bripkens opened this issue Nov 1, 2015 · 13 comments

Comments

@bripkens
Copy link

bripkens commented Nov 1, 2015

Test

ExpressionParser.newInstance().parse("<=2.0.0-rc7")

Exception

Caused by: Illegal character near 'rc7'
    at com.github.zafarkhaja.semver.expr.Lexer.tokenize(Lexer.java:218)
    at com.github.zafarkhaja.semver.expr.ExpressionParser.parse(ExpressionParser.java:86)
    at com.github.zafarkhaja.semver.expr.ExpressionParser.parse(ExpressionParser.java:43)

Expected

Release candidates can be compared. For instance 2.0.0-rc8 does not satisfy the expression <=2.0.0-rc7.

@zafarkhaja
Copy link
Owner

According to the grammar the pre-release versions and build metadata are not supported. Yet.

Let's keep this issue as a feature request as I was planning to implement it. Unfortunately, though, it's not easy with the current parser implementation because there's a grammar ambiguity we need to work around; there's a conflict between Hyphen ranges and pre-release version separator.

@zafarkhaja
Copy link
Owner

By the way, in the meantime you can use the internal DSL for this if it suits you.

Version v = Version.valueOf("2.0.0-rc8");
boolean result = v.satisfies(lte("2.0.0-rc7"))); 

@bripkens
Copy link
Author

bripkens commented Nov 2, 2015

Thanks for getting back to me @zafarkhaja. Unfortunately I am not creating the expression, but I am only consuming them.

@zafarkhaja
Copy link
Owner

I just checked the node-semver ranges and it looks like they don't support pre-release versions either...

@bripkens
Copy link
Author

bripkens commented Nov 3, 2015

pre-release versions seem to be supported, but only match other pre-release

> require('semver').satisfies('2.0.0', '<=2.0.0-rc7')
false
> require('semver').satisfies('2.0.0-rc8', '<=2.0.0-rc7')
false
> require('semver').satisfies('2.0.0-rc1', '<=2.0.0-rc7')
true
> require('semver').satisfies('2.0.0-rc2', '<=2.0.0-rc7')
true

@zafarkhaja
Copy link
Owner

Well, JS was never my strong suit and the user documentation is not clear about that.

Yeah, it seems to support pre-release versions pretty well. I'm not sure what you mean by "but only match other pre-release", but all the examples in your comment look quite correct to me.

@eric-isakson
Copy link

node-semver doesn't have the - ambiguity as they require space before and after the - operator. Their BNF is here: https://github.com/npm/node-semver/blob/master/range.bnf

hyphen ::= partial ' - ' partial

I just ran into this today and also need support for range expresssions that can be applied to prerelease versions. I don't really want it implemented they way node-semver chose to. To be more precise about "but only match other pre-release", node-semver defines it this way:

https://github.com/npm/node-semver#prerelease-tags

Essentially, for node-semver, when you express a range part with a prerelease value, you are saying the major.minor.patch of any candidate version with a prelease must match (at least according to the doc). So given candidates:

1.2.3-alpha.0
1.2.3-beta.0
1.2.3-rc.1
1.2.3-rc.2
1.2.3
1.2.4-alpha.0
1.2.4
1.3.0-alpha.0
1.3.0
2.0.0

An expression of '>=1.2.3-rc.1' would be satisfied by:

1.2.3-rc.1
1.2.3-rc.2
1.2.3
1.2.4
1.3.0
2.0.0

An expression of '~1.2.3-rc.1' would be satisfied by:

1.2.3-rc.1
1.2.3-rc.2
1.2.3
1.2.4

An expression of '^1.2.3-rc.1' would be satisfied by:

1.2.3-rc.1
1.2.3-rc.2
1.2.3
1.2.4
1.3.0

Notice that in each case, the only prerelease versions that match have the exact major, minor, patch as the version used in the range expression. I think node-semver prerelease handling is implemented somewhat ambiguously though and contradicts their doc. Check this out:

$ node -e "console.log(require('semver').satisfies('1.4.5', '>=1.0.5-0 <=2.0.0-0'))"
true
// makes sense
$ node -e "console.log(require('semver').satisfies('1.4.5-rc.1', '>=1.0.5-0 <=2.0.0-0'))"
false
// is about dependency management, not what the spec defines as > and < according to precedence
$ node -e "console.log(require('semver').satisfies('1.0.5-rc.1', '>=1.0.5-0 <=2.0.0-0'))"
true
// makes sense but doesn't follow the rules I expect for an "and" expression between these two given the rule that I shouldn't match for a prerelease that is not in my x.y.z in this case 2.0.0
$ node -e "console.log(require('semver').satisfies('1.0.5-rc.1', '<=2.0.0-0'))"
false
// ?? oh, it doesn't start with 2.0.0...so why did the previous one work?
$ node -e "console.log(require('semver').satisfies('1.0.5', '<=2.0.0-0'))"
true
// yep
$ node -e "console.log(require('semver').satisfies('2.0.0-0', '>=1.0.5-0 <=2.0.0-0'))"
true
// um? isn't this an and expression?
$ node -e "console.log(require('semver').satisfies('2.0.0-0', '>=1.0.5-0'))"
false
// ugh this one follow the node-semver doc but has nothing to do with what I expect of >= w.r.t. the semver spec

for an expression like '>=1.2.3-rc.1 <=1.3.0-alpha.0' which should never match a prerelease since either side of the expression should filter out the other side's prereleases according to the doc but apparently it isn't implemented quite that way.

Personally, I think the node-semver implementation is really about dependency resolution and making some reasonable assertion about what people want to do when they fetch a dependency. This is great and has practical application in its domain.

For a pure semver library though, I would rather see an implementation that supports prerelease ranges that is purely about the sort precedence of the versions and leave the dependency resolution details out of the range expression implementation. Something like:

An expression of '>=1.2.3-rc.1' would be satisfied by:

1.2.3-rc.1
1.2.3-rc.2
1.2.3
1.2.4-alpha.0
1.2.4
1.3.0-alpha.0
1.3.0
2.0.0

An expression of '~1.2.3-rc.1' would be equivalent to '>=1.2.3-rc.1 & <1.3.0-0' (prerelease 0 is the lowest possible prerelease) and satisfied by:

1.2.3-rc.1
1.2.3-rc.2
1.2.3
1.2.4-alpha.0
1.2.4

An expression of '^1.2.3-rc.1' would be equivalent to '>=1.2.3-rc.1 & <2.0.0-0' and satisfied by:

1.2.3-rc.1
1.2.3-rc.2
1.2.3
1.2.4-alpha.0
1.2.4
1.3.0-alpha.0
1.3.0

I can then present those to my dependency management tooling and make a decision on whether or not to accept a given prerelease version without clouding the semver range expression syntax with decisions that are driven by a requirement that may not hold in all contexts.

If you want to do filtering the way node-semver does, you would probably need access to the range expression AST to make the right decisions.

@zafarkhaja
Copy link
Owner

Eric, thank you for following up. Give me some time to digest this and I'll get back to you soon.

@jrmash
Copy link

jrmash commented Mar 25, 2016

I have a use-case for this as well, so I'm curious what your thoughts are on it? Also, I've noticed that the 'satisfies' functionality doesn't seem to work on versions with a tilde - Is that a related issue?

Thanks for the insights!

@zafarkhaja
Copy link
Owner

@eric-isakson @jrmash thank you guys for your feedback and suggestions. I must say that I totally agree with what Eric has proposed in his last comment. I'll be shipping this feature with the next release (0.10.0).

@jrmash I don't think the tilde ranges are somehow related to this issue unless you're using pre-release versions, other than that the tilde ranges should work as expected. If you believe otherwise, please create a separate issue and provide an example of what's not working for you and I'll take a look. Thank you.

@zafarkhaja zafarkhaja added this to the 0.10.0 milestone Apr 27, 2016
@zafarkhaja zafarkhaja self-assigned this Apr 27, 2016
@jrmash
Copy link

jrmash commented Apr 27, 2016

@zafarkhaja I should have been clearer with my inquiry, but yes I am/was using tilde ranges with pre-release versions and the 'satisfies' feature.

@maxleiko
Copy link

maxleiko commented Apr 26, 2017

@zafarkhaja could you tell me the status on this?

Any help required?

@zafarkhaja
Copy link
Owner

Closing this issue in favor of #70, the progress can be followed from there.

@zafarkhaja zafarkhaja closed this as not planned Won't fix, can't repro, duplicate, stale Jan 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants