Standardize versioning in dependabot common #10326
Conversation
amazimbe
changed the title
amazimbe
force-pushed
the
amazimbe/add-versioning-to-common
branch
4 times, most recently
from
cab4fd2
to
ca0d4c3
Compare
amazimbe
changed the title
| require "sorbet-runtime" | ||
| require "dependabot/version" | ||
|
|
||
| module Dependabot |
There was a problem hiding this comment.
Most of this code is from maven/version.rb. As explained in the PR description, this is one of the most comprehensive versioning implementations we have and I've decided to build on it for this task.
It will eventually be renamed to version.rb. I've had to add it because my approach is incremental so all ecosystems other than maven will still use the old version.rb and I'll incrementally subclass from new_version.rb until all ecosystems are covered.
There was a problem hiding this comment.
A lot of the code that's extracted into this new class seems very specific to Maven, for example the prefix tokens or named qualifiers are not valid in for example npm or Bundler:
irb(main):001> Gem::Version.new("1.0.0_pre1")
/Users/jurre/.rbenv/versions/3.3.1/lib/ruby/site_ruby/3.3.0/rubygems/version.rb:223:in `initialize': Malformed version number string 1.0.0_pre1 (ArgumentError)
raise ArgumentError, "Malformed version number string #{version}"It probably makes sense to keep things that are specific to the maven ecosystem in the maven implementation, and to only extract those things that are generic
There was a problem hiding this comment.
The goal is to standardize how dependabot handles versioning so we have an opinionated approach for all ecosystems. We do see non-standard version strings in the npm logs and having a standardized approach helps us to reason about the space.
SemVer2 actually supports pre-release fields and this is a step in that direction.
There was a problem hiding this comment.
Just to add: we are aiming to define a common version that is a bigger set that includes everything that's in the ecosystem-specific versions.
There was a problem hiding this comment.
Maven's version spec is not compatible with SemVer 2: https://maven.apache.org/pom.html#version-order-specification
The Maven version order algorithm is not compatible with Semantic Versioning 2.0.0. In particular, Maven does not special case the plus sign or consider build identifiers.
Each ecosystem diverges from SemVer in some way, I don't know of one that perfectly matches the spec. Currently we base common's version on Gem::Version which is also a variant of SemVer, and each ecosystem diverges from Gem::Version in some way or another as captured by their particular implementation.
So I'm not sure what swapping out the base Version class from Gem's variant to Maven's variant will achieve. If we wanted to reimplement the base Version to be exactly to spec, that might make more sense!
The goal is to standardize how dependabot handles versioning so we have an opinionated approach for all ecosystems.
I don't see how we can be opinionated about SemVer in our implementation, we're trying to exactly copy each ecosystem's behavior in Ruby. Any variance results in a bug report that we're not matching native behavior.
The best approach is probably have our base Version satisfy the SemVer spec exactly, and then each ecosystem only implements the variance. Again, this is kind of what we do now but the variance is from Gem::Version and not the SemVer spec.
| class Version < Dependabot::NewVersion | ||
| sig { override.overridable.params(version: VersionParameter, _version_string: T.nilable(String)).void } | ||
| def initialize(version, _version_string = nil) | ||
| super(version.to_s.tr("_", "-"), version.to_s) |
There was a problem hiding this comment.
do we have tests coverage?
There was a problem hiding this comment.
Also, any reason why we are doing the replace?
There was a problem hiding this comment.
what's the _version_string parameter used for?
There was a problem hiding this comment.
do we have tests coverage?
For now, just the current ecosystem specific tests but my plan is to add some generic ones as I incrementally update each ecosystem.
There was a problem hiding this comment.
Also, any reason why we are doing the replace?
The spec for the ruby superclass, Gem::Version works with dashes but not underscores. So without replacing we'd end up with the malformed version error.
There was a problem hiding this comment.
Also this comment "what's the _version_string parameter used for?"
It's not used but sorbet insisted that I add it. The initialize method in new_version.rb has an additional optional parameter to store the original version string and because I'm overriding it in here sorbet recommended doing it this way.
There was a problem hiding this comment.
Should it be a private variable for new_version then?
There was a problem hiding this comment.
I'm looking into it. If sorbet doesn't throw inexplicable errors I'll make it private
There was a problem hiding this comment.
@abdulapopoola sorbet still complains even if I make the variable private. Bacause initialize in new_version.rb accepts 2 keyword arguments, it expects every subclass of new_version to do the same.
There was a problem hiding this comment.
That's the question; why does new_version initiatialize need that as a public parameter if it is only used privately?
amazimbe
force-pushed
the
amazimbe/add-versioning-to-common
branch
from
7484a1d
to
881daf2
Compare
|
I'd recommend inheriting from this base class in of a handful of ecosystems, for example Or maybe even we can implement it in one other ecosystem that's not Maven, and show how it improves things there? |
@jurre the way I understand it, that's very similar to what we are doing but perhaps we're approaching it from the opposite end. We started with maven and gradle because the risk is very low so it could get us going quickly while I improve my understanding of the issues in readiness for more challenging / different ecosystem like npm / go / composer. |
|
I would like to see the benefits that this approach gives us in solving real world problems, extracting the maven code in a baseclass and then having maven inherit from it isn't going to allow us to learn much, that is fully expected to work. What's not clear to me is how this will help us when we start having other classes inherit from this new base class. What sort of problems is that going to solve, or is it going to cause some? It will be easy to show this by getting one of the other classes to inherit from the new base class. |
What: Move most versioning code into depenabot common so we can handle versioning in a standardized way. Why: To reduce issues relating to validity and comparisons of versions.
What: Move most versioning code into depenabot common so we can handle versioning in a standardized way. Why: To reduce issues relating to validity and comparisons of versions.
amazimbe
force-pushed
the
amazimbe/add-versioning-to-common
branch
from
881daf2
to
390f5be
Compare
|
Closing this PR. Following the feedback I received, the requirements have changed significantly and I've opened a new PR here: #10434 |
What are you trying to accomplish?
Move most versioning code into dependabot common so we can handle versioning in a standardised way across all ecosystems.
The goal is to reduce the amount of issues relating to validity and comparison of versions. Some of the issues are listed in this epic: #10187
Anything you want to highlight for special attention from reviewers?
I've chosen the versioning approach for Maven as the standard. It is the most comprehensive and the idea is that it should be able to handle the versioning implementations of many other ecosystems out of the box.
In follow up PRs, I'll make changes to the version file of each ecosystem to make sure it uses the new versioning code in
new_version.rbas well as to ensure the tests for that ecosystem pass.How will you know you've accomplished your goal?
Checklist