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

Error resolving pre-release version #184

Closed
vladfaust opened this issue Dec 7, 2017 · 9 comments · Fixed by #249
Closed

Error resolving pre-release version #184

vladfaust opened this issue Dec 7, 2017 · 9 comments · Fixed by #249
Assignees
Milestone

Comments

@vladfaust
Copy link

vladfaust commented Dec 7, 2017

I'm trying to use meta versions as described in SemVer. My lib has v0.2.0-pre1 tag.

My app's shard looks like this:

mylib:
  github: username/mylib
  version: 0.2.0-pre1

Getting Error resolving mylib (0.2.0-pre1). What's wrong?

@vladfaust
Copy link
Author

vladfaust commented Dec 9, 2017

What's the right way to define pre-release versions in shards?

From https://semver.org:

Additional labels for pre-release and build metadata are available as extensions to the MAJOR.MINOR.PATCH format.

A pre-release version MAY be denoted by appending a hyphen and a series of dot separated identifiers immediately following the patch version. Identifiers MUST comprise only ASCII alphanumerics and hyphen [0-9A-Za-z-]. Identifiers MUST NOT be empty. Numeric identifiers MUST NOT include leading zeroes. Pre-release versions have a lower precedence than the associated normal version. A pre-release version indicates that the version is unstable and might not satisfy the intended compatibility requirements as denoted by its associated normal version. Examples: 1.0.0-alpha, 1.0.0-alpha.1, 1.0.0-0.3.7, 1.0.0-x.7.z.92.

@ysbaddaden
Copy link
Contributor

Sorry, that's probably not supported.

@ysbaddaden
Copy link
Contributor

Fixing the detection of release tags, is easy (a mere regular expression), but then the pre-releases, which are unstable, would become the latest release for shards, which must be changed to distinguish stable releases from unstable pre-releases, default to releases, but still allow installing specified pre-releases, and probably add some command line argument to enable pre-releases by default... I.e. a lot of work.

You may use tag: v0.2.0-pre1 for the time being.

@vladfaust vladfaust changed the title Error resolving rest (0.2.0-pre1) Error resolving pre-release version Dec 19, 2017
@straight-shoota
Copy link
Member

I'm willing to work on this.

I think a good solution would be to introduce a Shards::Version type to combine all the logic related to parsing and interpreting version strings.
Rubygem's Gem::Version could serve as a guideline, including VERSION_PATTERN = '[0-9]+(?>\.[0-9a-zA-Z]+)*(-[0-9A-Za-z-]+(\.[0-9A-Za-z-]+)*)? and #prerelease? predicate. The logic is quite simple: Any version string with a letter in it is considered a pre-release. This works with semver as well as other versioning schemes (for example 1.1.alpha).

@RX14
Copy link
Contributor

RX14 commented Dec 26, 2018

Why not attempt to parse the versions using the stdlib's semver parser and handle them properly instead of using regexes?

@straight-shoota
Copy link
Member

Because SemanticVersion strictly expects a string complying to the semver spec. But shards explicitly doesn't enforce using semver.

We should have a generic Version type which is capable of representing semver as well as other version specifiers. This would certainly be useful in the stdlib, a limitation to semver only isn't really useful in the reality of software versioning schemes. But the best path would probably be to implement such a type for shards and later consider promoting it to stdlib.

Btw. SemanticVersion also parses strings using a regex ;)

@ysbaddaden
Copy link
Contributor

ysbaddaden commented Dec 26, 2018

It doesn't have to be complex.

We can evolve SPEC to accept pre-releases as /-([.a-zA-Z0-9])$/ (semver like) with examples such as -alpha, -pre1 and -rc4 as good candidates (semver examples are horrible), stating that pre-releases have lower precedence over actual releases (i.e. 1.0.0-rc1 < 1.0.0), and won't be considered without the --prerelease command line option.

On the implementation side, we must first accept pre-releases:

  1. accept the pre-release suffix on version numbers (e.g. Shards::GitResolver::RELEASE_VERSION);
  2. add a --prerelease command line option to set Shards::Config.prerelease (defaults to false); no, that leads to confusing behavior, prereleases should always be opt-in and specified as such to be considered;
  3. modify Shards::Helpers::NaturalSort to give lower precedence to pre-release versions, but still sort them naturally already sorts as expected;

Then modify Shards::Helpers::Versions to handle the presence of pre-release versions, that is:

  1. never resolve a pre-release version over a previous stable version, for example a shard with versions 1.0.0-rc1, 0.9.2 and 0.9.1 should install 0.9.2...
  2. unless Shards::Config.prerelease is set, in which case it should resolve and install 1.0.0-rc1;
  3. unless the pre-release version if explicitly required (i.e. version: 1.0.0-rc1) or was previously locked;

Maybe even:

  1. print a warning when a pre-release is installed but Shards::Config.prerelease isn't set.

@straight-shoota
Copy link
Member

Implementation is of course debatable, but I don't think that a specific version type would be overly complex. It would combine several aspects of handling versions in one place.

More important at first is the design decision.
Adding /-([.a-zA-Z0-9])$/ is an improvement, but it is enough? Not everyone uses semver. Should pre-release formats like 1.0.alpha be allowed?
It would be good advise to look at the experience rubygems and other tools involved in handling software versions.

This includes updating the specification which doesn't look completely right.

@ysbaddaden
Copy link
Contributor

TBH I don't care much. Following rubygems is probably simpler, which means accepting both -pre1 and .pre1 as being the same thing.

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

Successfully merging a pull request may close this issue.

4 participants