-
Notifications
You must be signed in to change notification settings - Fork 152
Add Gradle 9.0 support #768
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
Conversation
@@ -33,7 +33,7 @@ def build(self, source_dir, build_file, cache_dir=None, init_script_path=None, p | |||
if not self.os_utils.exists(build_file): | |||
raise BuildFileNotFoundError(build_file) | |||
|
|||
args = ["build", "--build-file", build_file] |
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.
Random thoughts here.
I see that now the build_file
parameter is not used anywhere in the method anymore, so maybe we could remove it from the function signature: build(self, source_dir, cache_dir=None, init_script_path=None, properties=None)
If we do that, we would probably want to update the rest of the places where this is used, like the actions file and remove it from the JavaGradleBuildAction fields, and then remove it from the workflow file where this is called.
This would potentially break users if they're using these classes directly instead of using the LambdaBuilder
object that SAM CLI uses, but I guess if that's the case, they can "not upgrade to the new lambda-builders version until they remove this extra parameter from their code".
Nah, but I think at the end of the day, it's okay to leave it like this. Maybe it could be better to add a comment here just mentioning that we're not using the build_file
parameter because Gradle doesn't support it anymore, but outside of that, I think we're okay.
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.
For some reason, when adding the Note, you undid the change to the parameter !
1c1f98e
to
6cd7ba8
Compare
…nd distsDirName property - Remove --build-file argument from gradle.py as it was deprecated in Gradle 8 and removed in Gradle 9 - Update lambda-build-init.gradle to use 'distributions' instead of deprecated distsDirName property - Update all related unit tests to reflect the command line changes - All unit and integration tests now pass with Gradle 9.0 Fixes: Unknown command-line option '--build-file' error when using Gradle 9.0
6cd7ba8
to
32d30ff
Compare
@@ -60,7 +60,7 @@ def test_build_no_init_script(self): | |||
gradle = SubprocessGradle(gradle_binary=self.gradle_binary, os_utils=self.os_utils) | |||
gradle.build(self.source_dir, self.manifest_path) | |||
self.os_utils.popen.assert_called_with( | |||
[self.gradle_path, "build", "--build-file", self.manifest_path], | |||
[self.gradle_path, "build"], |
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.
do we have test that actually runs gradle, rather than the mock?
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.
Line 3 in 3e531b3
distributionUrl=https\://services.gradle.org/distributions/gradle-8.4-bin.zip |
Seems the highest gradle version we are testing is gradle 8.4. Did we tested with 9.0?
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.
Addressed this in a new PR: #769
Add Gradle 9.0 support by removing deprecated --build-file argument and distsDirName property
Fixes: Unknown command-line option '--build-file' error when using Gradle 9.0
Issue #, if available:
Description of changes:
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.