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

crashing bug in prerelease version parser #3147

Closed
samhatoum opened this Issue Nov 21, 2014 · 23 comments

Comments

Projects
None yet
6 participants
@samhatoum

samhatoum commented Nov 21, 2014

Yesterday I published a velocity:core with version set to 1.0.0-rc.6-cucumber. Since then, all packages that depend on previous versions of velocity:core are no longer working, the error displayed is shown below.

To replicate try to add mike:mocha to any meteor project. Here's a copy/paste repro:

meteor create semvarIssue
cd semvarIssue
meteor add mike:mocha

You will see the following:

Error: Unexpected character in prerelease identifier: 6
    at validCharToNumber (packages/package-version-parser/package-version-parser.js:102)
    at packages/package-version-parser/package-version-parser.js:108
    at Array.reduce (native)
    at Function._.reduce._.foldl._.inject (packages/underscore/underscore.js:139)
    at prereleaseIdentifierToFraction (packages/package-version-parser/package-version-parser.js:92)
    at Object.PV.versionMagnitude (packages/package-version-parser/package-version-parser.js:82)
    at packages/constraint-solver/constraint-solver.js:424
    at Object.each (/Users/sam/.meteor/packages/meteor-tool/.1.0.35.gxa3d3++os.osx.x86_64+web.browser+web.cordova/meteor-tool-os.osx.x86_64/isopacks/constraint-solver/npm/node_modules/mori/mori.js:320:439)
    at Object.resolverOptions.costFunction (packages/constraint-solver/constraint-solver.js:388)
    at overallCostFunction (packages/constraint-solver/resolver.js:200)
    at packages/constraint-solver/resolver.js:232
    at Array.forEach (native)
    at Function._.each._.forEach (packages/underscore/underscore.js:105)
    at ConstraintSolver.Resolver.resolve (packages/constraint-solver/resolver.js:228)
    at ConstraintSolver.PackagesResolver.resolve (packages/constraint-solver/constraint-solver.js:238)
    at /Users/sam/.meteor/packages/meteor-tool/.1.0.35.gxa3d3++os.osx.x86_64+web.browser+web.cordova/meteor-tool-os.osx.x86_64/tools/catalog.js:327:32
    at /Users/sam/.meteor/packages/meteor-tool/.1.0.35.gxa3d3++os.osx.x86_64+web.browser+web.cordova/meteor-tool-os.osx.x86_64/tools/buildmessage.js:338:18
    at _.extend.withValue (/Users/sam/.meteor/packages/meteor-tool/.1.0.35.gxa3d3++os.osx.x86_64+web.browser+web.cordova/meteor-tool-os.osx.x86_64/tools/fiber-helpers.js:112:14)
    at /Users/sam/.meteor/packages/meteor-tool/.1.0.35.gxa3d3++os.osx.x86_64+web.browser+web.cordova/meteor-tool-os.osx.x86_64/tools/buildmessage.js:331:34
    at _.extend.withValue (/Users/sam/.meteor/packages/meteor-tool/.1.0.35.gxa3d3++os.osx.x86_64+web.browser+web.cordova/meteor-tool-os.osx.x86_64/tools/fiber-helpers.js:112:14)
    at /Users/sam/.meteor/packages/meteor-tool/.1.0.35.gxa3d3++os.osx.x86_64+web.browser+web.cordova/meteor-tool-os.osx.x86_64/tools/buildmessage.js:329:23
    at _.extend.withValue (/Users/sam/.meteor/packages/meteor-tool/.1.0.35.gxa3d3++os.osx.x86_64+web.browser+web.cordova/meteor-tool-os.osx.x86_64/tools/fiber-helpers.js:112:14)
    at Object.enterJob (/Users/sam/.meteor/packages/meteor-tool/.1.0.35.gxa3d3++os.osx.x86_64+web.browser+web.cordova/meteor-tool-os.osx.x86_64/tools/buildmessage.js:303:26)
    at _.extend.resolveConstraints (/Users/sam/.meteor/packages/meteor-tool/.1.0.35.gxa3d3++os.osx.x86_64+web.browser+web.cordova/meteor-tool-os.osx.x86_64/tools/catalog.js:320:30)
    at /Users/sam/.meteor/packages/meteor-tool/.1.0.35.gxa3d3++os.osx.x86_64+web.browser+web.cordova/meteor-tool-os.osx.x86_64/tools/commands-packages.js:2219:38
    at /Users/sam/.meteor/packages/meteor-tool/.1.0.35.gxa3d3++os.osx.x86_64+web.browser+web.cordova/meteor-tool-os.osx.x86_64/tools/buildmessage.js:250:13
    at _.extend.withValue (/Users/sam/.meteor/packages/meteor-tool/.1.0.35.gxa3d3++os.osx.x86_64+web.browser+web.cordova/meteor-tool-os.osx.x86_64/tools/fiber-helpers.js:112:14)
    at /Users/sam/.meteor/packages/meteor-tool/.1.0.35.gxa3d3++os.osx.x86_64+web.browser+web.cordova/meteor-tool-os.osx.x86_64/tools/buildmessage.js:243:29
    at _.extend.withValue (/Users/sam/.meteor/packages/meteor-tool/.1.0.35.gxa3d3++os.osx.x86_64+web.browser+web.cordova/meteor-tool-os.osx.x86_64/tools/fiber-helpers.js:112:14)
    at /Users/sam/.meteor/packages/meteor-tool/.1.0.35.gxa3d3++os.osx.x86_64+web.browser+web.cordova/meteor-tool-os.osx.x86_64/tools/buildmessage.js:241:18
    at _.extend.withValue (/Users/sam/.meteor/packages/meteor-tool/.1.0.35.gxa3d3++os.osx.x86_64+web.browser+web.cordova/meteor-tool-os.osx.x86_64/tools/fiber-helpers.js:112:14)
    at /Users/sam/.meteor/packages/meteor-tool/.1.0.35.gxa3d3++os.osx.x86_64+web.browser+web.cordova/meteor-tool-os.osx.x86_64/tools/buildmessage.js:232:23
    at _.extend.withValue (/Users/sam/.meteor/packages/meteor-tool/.1.0.35.gxa3d3++os.osx.x86_64+web.browser+web.cordova/meteor-tool-os.osx.x86_64/tools/fiber-helpers.js:112:14)
    at Object.capture (/Users/sam/.meteor/packages/meteor-tool/.1.0.35.gxa3d3++os.osx.x86_64+web.browser+web.cordova/meteor-tool-os.osx.x86_64/tools/buildmessage.js:231:19)
    at main.registerCommand.name [as func] (/Users/sam/.meteor/packages/meteor-tool/.1.0.35.gxa3d3++os.osx.x86_64+web.browser+web.cordova/meteor-tool-os.osx.x86_64/tools/commands-packages.js:2212:33)
    at /Users/sam/.meteor/packages/meteor-tool/.1.0.35.gxa3d3++os.osx.x86_64+web.browser+web.cordova/meteor-tool-os.osx.x86_64/tools/main.js:1353:23
