GROOVY-6908: Groovy Ant task should inherit Ant properties when in "f…#2476
GROOVY-6908: Groovy Ant task should inherit Ant properties when in "f…#2476paulk-asert merged 1 commit intoapache:masterfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Adds support for passing parent Ant project properties into the forked Groovy JVM (via system properties) when inheritAll="true" is used, and documents the behavior.
Changes:
- Introduce
inheritAlloption on theGroovyAnt task to copy parent Ant project properties into forked JVM system properties. - Add unit tests around property copying and collision behavior with explicitly provided
<sysproperty>. - Update the Groovy Ant task docs to describe
inheritAll,<sysproperty>, and<syspropertyset>usage with examples.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| subprojects/groovy-ant/src/main/java/org/codehaus/groovy/ant/Groovy.java | Adds inheritAll flag and logic to copy Ant project properties into forked JVM sysprops. |
| subprojects/groovy-ant/src/test/groovy/org/codehaus/groovy/ant/GroovyTest.java | Adds tests for property copying and explicit sysproperty precedence. |
| subprojects/groovy-ant/src/spec/doc/groovy-ant-task.adoc | Documents inheritAll and sysproperty/syspropertyset behavior with examples. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| public void testInheritAllPassesProjectProperties() { | ||
| Groovy task = new Groovy(); | ||
| task.setProject(project); | ||
| project.setProperty("alpha", "1"); | ||
| project.setProperty("beta", "two"); | ||
|
|
||
| task.setInheritAll(true); | ||
| task.passParentAntProperties(); | ||
|
|
||
| Map<String, String> passed = collectSysproperties(task); | ||
| assertEquals("1", passed.get("alpha")); | ||
| assertEquals("two", passed.get("beta")); | ||
| } | ||
|
|
||
| public void testInheritAllDoesNotOverrideExplicitSysproperty() { | ||
| Groovy task = new Groovy(); | ||
| task.setProject(project); | ||
| project.setProperty("shared", "fromProject"); | ||
|
|
||
| Environment.Variable explicit = new Environment.Variable(); | ||
| explicit.setKey("shared"); | ||
| explicit.setValue("fromSysproperty"); | ||
| task.addSysproperty(explicit); | ||
|
|
||
| task.setInheritAll(true); | ||
| task.passParentAntProperties(); | ||
|
|
||
| int sharedCount = 0; | ||
| for (Environment.Variable v : task.getSysProperties().getVariablesVector()) { | ||
| if ("shared".equals(v.getKey())) sharedCount++; | ||
| } | ||
| assertEquals(1, sharedCount); | ||
| assertEquals("fromSysproperty", collectSysproperties(task).get("shared")); | ||
| } |
There was a problem hiding this comment.
The new tests validate passParentAntProperties() directly, but they don’t exercise the actual forked execution path where this feature is supposed to work (i.e., that the properties become -D system properties in the child JVM). Consider adding an integration-style test (e.g., an Ant target with fork="true" inheritAll="true" that writes System.getProperty(...) values to an output file which the test can read) to ensure the end-to-end behavior is covered.
| // package-private for testing | ||
| void passParentAntProperties() { | ||
| Set<String> alreadySet = new HashSet<>(); | ||
| for (Environment.Variable v : super.getSysProperties().getVariablesVector()) { | ||
| alreadySet.add(v.getKey()); | ||
| } | ||
| for (Map.Entry<String, Object> entry : getProject().getProperties().entrySet()) { | ||
| Object value = entry.getValue(); | ||
| if (value == null || alreadySet.contains(entry.getKey())) continue; | ||
| Environment.Variable var = new Environment.Variable(); | ||
| var.setKey(entry.getKey()); | ||
| var.setValue(value.toString()); | ||
| super.addSysproperty(var); | ||
| } | ||
| } |
There was a problem hiding this comment.
passParentAntProperties() is package-private solely to be invoked from tests. To avoid expanding the production surface area, prefer keeping it private and testing the behavior through the public Ant task API (forked execution), or otherwise mark the method clearly as test-only (e.g., with a dedicated annotation/comment policy used elsewhere in the codebase).
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2476 +/- ##
==================================================
- Coverage 67.1472% 66.9879% -0.1593%
- Complexity 30944 30949 +5
==================================================
Files 1438 1440 +2
Lines 120148 120468 +320
Branches 21311 21364 +53
==================================================
+ Hits 80676 80699 +23
- Misses 32724 33023 +299
+ Partials 6748 6746 -2
🚀 New features to boost your workflow:
|
…orked" mode