#8 rewrote nodejs.req version handling
Closed by tomh. Opened by dcallagh.
dcallagh/nodejs-packaging rewrite-req  into  master

Download 8.patch

This is an alternative to PR#7. I was having trouble tweaking the existing logic to make it pass all the test cases (specifically to fix ~1.2 without breaking other cases).

So I tried writing the version stuff from scratch, more closely following the semver grammar from the NPM docs.

The rewritten version looks longer and scarier but I personally think it is easier to follow, particularly when you read it in conjunction with: https://docs.npmjs.com/misc/semver

Up to you whether you prefer this version or not.

I think this looks good, but some of the results it produces are different to #7 in ways that I don't think are correct... Specifically most of the X-Range and tilde tests because you have things like this:

assert process_dep('npm(a)', '~1.2.x')  == 'npm(a) = 1.2'

but I think that needs to be:

assert process_dep('npm(a)', '~1.2.x')  == 'npm(a)  >= 1.2 with npm(a) < 1.3'

otherwise rpm won't allow minor versions of 1.2 to match?

At least I don't think it does prefix matching for equals does it?

Hmm yeah so that is specifically this little "special case" at the end:

+    # Special case: when the lower and upper bounds both have the same number
+    # of parts and are only 1 version apart from each other, we can represent
+    # this as a single = requirement and rely on RPM's own version matching
+    # logic to do the right thing.
+    # This avoids producing unnecessary 'with' requirements in many common cases.
+    # '>=1.2 <1.3' -> '= 1.2'
+    if (len(lower_bound) == len(upper_bound)
+            and lower_bound_inclusive
+            and not upper_bound_inclusive
+            and upper_bound == incremented(lower_bound)):
+        return ['{} = {}'.format(req, '.'.join(str(part) for part in lower_bound))]

But I went back to double-check that it actually works this way, and it doesn't! You're right. I must have imagined this behaviour in RPM.

It does have special rules for ignoring a trailing release portion. So a requirement like npm(a) = 1.2 will be satisfied by npm(a) = 1.2 and also by npm(a) = 1.2-1.git.asdf123123.fc28. But it won't be satisfied by npm(a) = 1.2.5.

So, this actually means that we're producing incorrect requirements for the =1 case right now (which translates to npm(a) = 1 which won't be satisfied).

I'll amend this PR to take out that special case so that it always produces a range.

Also, for future reference, when I was trying to check RPM's behaviour with these version comparisons I ended up at tests/rpmdepmatch.at in its source tree. That revealed this little nugget which I would otherwise never have found...

>>> import rpm
>>> d1 = rpm.ds(('a', '=', '1.2.5'), 'provides')
>>> d2 = rpm.ds(('a', '=', '1.2'), 'requires')
>>> d1.Compare(d2)
False
>>> d1 = rpm.ds(('a', '=', '1.2-1'), 'provides')
>>> d2 = rpm.ds(('a', '=', '1.2'), 'requires')
>>> d1.Compare(d2)
True

2 new commits added

  • properly fix nodejs.req version handling
  • unit tests for nodejs.req

Okay, commit e5f7762 is amended to take out that (incorrect) special case, it produces ranges instead.

There is one situation I was not sure about, which is the =1.2.3 case. Right now we are producing:

npm(a) = 1.2.3

but with this patch it becomes:

(npm(a) >= 1.2.3 with npm(a) < 1.2.4)

Now that I know RPM's = doesn't match trailing version components, this seems better. We could leave it as npm(a) = 1.2.3 under the assumption that a nodejs package's RPM version will always be major.minor.patch according to the semver rules. But that assumes that the packager will never need to put anything unusual into the RPM package version. I can't think of any reason that would happen, but it doesn't seem like a wise assumption to make.

So I think making it a version range in this case is the right choice.

Pull-Request has been closed by tomh

Metadata