-
-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
lib/build: more spec-compliant semver regex (fixes #9267 again) #9410
base: main
Are you sure you want to change the base?
Conversation
In PR#9316 support for meta-only version suffixes was added. However, some version strings still weren't supported. Notably, `v0.13.0+not.allowed.to.do.this` is a valid regex but gets rejected. Here's updated regex with a bunch of new test samples from semver.org
I don't think we ever aimed for a complete implementation of semver though. This is more about what we use, and what we need our other tools like usage reporting and upgrade servers etc to understand. The value of widening this here is not obvious to me. Your previous use case was "because we build things with these version numbers" but I'm more inclined to just say "don't do that, then" now... |
It's the same use-case actually. I had strings like
At the beginning my logic was that "since semver is used here, then I can probably use any valid semver string". So it's more or less just a convenience thing.
I'd agree with that. If you don't think it's worth the hassle I don't mind if it'd be possible to make just versions Another thing I noticed is that some incorrect semver strings are considered correct by existing regex: If some external tools are used, there is a some chance that they might not like version that syncthing thinks are correct. But that's just a side note because if nobody thought of that, then there are no issues with other tools. |
I don't know how this affects your case, but note that build tags like noupgrade are noted in the binary and visible in the version output already as is. |
Basically, I want to add some meta to build version so it can be clearly distinguished from official builds and from each other. But it's kinda hard to do without delimiters. For all intents and purposes, Then, it turned out again that it's not that simple, so I finally tried to debug regex to find out what I can use. That's when I found out that it's not really a semver. But again, I understand that it might not be a desirable change and I won't be fighting to get it merged. As a side note, while I wrote this comment, I noticed that |
In PR#9316 support for meta-only version suffixes was added. However, some version strings still weren't supported. Notably,
v0.13.0+not.allowed.to.do.this
is a valid regex but gets rejected.Here's updated regex with a bunch of new test samples from semver.org