You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@daffodil.apache.org by "stevedlawrence (via GitHub)" <gi...@apache.org> on 2023/06/02 11:58:34 UTC
[GitHub] [daffodil] stevedlawrence opened a new pull request, #1022: Store Daffodil version in a file instead of in the manifest
stevedlawrence opened a new pull request, #1022:
URL: https://github.com/apache/daffodil/pull/1022
Daffodil has a Misc.getDaffodilVersion() function which gets the daffodil version from the package manifest. This is used in places like the CLI --version option, when writing the version to a saved parser, or checking the version when reloading a parser.
But if Daffodil is assembled into a fat jar, that manifest file is usually replaced, which means the wrong version can be found when saving or reloading parsers.
To avoid this, this modifies the SBT configuration to write the version to a resource file in org/apache/daffodil/lib/VERSION using a resource generator. The Misc.getDaffodilVersion() function is modified to read the contents of this resource to get the version. This no longer depends on the manifest file and uses a resource path that should never be discarded or overwritten when creating a fat jar.
Also modifies the genManaged task to use the managedSources/Resources tasks so we don't need to update it if we add more source/resource generators. And remove the type since it isn't needed--this is just a convenience to generate all managed files.
DAFFODIL-2816
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [daffodil] stevedlawrence commented on a diff in pull request #1022: Store Daffodil version in a file instead of in the manifest
Posted by "stevedlawrence (via GitHub)" <gi...@apache.org>.
stevedlawrence commented on code in PR #1022:
URL: https://github.com/apache/daffodil/pull/1022#discussion_r1214412980
##########
daffodil-lib/src/main/scala/org/apache/daffodil/lib/util/Misc.scala:
##########
@@ -250,12 +251,9 @@ object Misc {
* Returns the primary version of daffodil from the jar
*/
def getDaffodilVersion: String = {
- val implVersion = this.getClass.getPackage.getImplementationVersion
- if (implVersion == null) {
- ""
- } else {
- implVersion
- }
+ val uri = getRequiredResource("org/apache/daffodil/lib/VERSION")
+ val version = Source.fromInputStream(uri.toURL.openStream, "UTF-8").mkString
+ version
Review Comment:
Yep. To be a little bit more exact, the new `genVersion` task writes the version to `daffodil-lib/resource_managed/main/org/apache/daffodil/lib/VERSION`, which sbt copies to `daffodil-lib/target/classes/org/apache/daffodil/lib/` before it generates the jar.
Both the `resource_managed` and `target` directories are in the .gitignore so the file is never committed, its just generated for every build.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [daffodil] stevedlawrence commented on a diff in pull request #1022: Store Daffodil version in a file instead of in the manifest
Posted by "stevedlawrence (via GitHub)" <gi...@apache.org>.
stevedlawrence commented on code in PR #1022:
URL: https://github.com/apache/daffodil/pull/1022#discussion_r1214423936
##########
build.sbt:
##########
@@ -327,7 +328,8 @@ lazy val usesMacros = Seq(
lazy val libManagedSettings = Seq(
genManaged := {
- (Compile / genProps).value ++ (Compile / genSchemas).value
+ (Compile / managedSources).value
+ (Compile / managedResources).value
Review Comment:
Yep, that's the idea. Line 22 declares the task with a name, description, and type. Line 330 defines the actual body of that task to run when it is triggered at the sbt prompt (or if some other task triggers it). In this case, the body of the `genManaged` task just evaluates the two `managedSources` and `managedResources` tasks that are already defined by sbt. And since we added the `genVersion` task to `managedResources`, evaluating `managedResources` will evaluate `genVersion` (along with our other source/resource generators).
Note that we could in theory not even have `genManaged` and if someone wanted to generated all the managed stuff they could just do something like `sbt managedSources managedResources`, but one command is a bit more obvious.
##########
build.sbt:
##########
@@ -19,10 +19,11 @@ import scala.collection.immutable.ListSet
import sbtcc._
-lazy val genManaged = taskKey[Seq[File]]("Generate managed sources and resources")
+lazy val genManaged = taskKey[Unit]("Generate managed sources and resources")
lazy val genProps = taskKey[Seq[File]]("Generate properties scala source")
lazy val genSchemas = taskKey[Seq[File]]("Generate DFDL schemas")
lazy val genCExamples = taskKey[Seq[File]]("Generate C example files")
+lazy val genVersion = taskKey[Seq[File]]("Generate a VERSION file")
Review Comment:
Agreed, will fix.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [daffodil] stevedlawrence commented on a diff in pull request #1022: Store Daffodil version in a file instead of in the manifest
Posted by "stevedlawrence (via GitHub)" <gi...@apache.org>.
stevedlawrence commented on code in PR #1022:
URL: https://github.com/apache/daffodil/pull/1022#discussion_r1218019165
##########
build.sbt:
##########
@@ -373,8 +375,23 @@ lazy val libManagedSettings = Seq(
}
cachedFun(filesToWatch).toSeq
},
- Compile / sourceGenerators += (Compile / genProps).taskValue,
- Compile / resourceGenerators += (Compile / genSchemas).taskValue,
+ Compile / genVersion := {
+ val resourceDir = (Compile / resourceManaged).value
+ val outFile = resourceDir / "org" / "apache" / "daffodil" / "lib" / "VERSION"
+ if (!outFile.exists || IO.read(outFile) != version.value) {
Review Comment:
After a little testing and a little digging in the sbt code (I can't find much documentation about this), it looks like sbt uses hashes for things in `src/main/scala/` and uses last modification time for `src/main/resources`. I guess he concern is resources could be very large and could slow things down? I think it also uses last modification time for both managed sources and resources. And since this is a managed resource it uses last modification time.
I do think it's configurable, but I'd rather not change it--the more settings we tweak in sbt the more complicated it gets, and it's already too complicated. And for sbt source/resourceGenerators it's common practice to do something like this so we only generate files when needed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [daffodil] tuxji commented on a diff in pull request #1022: Store Daffodil version in a file instead of in the manifest
Posted by "tuxji (via GitHub)" <gi...@apache.org>.
tuxji commented on code in PR #1022:
URL: https://github.com/apache/daffodil/pull/1022#discussion_r1214348628
##########
build.sbt:
##########
@@ -19,10 +19,11 @@ import scala.collection.immutable.ListSet
import sbtcc._
-lazy val genManaged = taskKey[Seq[File]]("Generate managed sources and resources")
+lazy val genManaged = taskKey[Unit]("Generate managed sources and resources")
lazy val genProps = taskKey[Seq[File]]("Generate properties scala source")
lazy val genSchemas = taskKey[Seq[File]]("Generate DFDL schemas")
lazy val genCExamples = taskKey[Seq[File]]("Generate C example files")
+lazy val genVersion = taskKey[Seq[File]]("Generate a VERSION file")
Review Comment:
Picky, I know, but the sentence should not use an indefinite article ('a'). We use the indefinite article when not knowing which one of several VERSION files we are referring to or showing that the file is one of a group of VERSION files. There is only 'the' VERSION file, not 'a' VERSION file. However, none of the other sentences use definite or indefinite articles, so let's drop the 'a'.
##########
daffodil-lib/src/main/scala/org/apache/daffodil/lib/util/Misc.scala:
##########
@@ -250,12 +251,9 @@ object Misc {
* Returns the primary version of daffodil from the jar
*/
def getDaffodilVersion: String = {
- val implVersion = this.getClass.getPackage.getImplementationVersion
- if (implVersion == null) {
- ""
- } else {
- implVersion
- }
+ val uri = getRequiredResource("org/apache/daffodil/lib/VERSION")
+ val version = Source.fromInputStream(uri.toURL.openStream, "UTF-8").mkString
+ version
Review Comment:
In case it's not obvious to the next reviewer, the managed resource `VERSION` is generated inside the `daffodil-lib/target/classes/org/apache/lib directory` so it doesn't need to be put under source control or excluded from source control.
##########
build.sbt:
##########
@@ -327,7 +328,8 @@ lazy val usesMacros = Seq(
lazy val libManagedSettings = Seq(
genManaged := {
- (Compile / genProps).value ++ (Compile / genSchemas).value
+ (Compile / managedSources).value
+ (Compile / managedResources).value
Review Comment:
I was confused by not seeing any definition of `managedSources managedResources`, so I read the sbt documentation. I think here's what's going on. Line 22 above declares `genManaged` is a sbt task which we can type at the sbt prompt. I don't know the exact semantics of the `genManaged =` on line 22 and `genManaged :=` on line 330, but line 330 tells sbt that in order to evaluate `genManaged` (which has no value, but still needs to be evaluated), sbt must also get (and subsequently discard) the values of `managedSources managedResources` which are implicitly generated on the fly by calling the `sourceGenerators resourceGenerators` which we do define on lines 388 and 391.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [daffodil] mbeckerle commented on a diff in pull request #1022: Store Daffodil version in a file instead of in the manifest
Posted by "mbeckerle (via GitHub)" <gi...@apache.org>.
mbeckerle commented on code in PR #1022:
URL: https://github.com/apache/daffodil/pull/1022#discussion_r1217959524
##########
build.sbt:
##########
@@ -373,8 +375,23 @@ lazy val libManagedSettings = Seq(
}
cachedFun(filesToWatch).toSeq
},
- Compile / sourceGenerators += (Compile / genProps).taskValue,
- Compile / resourceGenerators += (Compile / genSchemas).taskValue,
+ Compile / genVersion := {
+ val resourceDir = (Compile / resourceManaged).value
+ val outFile = resourceDir / "org" / "apache" / "daffodil" / "lib" / "VERSION"
+ if (!outFile.exists || IO.read(outFile) != version.value) {
Review Comment:
I'm surprised you have to write this logic. Shouldn't sbt already have this kind of logic built in somewhere?
I mean if you overwrite a file with the exact same contents, sbt doesn't rebuild based on that normally does it? It's not purely date-based reasoning. I thought it created a hash value and only rebuilds if both the date and hash value changed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [daffodil] stevedlawrence merged pull request #1022: Store Daffodil version in a file instead of in the manifest
Posted by "stevedlawrence (via GitHub)" <gi...@apache.org>.
stevedlawrence merged PR #1022:
URL: https://github.com/apache/daffodil/pull/1022
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org