Stabilize local version file output#11661
Conversation
Local version files used to include the current git hash, so a HEAD-only change rewrote every generated *.version resource. Those changed resources dirtied downstream runtime classpaths and made tasks such as codenarcTest rerun after this version change. The change in #11651 (82ced12) used classpath normalization. But runtime classpath normalization on producer projects did not solve this because it is not propagated to consuming projects. Instead, this change now write a stable SNAPSHOT.dev on non-CI env, while CI keeps the previous version~hash format used for release artifacts.
This comment has been minimized.
This comment has been minimized.
|
Hi! 👋 Thanks for your pull request! 🎉 To help us review it, please make sure to:
If you need help, please check our contributing guidelines. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4418ed9253
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| "$trimmedVersion.dev" | ||
| } else { | ||
| "$trimmedVersion-SNAPSHOT.dev" |
There was a problem hiding this comment.
When a non-CI local build is made from two different commits with the same tracer project version, both branches here write the same *.version value (for example 1.64.0-SNAPSHOT.dev). That value is used as the tracer-version cache key in NoMatchFilter.discoverNoMatchFile and as the Gradle configuration-cache input set by CiVisibilityGradleListener; with dd.resolver.cache.dir or Gradle configuration cache enabled, a locally rebuilt agent can therefore reuse no-match/configuration cache entries from older instrumentation code and skip new preloads/task configuration. If the goal is only to ignore empty-commit churn, use a source/tree-based identifier or another cache-busting value for these local artifacts.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
This looks legit, I'll take a look.
There was a problem hiding this comment.
So yes, the root cause on local builds is that a stable version string gets written, like 1.64.0-SNAPSHOT.dev for different local commits, which is the purpose of this change.
However it has unwanted side effects, that ripple through and could cripple the local dev usability depending on what is done by the developer, also finding that is not immediate.
- Effect 1:
NoMatchFiltermay reuse a stale agent matcher results whendd.resolver.cache.diris enabled. - Effect 2:
CiVisibilityGradleListenermay let Gradle reuse stale configuration-cache entries because the tracer version input did not change. - A third finally, softer effect is observability/debugging: two different local agent builds become harder to distinguish by version alone, filename helps, but having the version is still better in my opinion.
| @get:Input | ||
| val ci: Property<Boolean> = objects.property<Boolean>() | ||
| .convention( | ||
| providerFactory.environmentVariable("CI") |
There was a problem hiding this comment.
are we setting that or should we expect that accept other semantic like 1/0?
There was a problem hiding this comment.
Good question! Indeed, that something I looked up more that once for either github or gilab.
Here's the official gitlab doc on CI.
🟢 Java Benchmark SLOs — All performance SLOs passed
PR vs. master results
Commit: Load and DaCapo benchmarks can be triggered manually in the GitLab pipeline. Results will appear in the Benchmarking Platform UI after completion. |
bric3
added
type: enhancement
|
After thinking about it, this might yield more problems than it solves. #11677 supersedes this PR and offer a better and broader fix to the issue in general, because it fixed the real underlying issue. |
3 participants
What Does This Do
Updates the version file plugin so non-CI environment write a stable local development versions such as
1.64.0-SNAPSHOT.devinstead of versions containing the current git hash. THis avoids dirtying the classpath, and forcing some tasks to be outdated.CI builds keep the previous
version~hashformat used for release artifacts.Motivation
Follow-up to #11651 and #11345.
Local version files previously included the current git hash, so a git HEAD-only change rewrote generated
*.versionresources. Even if other file file didn;t change these generated version files dirtied downstream runtime classpaths and made tasks such ascodenarcTestrerun after a version-only change.The classpath normalization attempt from #11651 did not solve this completely because runtime classpath normalization on producer projects is not propagated to consuming projects.
Additional Notes
./gradlew codenarcTest -PskipTeststook 2m12s on the warm run, then 15s on the immediate rerun../gradlew codenarcTest -PskipTeststook 13s. The raw log showed allcodenarcTesttasks up-to-date or no-source, with 0 executed, and allwriteVersionNumberFiletasks up-to-date.env CI=true ./gradlew :dd-trace-api:writeVersionNumberFile -PskipTestsgenerated1.64.0-SNAPSHOT~1b4d0ee540.