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

Better support fo module version format #54

Open
alexeagle opened this issue Feb 2, 2023 · 15 comments · Fixed by #55
Open

Better support fo module version format #54

alexeagle opened this issue Feb 2, 2023 · 15 comments · Fixed by #55

Comments

@alexeagle
Copy link
Contributor

bazelbuild/bazel-central-registry#404 broke the registry.bazel.build site, because our deploy action has been broken: https://github.com/bazel-contrib/bcr-ui/actions/runs/4055858424

Error occurred prerendering page "/modules/rules_xcodeproj/1.0.0rc2". Read more: https://nextjs.org/docs/messages/prerender-error
Error: Invalid argument not valid semver ('1.0.0rc1' received)
    at validateAndParse (/home/runner/work/bcr-ui/bcr-ui/node_modules/.pnpm/compare-versions@4.1.3/node_modules/compare-versions/index.js:38:13)

I guess we have to patch this dependency to understand more semver formats, since rules_xcodeproj isn't following the spec https://semver.org/#semantic-versioning-specification-semver

/cc @brentleyjones

@alexeagle
Copy link
Contributor Author

Alternatively we could tighten the semantics on BCR itself to reject versions that don't follow the semver spec.

@brentleyjones
Copy link

Ahh, dang. Yeah, it should have a -, sorry.

@brentleyjones
Copy link

brentleyjones commented Feb 2, 2023

https://bazel.build/rules/lib/bazel_module#version doesn't say it has to be semver btw (though I did intend for it to be semver). Also no mention of semver here: https://bazel.build/external/overview#bzlmod.

@hobofan
Copy link
Member

hobofan commented Feb 3, 2023

According to this comment, the format is specifed in-code like this: https://cs.opensource.google/bazel/bazel/+/master:src/main/java/com/google/devtools/build/lib/bazel/bzlmod/Version.java

That leads me to believe that there is no other place where this is formalized right now.

each segment in the "release" part can be identifiers instead of just numbers (so letters are also allowed -- although hyphens are not)

With that, 1.0.0rc1 should be valid and interpreted as release_segments: ['1', '0', '0rc1']

@brentleyjones
Copy link

When I release 1.0.0, will it be "newer" than 1.0.0rc1? If not, how can I fix this now?

@alexeagle
Copy link
Contributor Author

I'd just run https://www.npmjs.com/package/semver-compare-cli via npx to answer that experimentally.

Do we think Bazel is correct here? Feels to me like we'll hit lots of other places where people's semver parsers will break and so BCR/bzlmod should be stricter about what it accepts.

@Wyverald
Copy link
Collaborator

Wyverald commented Feb 3, 2023

The version format is specified here: https://bazel.build/external/module#version_format

"1.0.0rc1" is newer than "1.0.0" because "0rc1" > "0" per https://semver.org/#spec-item-11, sub-item 4

@brentleyjones
Copy link

Good to know. I think this might catch others as well. I'll work around my own mistake though.

@Wyverald
Copy link
Collaborator

Wyverald commented Feb 3, 2023

https://bazel.build/rules/lib/bazel_module#version doesn't say it has to be semver btw (though I did intend for it to be semver).

This is quite an unfortunate situation :S https://bazel.build/rules/lib/bazel_module#version is the documentation for the bazel_module type (as in, the type of module_ctx.modules[0] in a module extension impl function).

The actual documentation for the module() directive is https://bazel.build/rules/lib/globals#module . The documentation organization is a flustercuck right now and I have a pending change to improve it, but I'm a bit afraid the potential for confusion will always be there.

@brentleyjones
Copy link

brentleyjones commented Feb 3, 2023

Just to confirm, 1.0.0.1 > 1.0.0rc1, right? (Actually, it's not, great)

@hobofan hobofan reopened this Feb 3, 2023
@hobofan hobofan changed the title Deploy is broken Better support fo module version format Feb 3, 2023
@brentleyjones
Copy link

The actual documentation for the module() directive is https://bazel.build/rules/lib/globals#module.

There it also doesn't say anything about version format.

@Wyverald
Copy link
Collaborator

Wyverald commented Feb 3, 2023

Just to confirm, 1.0.0.1 > 1.0.0rc1, right?

No; the comparison is lexicographical regarding dot-delimited sections, so 1.0.0 < 1.0.0.1 < 1.0.0rc1 < 1.0.1

EDIT: This is (partially) wrong: 1.0.0rc1 > 1.0.1. In fact, 1.0.0rc1 > 1.0.9, because 0rc1 is a string identifier that compares greater than any number identifier.

@Wyverald
Copy link
Collaborator

Wyverald commented Feb 3, 2023

The actual documentation for the module() directive is https://bazel.build/rules/lib/globals#module.

There it also doesn't say anything about version format.

Indeed, I'll fix that ASAP.

@Wyverald
Copy link
Collaborator

Wyverald commented Feb 3, 2023

It just occurred to me that we could mitigate the confusion by enforcing that the version list in metadata.json is sorted. That way, if a PR adds a new version and it's not at the end, you'll immediately see something is wrong.

@hobofan
Copy link
Member

hobofan commented Apr 25, 2023

It looks like @cgrindel has implemented proper version support in renovatebot/renovate#21606 if I'm not mistaken.

Will have to see if/how we could also use that here (and maybe whether we can extract that in a separate repo, so that all JS/TS projects that need Bazel module version support can take advandtage of it)?

Wyverald added a commit that referenced this issue Jul 24, 2024
Got bored and wanted to explore Typescript.

Tested on https://typescriptlang.org/play:

    > console.log(sortVersions(['1.2.3', '1.2.3-d', '1.2.3.bcr.1', '1.2.3rc3', '1', 'k']))
    ["k", "1.2.3rc3", "1.2.3.bcr.1", "1.2.3", "1.2.3-d", "1"] 

Addresses #54.

Fixes #142.
meteorcloudy added a commit that referenced this issue Aug 8, 2024
* Implement proper version sorting

Got bored and wanted to explore Typescript.

Tested on https://typescriptlang.org/play:

    > console.log(sortVersions(['1.2.3', '1.2.3-d', '1.2.3.bcr.1', '1.2.3rc3', '1', 'k']))
    ["k", "1.2.3rc3", "1.2.3.bcr.1", "1.2.3", "1.2.3-d", "1"] 

Addresses #54.

Fixes #142.

* fix formatting

* more formatting

* fiiiiiinnnne i'll install pnpm

---------

Co-authored-by: Yun Peng <pcloudy@google.com>
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 a pull request may close this issue.

4 participants