Skip to content

Implement simple versions and ranges #305

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

Merged
merged 10 commits into from
Jan 27, 2022
Merged

Conversation

thomashoneyman
Copy link
Member

@thomashoneyman thomashoneyman commented Jan 20, 2022

Fixes #43 by implementing new Version and Range types with parsers according to the spec described there.

In 5b3da44 I added a new Version.purs file that implements a Version type along with a parser and printer, enforcing that versions have the form X.Y.Z where each place is a non-negative integer. This file also implements a Range type with a parser and printer, enforcing that ranges have the form >=X.Y.Z <X.Y.Z. I also added tests to ensure this is working properly.

In 4a48ff5 I then replaced the existing SemVer and Range types with the new types throughout the application. This allows us to remove almost all of the SemVer module and the accompanying tests. We retain parseRange since we use that in the legacy import tool to convert ranges.

In f4295c2 I introduced 'strict' vs. 'lenient' parsers for the Version and Range types, as sometimes we are working with user input we expect to be valid (ie. when a user uploads a new package) and sometimes we expect the input to be invalid (ie. when converting from an existing Bowerfile). I've also added a raw field to both types as we need to preserve the input in the case of the lenient parser; for the strict parser the input is identical to the printing functions. One day we can probably remove the lenient mode, but we need it for the time being.

In d3517d1 I went further by doing more work to convert ranges that are technically incorrect but which could be converted into correct ranges. This saves us a lot of versions that would otherwise be kicked out of the registry due to an incorrect format. For example, the range 1.0.0 is invalid but can be rewritten to >=1.0.0 <1.0.1.

This change affects several hundred package versions, but the vast majority of those are not due to removing the prerelease identifiers. Instead, it's because we previously accepted many ranges that we shouldn't have, like * or >=1.0.0 or >1.0.0 <2.0.0 || >5.0.0 <6.0.0.

However, 237 package versions will be dropped due to using a prerelease identifier in their version, and 1 package version will be dropped due to using build metadata (out of ~13,000 total). The vast majority of these predate the use of package sets and won't affect typical user -- for example, many of them are in the PureScript libraries that were part of the 0.9 release back in 2016.

For this reason I feel comfortable with these versions being dropped and us sticking with the new strict simple ranges.


A topic for discussion: What if we just stripped all prerelease identifiers while cleaning version ranges in the legacy import tool? For example, a version range >=1.0.0-beta <2.0.0 would just be rewritten to >=1.0.0 <2.0.0. We'd still remove package versions that include a prerelease identifier, but we could fix packages that depend on those packages.

@thomashoneyman thomashoneyman marked this pull request as ready for review January 21, 2022 20:47
@thomashoneyman
Copy link
Member Author

For reference, here are the stats based on master prior to the Json class merging:

Packages: 1598 total (1230 totally succeeded, 248 partially succeeded, 120 totally failed)
Versions: 12602 total (11302 totally succeeded, 0 partially succeeded, 1300 totally failed)

Failures by error:
  manifestError: 1231 versions across 308 packages
    missingLicense: 501 versions across 147 packages
    badLicense: 415 versions across 55 packages
    badDependencyVersions: 330 versions across 118 packages
    badVersion: 61 versions across 40 packages
  noDependencyFiles: 68 versions across 36 packages
  resourceError: 0 versions across 31 packages
  malformedPackageName: 1 versions across 1 packages

271 manifest entries with unsatisfied dependencies

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.
@thomashoneyman thomashoneyman force-pushed the trh/simple-version-range branch from 7988f72 to f4295c2 Compare January 24, 2022 21:47
@thomashoneyman
Copy link
Member Author

And here's the results from this pull request:

Packages: 1598 total (877 totally succeeded, 409 partially succeeded, 312 totally failed)
Versions: 12598 total (9142 totally succeeded, 0 partially succeeded, 3456 totally failed)

Failures by error:
  manifestError: 3390 versions across 668 packages
    badDependencyVersions: 2323 versions across 467 packages
    missingLicense: 495 versions across 145 packages
    badLicense: 410 versions across 55 packages
    badVersion: 323 versions across 143 packages
  noDependencyFiles: 65 versions across 34 packages
  resourceError: 0 versions across 32 packages
  malformedPackageName: 1 versions across 1 packages

199 manifest entries with unsatisfied dependencies

