[SPARK-54282][BUILD] Read bom version from pom in SBT - #52978
Conversation
| } | ||
| } | ||
|
|
||
| // SparkBom can be removed once https://github.com/heremaps/here-sbt-bom/pull/47 is merged |
There was a problem hiding this comment.
it's really nice! since this is not a blocker, maybe we can wait a few more days to have this upstream change get reviewed by the plugin authors
There was a problem hiding this comment.
I waited for heremaps/here-sbt-bom#47 to be reviewed and merged before moving forward with this PR. It will be good to have this PR merged as I have other changes pending this PR merge.
There was a problem hiding this comment.
@pan3793 I don't see any progress with heremaps/here-sbt-bom#47. How do you want to proceed?
|
@pan3793 Please check |
| private lazy val properties = settingKey[Properties]("Effective POM properties") | ||
| lazy val jacksonVersion = settingKey[String]("Jackson version") | ||
| lazy val jacksonModuleID = settingKey[ModuleID]("Jackson Module ID") | ||
| lazy val settings = Seq( | ||
| properties := SbtPomKeys.effectivePom.value.getProperties, | ||
| jacksonVersion := getVersion(properties.value, "fasterxml.jackson"), | ||
| jacksonModuleID := ModuleID("com.fasterxml.jackson", "jackson-bom", jacksonVersion.value), | ||
| ) | ||
|
|
||
| private def getVersionFromPom(properties: Properties, property: String) : String = { | ||
| properties.get(property).asInstanceOf[String] | ||
| } | ||
|
|
||
| private def getVersion(properties: Properties, property: String) : String = { | ||
| val version = property + ".version" | ||
| sys.props.get(version).getOrElse(getVersionFromPom(properties, version)) | ||
| } |
There was a problem hiding this comment.
small suggestion, hope to make it clearer
| private lazy val properties = settingKey[Properties]("Effective POM properties") | |
| lazy val jacksonVersion = settingKey[String]("Jackson version") | |
| lazy val jacksonModuleID = settingKey[ModuleID]("Jackson Module ID") | |
| lazy val settings = Seq( | |
| properties := SbtPomKeys.effectivePom.value.getProperties, | |
| jacksonVersion := getVersion(properties.value, "fasterxml.jackson"), | |
| jacksonModuleID := ModuleID("com.fasterxml.jackson", "jackson-bom", jacksonVersion.value), | |
| ) | |
| private def getVersionFromPom(properties: Properties, property: String) : String = { | |
| properties.get(property).asInstanceOf[String] | |
| } | |
| private def getVersion(properties: Properties, property: String) : String = { | |
| val version = property + ".version" | |
| sys.props.get(version).getOrElse(getVersionFromPom(properties, version)) | |
| } | |
| private lazy val pomProps = settingKey[Properties]("Effective POM properties") | |
| lazy val jacksonVersion = settingKey[String]("Jackson version") | |
| lazy val jacksonBom = settingKey[ModuleID]("Jackson BOM Module ID") | |
| lazy val settings = Seq( | |
| pomProps := SbtPomKeys.effectivePom.value.getProperties, | |
| jacksonVersion := getProperty(pomProps.value, "fasterxml.jackson.version"), | |
| jacksonBom := ModuleID("com.fasterxml.jackson", "jackson-bom", jacksonVersion.value), | |
| ) | |
| private def getProperty(pomProps: Properties, property: String) : String = { | |
| sys.props.get(property).getOrElse(pomProps.get(property).asInstanceOf[String]) | |
| } |
BTW, could you please move object DependencyVersions to line 1180?
| ) | ||
| } | ||
|
|
||
| object DependencyVersions { |
There was a problem hiding this comment.
code refactoring suggestions were almost ignored, I'd like to see the technical explanation when doing that.
here are my thoughts behind the suggestions:
- rename var
jacksonModuleID=>jacksonBom, theModuleID(...)already indicates the var type, what is important here isbom - merge
getVersionandgetVersionFromPomtogetProperty- it's overengineering, also disturbs IDE search functionality, when something goes wrong, or users want to understand wherefasterxml.jackson.versionis used, it's likely to do a global search
There was a problem hiding this comment.
code refactoring suggestions were almost ignored, I'd like to see the technical explanation when doing that.
There were too many changes in the code refactoring suggestion, and not every intention was clear. It would be much easier to explain what you wanted to change instead of providing a single code suggestion.
rename var
jacksonModuleID=>jacksonBom
It is not BOM, it is moduleID. BOM is the content of the file: "a BOM is a special type of POM file that lists a curated set of dependencies and their compatible versions. It's designed to simplify dependency management in projects that use multiple modules from the same ecosystem". ModuleID is a way to refer to pom, bom, and other artifacts using groupId, artifactId, and version. The best I can do is rename jacksonModuleID => jacksonBomModuleID (similar to pomProperties naming).
merge getVersion and getVersionFromPom to getProperty - it's overengineering, also disturbs IDE search functionality, when something goes wrong, or users want to understand where fasterxml.jackson.version is used, it's likely to do a global search
I tried your suggestion, and it does not work. IDE (IntelliJ) does not understand that pom property is used in SBT build anyway. The expectation that a string is always used "as is" is not a valid expectation. The goal here is to make DependencyVersions object to support other artifacts in the following PRs, and the current approach allows to avoid repeating ".version" over and over again.
There was a problem hiding this comment.
The best I can do is rename
jacksonModuleID=>jacksonBomModuleID
this makes sense.
IDE (IntelliJ) does not understand that pom property is used in SBT build anyway.
what I mean "search" is text search, something like grep.
There was a problem hiding this comment.
what I mean "search" is text search, something like grep.
The expectation that a string is always used "as is" is not a valid expectation. It may be used with macros, regular expressions, concatenated with another string and etc. What is expected in maven pom.xml for tags, does not apply to SBT.
|
We're closing this PR because it hasn't been updated in a while. This isn't a judgement on the merit of the PR in any way. It's just a way of keeping the PR queue manageable. |
What changes were proposed in this pull request?
Use pom file properties to read bom version from pom file and use it in SBT
Why are the changes needed?
To avoid redefinition of bom version in SBT build
Does this PR introduce any user-facing change?
No
How was this patch tested?
SBT build
Was this patch authored or co-authored using generative AI tooling?
No