-
Notifications
You must be signed in to change notification settings - Fork 1.3k
fix(core): derive Version.VERSION from Maven project version instead of hardcoding #3087
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -206,7 +206,77 @@ | |
| </dependencies> | ||
|
|
||
| <build> | ||
| <resources> | ||
| <!-- Non-filtered default, excluding the version file so this block never writes the | ||
| same target path as the filtered block below. If both blocks copied | ||
| META-INF/agentscope/version.properties, the unfiltered copy (which runs first and | ||
| rewrites the target with a newer timestamp than the source) would make the | ||
| filtered copy be skipped by maven-resources-plugin (default overwrite=false), | ||
| silently packaging the literal ${project.version} placeholder. --> | ||
| <resource> | ||
| <directory>src/main/resources</directory> | ||
| <filtering>false</filtering> | ||
| <excludes> | ||
| <exclude>META-INF/agentscope/version.properties</exclude> | ||
| </excludes> | ||
| </resource> | ||
| <!-- Filter only the version file so ${project.version} is resolved. This block is the | ||
| sole writer of the target version.properties. --> | ||
| <resource> | ||
| <directory>src/main/resources</directory> | ||
| <filtering>true</filtering> | ||
| <includes> | ||
| <include>META-INF/agentscope/version.properties</include> | ||
| </includes> | ||
| </resource> | ||
| </resources> | ||
| <testResources> | ||
| <testResource> | ||
| <directory>src/test/resources</directory> | ||
| <filtering>false</filtering> | ||
| <excludes> | ||
| <exclude>agentscope-test-version.properties</exclude> | ||
| </excludes> | ||
| </testResource> | ||
| <testResource> | ||
| <directory>src/test/resources</directory> | ||
| <filtering>true</filtering> | ||
| <includes> | ||
| <include>agentscope-test-version.properties</include> | ||
| </includes> | ||
| </testResource> | ||
| </testResources> | ||
| <plugins> | ||
| <!-- Fail the build loudly if the filtered version resource still contains the | ||
| ${project.version} placeholder (i.e. Maven resource filtering did not apply). | ||
| Without this check the packaged jar would silently carry an unfiltered literal, | ||
| and Version.resolveVersionFrom would mask it by returning "unknown". --> | ||
| <plugin> | ||
| <groupId>org.apache.maven.plugins</groupId> | ||
| <artifactId>maven-antrun-plugin</artifactId> | ||
| <version>3.2.0</version> | ||
| <executions> | ||
| <execution> | ||
| <id>verify-version-resource-filtered</id> | ||
| <phase>process-classes</phase> | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
| <goals> | ||
| <goal>run</goal> | ||
| </goals> | ||
| <configuration> | ||
| <target> | ||
| <fail message="META-INF/agentscope/version.properties still contains the unfiltered 'dollar-brace' placeholder (the Maven project.version expression was not resolved): Maven resource filtering did not apply. Check the resource filtering configuration in agentscope-core/pom.xml."> | ||
| <condition> | ||
| <!-- A single '$' is enough to detect the unmapped | ||
| ${project.version} placeholder: the filtered file is | ||
| just 'version=2.0.3-SNAPSHOT' and contains none. --> | ||
| <resourcecontains resource="${project.build.outputDirectory}/META-INF/agentscope/version.properties" substring="$" /> | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good addition - this is the loud failure the previous round asked for. One nit: <resourcecontains resource="${project.build.outputDirectory}/META-INF/agentscope/version.properties" substring="project.version" />A correctly filtered file ( |
||
| </condition> | ||
| </fail> | ||
| </target> | ||
| </configuration> | ||
| </execution> | ||
| </executions> | ||
| </plugin> | ||
| <!-- Publish a test-jar so downstream modules (agentscope-harness) can reuse the | ||
| shared test fixtures (MockModel, MockToolkit, TestConstants, TestUtils). --> | ||
| <plugin> | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| version=${project.version} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Relying on declaration order to make the filtered copy win is fragile here: both resource blocks read the same
src/main/resourcestree and the same target path, andmaven-resources-pluginskips a copy when the destination is not older than the source (defaultoverwrite=false). Since the unfiltered copy runs first and rewritestarget/classes/META-INF/agentscope/version.propertieswith a newer timestamp than the source, the filtered copy can be skipped and the packaged jar ends up containing the literal${project.version}.Version.resolveVersionFromthen masks that by returning"unknown", so the failure is silent rather than loud. Safer: drop the catch-all unfiltered block's duplicate and exclude the version file from it, e.g. keep one<resource>forsrc/main/resourceswith<excludes><exclude>META-INF/agentscope/version.properties</exclude></excludes>plus the filtered resource — then add an assertion (or amaven-enforcer/groovycheck) that the packaged resource no longer contains${.