-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Import properties from build.gradle and remove hardcoded quarkus.package.output-directory #29971
Conversation
This comment was marked as resolved.
This comment was marked as resolved.
Thanks a lot for this @jacobdotcosta! I'll let @glefloch and @aloubyansky comment |
integration-tests/gradle/src/main/resources/build-configuration/gradle.properties
Outdated
Show resolved
Hide resolved
b00ae13
to
ac97157
Compare
This comment has been minimized.
This comment has been minimized.
I'm not sure how to fix these errors as I'm not sure how they relate to this PR, could someone give me a hand? Gradle Tests - JDK 11 Windows has 1 error which is the following:
JVM Tests - JDK 17 MacOS M1 seems to fail to start the tests with:
|
The MacOS is known to be problematic. The |
270d407
to
b60598a
Compare
I've downloaded the logs and checked the error messages. From what I can understand the The problem seems to be that somewhere in the process the extension is trying to cleanup the @aloubyansky , could you assist me on a way of tracking this error? Do you know of any way I could try reproducing this error without having a MS Windows VM? Thank you. |
This comment has been minimized.
This comment has been minimized.
Perhaps, instead |
b60598a
to
a53fe0b
Compare
…age.output-directory value tests: IT for the build.gradle configuration
a53fe0b
to
74da3d7
Compare
I think I found the problem. I wasn't closing the Stream that reads the |
Thanks @jacobdotcosta! |
public Map<String, String> getQuarkusBuildProperties() { | ||
return quarkusBuildProperties; | ||
} | ||
|
||
public void set(String name, @Nullable String value) { | ||
quarkusBuildProperties.put(String.format("quarkus.%s", name), value); | ||
} |
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.
This should use lazy configuration APIs. So quarkusBuildProperties
should not be of type Map<String, String>
but MapProperty<String, String>
. At the same time there should be an overload for set
where value can be passed as a Property<String>
.
Another issue with this solution is that is assumes everything is a string, but that's not the case. For example the package.output-directoy
should probably be of type DirectoryProperty
(see https://docs.gradle.org/current/userguide/lazy_configuration.html#working_with_files_in_lazy_properties).
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.
@britter thanks a lot for the review. Let us know if you would like to submit a PR yourself. That'd be much appreciated.
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.
Thank you for the feedback @britter , I'm reviewing the docs. I can also submit a PR to fix this, if you won't.
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.
I don't see myself doing this before the end of the month. So if you have some time @jacobdotcosta please go ahead.
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.
It doesn't look like it was fixed yet. @britter in case you have some time to contribute the fixes you mentioned previously, it'll be much appreciated. Thanks!
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.
@aloubyansky I managed to address the first part of it in #30713. I'm not sure it's possible to address the second part using a map based approached.
Changes the type of quarkusBuildProperties from Map to MapProperty. This allows build authors to specify values which may only become available later in the build by passed into the build using providers. Follow up to quarkusio#29971
Changes the type of quarkusBuildProperties from Map to MapProperty. This allows build authors to specify values which may only become available later in the build by passed into the build using providers. Follow up to quarkusio#29971
Changes the type of quarkusBuildProperties from Map to MapProperty. This allows build authors to specify values which may only become available later in the build by passed into the build using providers. Follow up to quarkusio#29971 (cherry picked from commit f29e918)
build.gradle
file using the syntax described belowquarkus.package.output-directory
value is passed through configurationbuild.gradle
quarkus property definition:Closes #28897
Closes #28896