Preserve custom properties in gradle-wrapper.properties during wrapper updates #14026
Conversation
yeikel
left a comment
yeikel
left a comment
There was a problem hiding this comment.
I'll test it today and confirm the fix but the change itself looks good; Consider this an approval
Thanks @yeikel for reviewing it. |
|
@gmazzo , will it be possible if you also can review this? It will be helpful to be sure we are not breaking anything. |
….properties Implemented a general solution to preserve all custom properties in gradle-wrapper.properties during wrapper updates. The wrapper command regenerates the properties file with defaults, so we now capture original property values before the update and restore them afterward. Only properties that are intentionally updated (distributionUrl, distributionSha256Sum) are allowed to change. This ensures user customizations like networkTimeout are maintained. Added test fixture and test case to verify networkTimeout preservation during updates. Co-authored-by: kbukum1 <171620528+kbukum1@users.noreply.github.com>
Co-authored-by: kbukum1 <171620528+kbukum1@users.noreply.github.com>
Changed approach from "restore properties" to "update only what changed": - Save entire original file content (not just property values) - Run gradle wrapper command (generates new file) - Extract only the NEW values for distributionUrl and distributionSha256Sum - Update ONLY those properties in the original content via regex replacement - Write back the modified original content Benefits: - Preserves all comments in the file - Preserves property order - Preserves whitespace and formatting - Only modifies distributionUrl and distributionSha256Sum - More surgical approach Added test case to verify comments and order are preserved. Removed unused 'set' require since we no longer need it. Co-authored-by: kbukum1 <171620528+kbukum1@users.noreply.github.com>
Removed the following deprecated private methods as they are no longer used: - get_properties - restore_properties - get_validate_distribution_url_option - override_validate_distribution_url_option These methods were replaced by update_distribution_properties which handles property updates more elegantly while preserving file structure and comments. Also updated the comment on line 126 to reference the correct method. Co-authored-by: kbukum1 <171620528+kbukum1@users.noreply.github.com>
1. Remove unnecessary nil check for properties file (line 75) - The file is always populated from dependency_files in populate_temp_directory - If it doesn't exist, we should fail with an exception (File.read will raise) - Added comment explaining why the file should exist 2. Add explanatory comment for .dup call (line 202) - Clarifies that .dup is needed to avoid mutating the original_content parameter - gsub! modifies strings in-place, so we need a copy - This is defensive programming to prevent side effects 3. Update method signature - Changed original_content from T.nilable(String) to String - Removed unnecessary nil check inside the method These changes make the code clearer and more robust by failing fast if the expected file doesn't exist, rather than silently continuing with nil. Co-authored-by: kbukum1 <171620528+kbukum1@users.noreply.github.com>
Refined comments to be more concise and developer-friendly: 1. Line 74: Simplified from 2 verbose lines to 1 concise line - Removed explanation about populate_temp_directory (implementation detail) - Kept focus on what matters: preserving comments, order, and custom properties 2. Line 200-204: Simplified from 4 lines to 2 lines - Removed Ruby basics explanation about gsub! behavior - Kept essential info: why we dup and what we're updating Comments now focus on intent rather than implementation details. Tests already verify: - Comments are preserved (line 908-911) - Custom properties preserved (line 913-916) - Property order maintained (line 922-934) Co-authored-by: kbukum1 <171620528+kbukum1@users.noreply.github.com>
Add type annotation to updated_values hash to fix Sorbet type checking error.
The hash stores String keys (property names like "distributionUrl") and
String values (property values), so we annotate it as T::Hash[String, String].
This ensures that when iterating over the hash, the key variable is typed
as String, which satisfies Regexp.escape's type requirement.
Fixes Sorbet error 7018:
- Line 187: updated_values = T.let({}, T::Hash[String, String])
- Now key is String when passed to Regexp.escape on line 205
Co-authored-by: kbukum1 <171620528+kbukum1@users.noreply.github.com>
Fixed all Sorbet type errors and RuboCop linting issues: Sorbet fixes: - Wrapped Regexp.last_match(1) and Regexp.last_match(2) with T.must() to assert non-nil values (safe since we're inside a regex match) - This satisfies the T::Hash[String, String] type requirement RuboCop fixes: - Style/Next: Changed if block to use next unless pattern - Lint/UnusedBlockArgument: Renamed unused env: to _env: in test mocks - Layout/FirstMethodArgumentLineBreak: Added line break before first arg - Style/PercentLiteralDelimiters: Changed %w[] to %w() delimiter Verification: - bundle exec srb tc: No errors! - bundle exec rubocop: No offenses detected Co-authored-by: kbukum1 <171620528+kbukum1@users.noreply.github.com>
kbukum1
force-pushed
the
copilot/fix-network-timeout-issue
branch
from
57383fd to
1dcb8ce
Compare
|
I am going to check smoke test problem and possible create a PR using copilot to create a fix for smoke test in smoke test repository. Currently checking it |
|
Let me check this in detail tomorrow. AThat was the intended behavior. |
gmazzo
left a comment
gmazzo
left a comment
There was a problem hiding this comment.
I can't block the PR because I'm not a maintainer, but by feedback on this is negative.
The IA just worked under a false assumption, the final outcome will be the same since it's Gradle's wrapper task the one changing values, not us. More details described in here:
#14024 (comment)
Thank you so much. I am going to close the issue according to what you shared. Thanks for quick reply. |
| # Extract the new values for distributionUrl and distributionSha256Sum | ||
| new_content.each_line do |line| | ||
| next if line.strip.start_with?("#") || line.strip.empty? | ||
| next unless line =~ /^(distributionUrl|distributionSha256Sum)=(.*)$/ |
There was a problem hiding this comment.
The whole point of reading the .properties file here was to "preserve" the original value of validateDistributionUrl (if it was present), because we were forced to turn that feature off because dependabot's proxy was not letting the HTTP request go through.
I think the IA has hallucinated here and messed up the whole logic by looking for distributionUrl and distributionSha256Sum. That's the thing with IA stuff.
@yeikel @kbukum1 if there is an actual fix here, please let me know because I don't picture it 🤷♂️
Current Status: Tests Pass Locally ✅
Test Results
Ran tests with our latest changes:
About the Smoke Test Failure
The smoke test failure shown in the problem statement is revealing an important design question:
Current behavior: Our code preserves
networkTimeout=10000from original fileExpected behavior: Smoke test expects it to be REMOVED after upgrading to Gradle 9.2.0
Native Gradle 9.x: Doesn't auto-generate
networkTimeoutanymore (it was an incubating feature)The Question
Should we preserve properties that native Gradle no longer generates?
Based on research:
networkTimeout=10000wrappertask but it is an incubating feature gradle/gradle#27084) - forcing incubating featuresTwo Approaches
Approach 1: Current (Preserve Everything)
Approach 2: Match Native (Use generated file)
Recommendation
I believe Approach 2 is correct because:
gradle wrappernetworkTimeout=30000), we should preserve themnetworkTimeout=10000), we should respect Gradle's decision to remove themNext Action
Awaiting decision on which approach to take:
Original prompt
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.