We're looking at about 2,000 versions affected either because their package version is not good, or because they depend on package versions that are no good. This is more significant than I was expecting; I'll need to dig in a little more to see exactly why so many versions are affected.

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.
@thomashoneyman
Copy link
Member Author

thomashoneyman commented Jan 25, 2022

In the latest commit I've taken some actions to fix ranges that could be valid according to our simple range spec, but which currently aren't converted. Some examples:

  • When only an upper bound is specified we can set the lower bound to 0.0.0
    "<1.0.0" -> ">=0.0.0 <1.0.0", "<=0.1.1" -> ">=0.0.0 <0.1.0"

  • When ranges unexpectedly end with <= we can increment the patch version:
    ">=1.0.0 <=2.0.0" -> ">=1.0.0 <2.0.1"

  • When ranges unexpectedly begin with > we can increment the patch version:
    ">1.1.0 <2.0.0" -> ">=1.1.1 <2.0.0"

  • When ranges are unbounded towards 0, we can set the lower limit to 0.0.0:
    "<1.0.0" -> ">=0.0.0 <1.0.0"

With this change, the new stats are:

Packages: 1598 total (1110 totally succeeded, 344 partially succeeded, 144 totally failed)
Versions: 12598 total (10627 totally succeeded, 0 partially succeeded, 1971 totally failed)

Failures by error:
  manifestError: 1969 versions across 455 packages
    badDependencyVersions: 823 versions across 218 packages
    missingLicense: 497 versions across 146 packages
    badLicense: 424 versions across 65 packages
    badVersion: 323 versions across 143 packages
  resourceError: 0 versions across 32 packages
  malformedPackageName: 1 versions across 1 packages
  noDependencyFiles: 1 versions across 1 packages

254 manifest entries with unsatisfied dependencies

Many of the new failed versions have nothing to do with prerelease identifiers but rather to do with ranges we should never have accepted, like * or >=1.0.0 with no upper bound. That said, 237 packages have been dropped due to prerelease identifiers and 1 package has been dropped due to using build metadata.

@thomashoneyman thomashoneyman linked an issue Jan 25, 2022 that may be closed by this pull request
Copy link
Contributor

@JordanMartinez JordanMartinez left a comment

Choose a reason for hiding this comment

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

LGTM. Just a few places that can be cleaned up.

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
@thomashoneyman
Copy link
Member Author

In 0bfe903 I added support for trimming prerelease identifiers from dependency ranges when parsing in lenient mode. The new results with this change:

Packages: 1598 total (1124 totally succeeded, 331 partially succeeded, 143 totally failed)
Versions: 12601 total (10726 totally succeeded, 0 partially succeeded, 1875 totally failed)

Failures by error:
  manifestError: 1873 versions across 441 packages
    badDependencyVersions: 726 versions across 200 packages
    missingLicense: 497 versions across 146 packages
    badLicense: 424 versions across 65 packages
    badVersion: 323 versions across 143 packages
  resourceError: 0 versions across 32 packages
  malformedPackageName: 1 versions across 1 packages
  noDependencyFiles: 1 versions across 1 packages

255 manifest entries with unsatisfied dependencies

There are 83 packages that have moved from 'totally succeeded' to 'partially succeeded' and 23 packages that are now failing entirely.

There are about 400 more package versions that are affected by the BadDependencyVersions error, which is generally because they used ranges we don't support (a large number use ranges like * or >=1.0.0). There are 262 more package versions affected by the BadVersion error, which generally means these packages used prerelease identifiers or build metadata or something SemVer.parse recognized, but which are not accepted anymore.

@@ -83,14 +84,17 @@ exampleFailures = PackageFailures $ Map.fromFoldable
exampleStats :: Stats.Stats
exampleStats = Stats.errorStats mockStats
where
unsafeFromRight :: forall e a. Either e a -> a
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we put this in Registry.Prelude? I think we define this in quite a few places.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm down to do that, but I'd like to do it in a followup PR to keep this one from touching many more files.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sounds good!

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.
@thomashoneyman thomashoneyman merged commit 9c9d1bb into master Jan 27, 2022
@thomashoneyman thomashoneyman deleted the trh/simple-version-range branch January 27, 2022 19:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement simple versions & simple version ranges Suggestion: ban fancy semver ranges
4 participants