@Sanjo

This comment has been minimized.

Contributor

Sanjo commented Nov 21, 2014

Maybe the - character before cucumber is illegal in this part.

@samhatoum

This comment has been minimized.

samhatoum commented Nov 21, 2014

Yep, and it looks like there might have been a fix for this?

#2715

@skandragon

This comment has been minimized.

skandragon commented Nov 21, 2014

And yesterday, when I first tried Velocity, it worked. Today, it does not. :P Great timing! I was just about to add it to another dozen projects or so.

In any case, I too am getting this error. I thought the referenced fix went into a pre-1.0 meteor, but apparently either it did not fix it, or it is a new issue. It's not clear what was fixed, as no commits reference that issue number.

@skandragon

This comment has been minimized.

skandragon commented Nov 21, 2014

What if it's called 1.0.0-rc6-cucumber? That is, without the .6?

@awatson1978

This comment has been minimized.

Contributor

awatson1978 commented Nov 21, 2014

It's the dash character. The dash is reserved for release suffix, and this creates two suffixes.

@skandragon

This comment has been minimized.

skandragon commented Nov 21, 2014

It seems that anything after the first dash should be the suffix...

@awatson1978

This comment has been minimized.

Contributor

awatson1978 commented Nov 21, 2014

Depends on how the regex is written.

@samhatoum

This comment has been minimized.

samhatoum commented Nov 21, 2014

I tried two new releases right after that, which are:

Version 1.0.0-rc.6-cucumber.0
Version 1.0.0-temp.0

None of them supercede the previously added two dashes

@skandragon

This comment has been minimized.

skandragon commented Nov 21, 2014

From the semantic versioning spec 2.0.0:

Identifiers MUST comprise only ASCII alphanumerics and hyphen [0-9A-Za-z-].

