[
https://issues.apache.org/jira/browse/GROOVY-12461?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18130385#comment-18130385
]
ASF GitHub Bot commented on GROOVY-12461:
-----------------------------------------
codecov-commenter commented on PR #2995:
URL: https://github.com/apache/groovy/pull/2995#issuecomment-6344028001
##
[Codecov](https://app.codecov.io/gh/apache/groovy/pull/2995?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
Report
:white_check_mark: All modified and coverable lines are covered by tests.
:white_check_mark: Project coverage is 72.4663%. Comparing base
([`434b75d`](https://app.codecov.io/gh/apache/groovy/commit/434b75d05fa8bb2628b8908ecac0abd8158474cd?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache))
to head
([`3f61b7e`](https://app.codecov.io/gh/apache/groovy/commit/3f61b7ee4d9efd0d72a955e7e9dde7fd63ac1235?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
<details><summary>Additional details and impacted files</summary>
[](https://app.codecov.io/gh/apache/groovy/pull/2995?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
```diff
@@ Coverage Diff @@
## master #2995 +/- ##
==================================================
+ Coverage 72.4304% 72.4663% +0.0359%
- Complexity 40648 40674 +26
==================================================
Files 1643 1643
Lines 142795 142796 +1
Branches 26844 26844
==================================================
+ Hits 103427 103479 +52
+ Misses 29826 29779 -47
+ Partials 9542 9538 -4
```
| [Files with missing
lines](https://app.codecov.io/gh/apache/groovy/pull/2995?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
| Coverage Δ | |
|---|---|---|
|
[...ava/groovy/grape/GrabAnnotationTransformation.java](https://app.codecov.io/gh/apache/groovy/pull/2995?src=pr&el=tree&filepath=src%2Fmain%2Fjava%2Fgroovy%2Fgrape%2FGrabAnnotationTransformation.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3JjL21haW4vamF2YS9ncm9vdnkvZ3JhcGUvR3JhYkFubm90YXRpb25UcmFuc2Zvcm1hdGlvbi5qYXZh)
| `55.4960% <100.0000%> (+8.1842%)` | :arrow_up: |
... and [11 files with indirect coverage
changes](https://app.codecov.io/gh/apache/groovy/pull/2995/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
</details>
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test
Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests,
report on failures, and find test suite problems.
- :package: [JS Bundle
Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save
yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
> @GrabConfig(systemProperties = ...) sets JVM system properties at runtime
> even when Grape is disabled or unavailable
> --------------------------------------------------------------------------------------------------------------------
>
> Key: GROOVY-12461
> URL: https://issues.apache.org/jira/browse/GROOVY-12461
> Project: Groovy
> Issue Type: Improvement
> Affects Versions: 6.0.0
> Reporter: Paul King
> Assignee: Paul King
> Priority: Major
>
> h2. Problem
> When a class or script uses {{@Grab}} together with
> {{@GrabConfig(systemProperties = ...)}}, {{GrabAnnotationTransformation}}
> (line 490 in 6.0.0) adds a static initializer that does two things in order:
> # sets each listed system property, guarded by
> {{System.getProperty("groovy.grape.enable", "true") == "true"}};
> # calls {{Grape.grab(...)}}.
> The {{@GrabConfig}} javadoc says the properties "must be set before invoking
> the grab", for example SSL or proxy settings. Their only purpose is to
> support the grab. But the guard looks at the system property alone. It
> ignores the other two ways Grape can be off:
> * *No engine on the classpath.* In Groovy 6, Grape engines live in
> {{groovy-grape-ivy}} and {{groovy-grape-maven}}. With neither present,
> {{Grape.getInstance()}} returns null, {{Grape.grab}} does nothing, and the
> log says "Grapes disabled". The properties are still set.
> * *{{Grape.setEnableGrapes(false)}}.* {{Grape.grab}} checks the static
> {{enableGrapes}} flag and does nothing. The generated guard never looks at
> that flag, so the properties are still set.
> In both cases the class changes process-wide state, possibly trust-store or
> proxy settings, for a grab that never happens. Hosts that turn Grape off,
> such as embedders, tooling and notebook kernels, reasonably expect
> {{@GrabConfig}} to be inert too.
> A smaller inconsistency: {{Grape}} reads the property with
> {{Boolean.parseBoolean}}, which ignores case, while the generated guard
> compares it to the exact string "true". So {{-Dgroovy.grape.enable=TRUE}}
> turns grabbing on but skips the properties.
> h2. Reproducer
> {code:groovy}
> // G2.groovy
> @GrabConfig(systemProperties = 'probe.flag=set-by-grabconfig')
> @Grab('org.apache.commons:commons-lang3:3.14.0')
> import java.util.List
> println "flag=" + System.getProperty('probe.flag')
> {code}
> Results against Groovy 6.0.0 (JDK 17):
> ||Setup||Grab runs?||{{probe.flag}}||
> |core jar only ({{java -cp groovy-6.0.0.jar groovy.ui.GroovyMain
> G2.groovy}})|no ("Grapes disabled")|*set-by-grabconfig*|
> |engines present, {{Grape.enableGrapes = false}} before evaluating with
> {{GroovyShell}}|no|*set-by-grabconfig*|
> |core jar only, {{-Dgroovy.grape.enable=false}}|no|null|
> |engines present, {{GrabAnnotationTransformation}} disabled via
> {{CompilerConfiguration.disabledGlobalASTTransformations}}|no|null|
> |engines present, defaults|yes|set-by-grabconfig (expected)|
> When {{@Grab}} is on a class rather than a script, the properties are set
> when that class initializes, not when the script starts.
> h2. Proposed fix
> Set the properties only when a grab will actually run. The simplest form is
> to add a small public helper to {{Grape}}, for example
> {{Grape.isGrabAvailable()}}, returning {{enableGrapes && getInstance() !=
> null}}. The generated guard then calls the helper instead of reading the
> system property. This also removes the case mismatch, because the helper uses
> the same parsed flag as {{Grape.grab}}.
> An alternative is to pass the properties to {{Grape.grab}} in its arguments
> map, which the compile-time path already does but no engine reads. The facade
> would then apply them just before delegating to the engine. That changes
> compile-time behaviour, because the properties would also be set in the
> compiler's JVM. That should be a separate decision, so the minimal fix above
> is preferred for this ticket.
> h2. Compatibility
> Behaviour changes only when Grape is disabled or no engine is available. That
> is exactly when the properties had no effect on any grab. Code that relied on
> {{@GrabConfig}} as a general way to set system properties, without a working
> grab, would need to call {{System.setProperty}} itself. That seems unlikely,
> and it could be noted in the release notes.
> h2. Tests
> * With no engine on the test classpath, a class using
> {{@GrabConfig(systemProperties)}} leaves the property unset. Run it in a
> forked JVM, or in {{groovy-grape-test}} with a classloader that hides the
> engines.
> * After {{Grape.setEnableGrapes(false)}}, the same class leaves the property
> unset. Restore the flag afterwards.
> * With an engine present and grapes enabled, behaviour is unchanged.
> h2. Context
> Found while drafting GEP-32 (restricted compiler profile). The profile turns
> off the Grab transform for each compilation, which avoids this. Hosts that
> rely on keeping the engines off the classpath, or on
> {{Grape.setEnableGrapes(false)}}, do not get that protection.
--
This message was sent by Atlassian Jira
(v10.3.26#10030026)
