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

Fix version filename. #19

Closed
wants to merge 2 commits into from

Conversation

k-lergic
Copy link

Change the version filename from rally-version.properties to sbt-git-version.properties. :)

I upgraded things I could easily upgrade. I tried going to sbt 1.4.7 but there were syntax changes that cause the build to fail. :(

@htmldoug
Copy link

I totally removed generateVersion in https://github.com/AudaxHealthInc/rally-versioning/pull/115 in April 2020. Rationale:


https://github.com/AudaxHealthInc/rally-versioning/pull/111/files#r314966791 was never resolved, but merged anyway, and now it's causing problems:

[error] (cli / assembly) deduplicate: different file contents found in the following:
[error] /Users/doug.roper/code/openapi-codegen/cli/target/scala-2.11/openapi-cli_2.11-1.6.0-dirty-SNAPSHOT.jar:rally-version.properties
[error] /Users/doug.roper/code/openapi-codegen/codegen/target/scala-2.11/openapi-codegen_2.11-1.6.0-dirty-SNAPSHOT.jar:rally-version.properties
[error] /Users/doug.roper/code/openapi-codegen/wrapper/target/scala-2.11/openapi-wrapper_2.11-1.6.0-dirty-SNAPSHOT.jar:rally-version.properties

Inclusion here as-is does not make sense. If every lib adds its own version to the classpath at /rally-version.properties, it's a total crapshoot which version will be returned by https://github.com/AudaxHealthInc/lib-spartan/blob/f4352ef87180c53c332da626596202cd2621dab3/lib-spartan/src/main/scala/com/rallyhealth/spartan/v2/ops/DefaultModuleDetails.scala#L31. At the time of the PR, @jrouly said he'd never encountered this scenario--most likely because generateVersion was never included in the classpath of every library as it is now.

This code might make sense in rally-docker-sbt-plugin, but definitely not here. I'm backing out the whole thing until we can give it more thought.


It did end up in rally-docker-sbt-plugin mostly for backwards compatibility with lib-spartan/DefaultModuleDetails.

I'd recommend using https://github.com/sbt/sbt-buildinfo instead. @AndyKirsch also gutted the version determination in https://github.com/AudaxHealthInc/rally-versioning/pull/120, offloading that to https://github.com/dwijnand/sbt-dynver. https://github.com/lightbend/mima is perfectly useable these days on its own. I fixed our one gripe with it in lightbend-labs/mima#490. I would just use those directly.

Between those other plugins, does rally-versioning/sbt-git-versioning still add any novel value? If not, we should consider archiving it. I don't plan to maintain it. WDYT, @jeffmay @arkban @k-lergic?

@jeffmay
Copy link
Contributor

jeffmay commented Mar 23, 2021

I totally removed generateVersion in AudaxHealthInc/rally-versioning#115 in April 2020. Rationale:

https://github.com/AudaxHealthInc/rally-versioning/pull/111/files#r314966791 was never resolved, but merged anyway, and now it's causing problems:

[error] (cli / assembly) deduplicate: different file contents found in the following:
[error] /Users/doug.roper/code/openapi-codegen/cli/target/scala-2.11/openapi-cli_2.11-1.6.0-dirty-SNAPSHOT.jar:rally-version.properties
[error] /Users/doug.roper/code/openapi-codegen/codegen/target/scala-2.11/openapi-codegen_2.11-1.6.0-dirty-SNAPSHOT.jar:rally-version.properties
[error] /Users/doug.roper/code/openapi-codegen/wrapper/target/scala-2.11/openapi-wrapper_2.11-1.6.0-dirty-SNAPSHOT.jar:rally-version.properties

Inclusion here as-is does not make sense. If every lib adds its own version to the classpath at /rally-version.properties, it's a total crapshoot which version will be returned by https://github.com/AudaxHealthInc/lib-spartan/blob/f4352ef87180c53c332da626596202cd2621dab3/lib-spartan/src/main/scala/com/rallyhealth/spartan/v2/ops/DefaultModuleDetails.scala#L31. At the time of the PR, @jrouly said he'd never encountered this scenario--most likely because generateVersion was never included in the classpath of every library as it is now.

This code might make sense in rally-docker-sbt-plugin, but definitely not here. I'm backing out the whole thing until we can give it more thought.

It did end up in rally-docker-sbt-plugin mostly for backwards compatibility with lib-spartan/DefaultModuleDetails.

I'd recommend using https://github.com/sbt/sbt-buildinfo instead. @AndyKirsch also gutted the version determination in AudaxHealthInc/rally-versioning#120, offloading that to https://github.com/dwijnand/sbt-dynver. https://github.com/lightbend/mima is perfectly useable these days on its own. I fixed our one gripe with it in lightbend/mima#490. I would just use those directly.

Between those other plugins, does rally-versioning/sbt-git-versioning still add any novel value? If not, we should consider archiving it. I don't plan to maintain it. WDYT, @jeffmay @arkban @k-lergic?

I've been planning to migrate off of this plugin, but I haven't spent enough time on tying those other plugins into a coherent example project. Once I get something working, I might deprecate this library and link to a gist of an sbt project that provides the same basic capabilities needed for ShadingPlugin and SemVerPlugin.

@jeffmay
Copy link
Contributor

jeffmay commented Mar 23, 2021

I would favor removing the sbt-git-version.properties file generation in favor of combining this plugin with the sbt-buildinfo plugin. Maybe we can add a scripted test to make sure these plugins are compatible with each other? It can be a separate pull request.

@arkban
Copy link

arkban commented Mar 23, 2021

Uhh, this might need someone to take over since @k-lergic is no longer working on this.

@k-lergic k-lergic closed this Dec 9, 2022
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.

4 participants