search

sbt / sbt-dynver

An sbt plugin to dynamically set your version from git
Apache License 2.0
300 stars 47 forks source link

✨ Select highest tag when there are sibling tags #279

Open jprudent opened 1 year ago

jprudent commented 1 year ago

This fixes issue #238

jprudent commented 11 months ago

Oh I didn't notice tests were not passing on CI. I don't have time today I think. I will definitely try to fix those.

SethTisue commented 11 months ago

I had to approve CI to run. I'll see any further updates sooner now that I'm watching the PR.

jprudent commented 11 months ago

I run the tests locally. They are passing. Thanks for your review!

SethTisue commented 10 months ago

I'll take a look later this week.

SethTisue commented 10 months ago

(sorry for the slower-than-promised response time)

Well, this looks reasonable to me, though I don't know the code in this repo — my involvement here is largely administrative. So I'd be inclined to optimistically merge, as it could always be refined later.

But I have one question for @dwijnand and @eed3si9n — is there prior art for the version number comparison logic, something we could be using instead of adding or own custom code...? (I imagine this wheel has been reinvented many times in many programming languages...) Something in sbt itself perhaps?

eed3si9n commented 10 months ago

The exact detail of what's considered high or low actually depends on the impl detail of Ivy or Coursier for somethings like 1.0-A1 vs 1.0-RC1, iirc but sbt implements https://www.scala-sbt.org/1.x/api/sbt/librarymanagement/SemanticSelector.html, which lets you create a semantic versioning selector a la npm version expression, which can say greater than based on SemVer logic, which is usually good enough.

SethTisue commented 10 months ago

@jprudent want to take a stab at using the API Eugene suggests?

jprudent commented 10 months ago

sure! let's change the status of PR to changes required, I'll have a look soon

jprudent commented 9 months ago

Hello fellows, I need a bit of advice.

This project has 2 modules:

The class SemanticSelector is provided by librarymanagement-core which is an SBT artifact, and not available in dynver module. To use it I need to introduce a dependency or merge both modules. What would you do ?

dwijnand commented 9 months ago

Add the dependency to the core.

jprudent commented 9 months ago

Hello,

I added the dependency as provided. I also added a few tests.

sbt test ✅

Commits can be squashed.

SethTisue commented 9 months ago

I added the dependency as provided

What's the reasoning there?

Note that CI is failing.

jprudent commented 9 months ago

Hello Seth, thanks again for your time and feedback.

I didn't notice that the project was cross compiled. The CI does a sbt +test that runs tests for all scala versions, I didn't know.

I just changed the dependency to pull the correct artifact depending of scala version. Note that scala 3 artifacts are still alpha in the repo

The theory behind the Provided was that since the plugin is running in SBT, it already has the library in its classloader when executed and doesn't need to hard depend on it. But with the new definition of dependency in my last commit fce4d43, the dependency clashes with the one provided by SBT (tests failed in scripted). So I will reintroduce Provided. The danger with this kind of dependency is that the code is compiled against a given version of it, but at runtime the API may change or the Class can't be found. Since I only use a very small subset of it that looks quite stable (VersionSelector and SemanticSelector) we can expect that will never happen.

I will set the dep as Provided and I will write a scripted test (that I discovered in the process...) in a future commit of this PR.

Thanks again and sorry for all that churn.