It looks like meteor's parser is not following the spec, regardless of how the regexp is written. :)

Plus, with the ability to have build info (with a + marker) it looks like a simple parser using a regexp would be out of the question at this point. It looks like it'll have to become stateful, and follow character by character to determine what it is parsing as it finds the first and following dots, dash, and plus sign.

@skandragon

This comment has been minimized.

skandragon commented Nov 21, 2014

Without actually doing it yet... I'd probably write this as:

  1. Split at the first + sign found. Discard the part after it.
  2. Split at the first - sign found. If there is anything after it, this is the prerelease identifier.
  3. Process the remainder as major.minor.patch format data.

The prerelease part may need to be further broken up by dots later, if the major.minor.patch comparison is equal, since "alpha" < "alpha.1".

@Sanjo

This comment has been minimized.

Contributor

Sanjo commented Nov 21, 2014

@skandragon

This comment has been minimized.

skandragon commented Nov 21, 2014

I don't think so. It looks like numbers are intended to be caught in the "if (typeof part === 'number')" case.

@skandragon

This comment has been minimized.

skandragon commented Nov 21, 2014

I think the real problem here is that pre-release IDs are probably able to be treated as simple strings for comparison, and instead it's trying to do magic.

Of course, that there are dots inside a pre-release ID that matter, makes this harder. I guess one could compare pure numeric fields as numeric, and any field that has a string as a string, but I have no idea what happens when you try to compare "alpha.0" and "alpha.b"

@Sanjo

This comment has been minimized.

Contributor

Sanjo commented Nov 21, 2014

I think the semver package parses the version to: { ..., prerelease: ['rc', '6-cucumber']}.
Therefore the second part with the 6 is no number.

@skandragon

This comment has been minimized.

skandragon commented Nov 21, 2014

So, to get this package working again for those of us wanting to use it... can Version 1.0.0-rc6-cucumber and rc.6-cucumber.0 be removed from the package system entirely?

@Sanjo

This comment has been minimized.

Contributor

Sanjo commented Nov 21, 2014

A temporary fix is to add velocity:core@1.0.0-rc.6 to your .meteor/packages file and your .meteor/versions file.

meteor update will fail though. You can temporarely remove all testing packages from .meteor/packages if you need to execute meteor update.

@rissem

This comment has been minimized.

Contributor

rissem commented Nov 21, 2014

Here's a simple reproduction repo I just made.

@skandragon

This comment has been minimized.

skandragon commented Nov 21, 2014

Can't each of the elements of a pre-release ID (that is, the parts between the dots) be treated as a string, and directly compared that way? "1" < "100", "10" < "100", etc. This appears to be the intent of the spec, but it's not clear on this. Thus, if you had an array:

["rc", "6-cucumber"] it would compare to ["rc", "6"] and come after it automatically and predictability.

@rissem

This comment has been minimized.

Contributor

rissem commented Nov 21, 2014

I just published velocity:core@0.3.0 (equivalent to rc5) as a work around for this issue.

@skandragon

This comment has been minimized.

skandragon commented Nov 21, 2014

Success! There is no way to un-publish something once it's published? That seems like a flaw.

@samhatoum

This comment has been minimized.

samhatoum commented Nov 21, 2014

Totally. I'm glad we've found an issue and I'm sure the MDG will fix it. Here's a story I'd love to see implemented:

As a package maintainer
I would like to remove/unpublish my packages
So that I can keep my users happy

:)

@glasser

This comment has been minimized.

Member

glasser commented Nov 21, 2014

OK, does that mean there's no current emergency issue here?

We can remove packages right now but it's not the greatest operation (eg it ends up requiring a full catalog resync for all users due to how the sync protocol works). And we'll look into the underlying bug soon too. (I apologize, I'm generally in charge of our GitHub bug triage process and I've been taking a few weeks of focusing on fixing [a few problems](https://github.com/meteor/meteor/milestones/Tool Performance and Stability) instead of incoming bugs!)

@Sanjo

This comment has been minimized.

Contributor

Sanjo commented Nov 21, 2014

OK, does that mean there's no current emergency issue here?

Yes. We have solved our issue, that velocity:core was no longer usable, by releasing a new version 0.3.0.

@glasser glasser changed the title from velocity:core dependents no longer working due to semvar issue to crashing bug in prerelease version parser Dec 4, 2014

@glasser glasser closed this in 44563f1 Dec 10, 2014

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