Add contributing task #605
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
dnestoro
requested review from
melix,
mhalbritter,
olpaw,
fniephaus and
vjovanov
|
Hey, I'm a bit lost how the workflow works: It asks me a question where my tests are located. What tests? Existing tests for the library I want to add? I thought it creates some test stubs which I have then to implement. I ran the task |
mhalbritter
reviewed
Mar 21, 2025
tests/tck-build-logic/src/main/java/org/graalvm/internal/tck/ContributionTask.java
Outdated
Show resolved
Hide resolved
|
Okay, after cleaning up some empty directories it now continues, but then crashes with: I'm using |
|
Okay, i retried with a GraalVM 17, and now it looks better. One thing to note: I first added metadata for 2.12, and then for 2.11. Now the metadata index.json looks like this: [ {
"metadata-version" : "2.12",
"tested-versions" : [ "2.12" ],
"module" : "de.mkammerer:argon2-jvm"
}, {
"metadata-version" : "2.11",
"tested-versions" : [ "2.11" ],
"latest" : true,
"module" : "de.mkammerer:argon2-jvm"
} ]I don't think the |
|
I also noticed in the {
"allowed-packages" : [ "de.mkammerer.argon2", "de.mkammerer" ],
"directory" : "de.mkammerer/argon2-jvm",
"module" : "de.mkammerer:argon2-jvm"
}I get where the |
|
Now I've tried to add tests which not only need JUnit, but also some more dependencies (in this case In the first run, I've omitted Awaitility just to see what happens, and it breaks (which I guess is expected): Then i reran [ {
"metadata-version" : "2.12",
"tested-versions" : [ "2.12" ],
"module" : "de.mkammerer:argon2-jvm"
}, {
"metadata-version" : "2.12",
"tested-versions" : [ "2.12" ],
"latest" : true,
"module" : "de.mkammerer:argon2-jvm"
} ]The first |
mhalbritter
reviewed
Mar 21, 2025
tests/tck-build-logic/src/main/java/org/graalvm/internal/tck/ContributionTask.java
Outdated
Show resolved
Hide resolved
tests/tck-build-logic/src/main/java/org/graalvm/internal/tck/ContributionTask.java
Outdated
Show resolved
Hide resolved
tests/tck-build-logic/src/main/java/org/graalvm/internal/tck/ContributionTask.java
Outdated
Show resolved
Hide resolved
tests/tck-build-logic/src/main/java/org/graalvm/internal/tck/ContributionTask.java
Outdated
Show resolved
Hide resolved
tests/tck-build-logic/src/main/java/org/graalvm/internal/tck/ContributionTask.java
Outdated
Show resolved
Hide resolved
tests/tck-build-logic/src/main/java/org/graalvm/internal/tck/ContributionTask.java
Show resolved
Hide resolved
tests/tck-build-logic/src/main/java/org/graalvm/internal/tck/utils/FilesUtils.java
Outdated
Show resolved
Hide resolved
tests/tck-build-logic/src/main/resources/contributing/questions.json
Outdated
Show resolved
Hide resolved
tests/tck-build-logic/src/main/resources/contributing/questions.json
Outdated
Show resolved
Hide resolved
tests/tck-build-logic/src/main/resources/contributing/questions.json
Outdated
Show resolved
Hide resolved
this issue is already reported here |
It came from the |
@mhalbritter I am now throwing an exception in case you want to provide support for the library + same version of it, if it already exists. See this |
melix
reviewed
Mar 24, 2025
There was a problem hiding this comment.
I didn't take a look yet, just something that was obvious in the first place: you are invoking CLI in many places, but you shouldn't do this. You are running as a Gradle plugin, so you should use the Gradle constructs which are cross platform, e.g FileSystemOperations.
|
@melix @mhalbritter thanks for the feedback. I will refactor the PR to avoid using CLI, and ping you again when the PR is ready for review again. Until then, I will move this PR into a draft state, so please ignore incoming changes and don't test it until I finish refactoring |
dnestoro
force-pushed
the
dnestoro/add-contributing-task
branch
from
0f8780c to
26bed69
Compare
mhalbritter
approved these changes
May 5, 2025
dnestoro
force-pushed
the
dnestoro/add-contributing-task
branch
from
1b59e9d to
4ea3f77
Compare
olpaw
reviewed
May 19, 2025
| @Inject | ||
| protected abstract FileSystemOperations getFileSystemOperations(); | ||
|
|
||
| private Path gradlew; |
There was a problem hiding this comment.
Suggested change
| private Path gradlew; | |
| private final Path gradlew; |
There was a problem hiding this comment.
If I make it final, I can only initialize it immediately in the declaration (but since I am using Gradle's getProject to calculate those paths, I would rather call it when the task actually starts run function), otherwise I can't modify it in the initializeWorkingDirectories function.
olpaw
reviewed
May 19, 2025
|
|
||
| private final ObjectMapper objectMapper = new ObjectMapper().enable(SerializationFeature.INDENT_OUTPUT).setSerializationInclusion(JsonInclude.Include.NON_NULL); | ||
|
|
||
| private Path testsDirectory; |
There was a problem hiding this comment.
Suggested change
| private Path testsDirectory; | |
| private final Path testsDirectory; |
olpaw
reviewed
May 19, 2025
| private final ObjectMapper objectMapper = new ObjectMapper().enable(SerializationFeature.INDENT_OUTPUT).setSerializationInclusion(JsonInclude.Include.NON_NULL); | ||
|
|
||
| private Path testsDirectory; | ||
| private Path metadataDirectory; |
There was a problem hiding this comment.
Suggested change
| private Path metadataDirectory; | |
| private final Path metadataDirectory; |
olpaw
reviewed
May 19, 2025
tests/tck-build-logic/src/main/java/org/graalvm/internal/tck/ContributionTask.java
Outdated
Show resolved
Hide resolved
olpaw
reviewed
May 19, 2025
tests/tck-build-logic/src/main/java/org/graalvm/internal/tck/ContributionTask.java
Outdated
Show resolved
Hide resolved
olpaw
reviewed
May 19, 2025
tests/tck-build-logic/src/main/java/org/graalvm/internal/tck/ContributionTask.java
Outdated
Show resolved
Hide resolved
olpaw
reviewed
May 19, 2025
tests/tck-build-logic/src/main/java/org/graalvm/internal/tck/ContributionTask.java
Outdated
Show resolved
Hide resolved
olpaw
reviewed
May 19, 2025
tests/tck-build-logic/src/main/java/org/graalvm/internal/tck/ContributionTask.java
Outdated
Show resolved
Hide resolved
olpaw
reviewed
May 19, 2025
tests/tck-build-logic/src/main/java/org/graalvm/internal/tck/ContributionTask.java
Outdated
Show resolved
Hide resolved
olpaw
reviewed
May 19, 2025
tests/tck-build-logic/src/main/java/org/graalvm/internal/tck/ContributionTask.java
Outdated
Show resolved
Hide resolved
olpaw
reviewed
May 19, 2025
| } | ||
|
|
||
| InteractiveTaskUtils.printSuccessfulStatement("Contribution successfully completed! Thank you!"); | ||
| } |
There was a problem hiding this comment.
Should you have the whole questionnaire in a try-catch block?
Do you need to clean up if any of those IllegalStateExceptions below occur?
There was a problem hiding this comment.
I refactored code so that now we don't have anything that is not ContributingException or RuntimeException, so I guess there is no need for:
Should you have the whole questionnaire in a try-catch block?
olpaw
reviewed
May 19, 2025
tests/tck-build-logic/src/main/java/org/graalvm/internal/tck/ContributionTask.java
Outdated
Show resolved
Hide resolved
olpaw
reviewed
May 19, 2025
tests/tck-build-logic/src/main/java/org/graalvm/internal/tck/ContributionTask.java
Outdated
Show resolved
Hide resolved
olpaw
reviewed
May 19, 2025
tests/tck-build-logic/src/main/java/org/graalvm/internal/tck/ContributionTask.java
Outdated
Show resolved
Hide resolved
olpaw
reviewed
May 19, 2025
tests/tck-build-logic/src/main/java/org/graalvm/internal/tck/ContributionTask.java
Outdated
Show resolved
Hide resolved
olpaw
reviewed
May 19, 2025
| private Path getResourcesLocation(){ | ||
| ContributingQuestion question = questions.get("resourcesLocation"); | ||
| return InteractiveTaskUtils.askQuestion(question.question(), question.help(), (answer) -> { | ||
| if (answer.equalsIgnoreCase("-")) { |
There was a problem hiding this comment.
Wouldn't be more user-friendly to end a sequence of items with just ?
E.g.
foo<enter>
bar<enter>
<enter>
vs.
foo<enter>
bar<enter>
-<enter>
Also you could provide a generalized form of asking for a list of items in InteractiveTaskUtils and use it wherever you need lists of items.
There was a problem hiding this comment.
Wouldn't be more user-friendly to end a sequence of items with just
I am slightly against that approach... I intentionally added this required - because I want to avoid a situation where user accidentally presses enter two times and terminates the process unintentionally.
Also you could provide a generalized form of asking for a list of items in InteractiveTaskUtils and use it wherever you need lists of items.
Implemented here: https://github.com/oracle/graalvm-reachability-metadata/pull/605/files#diff-a552aa0289476dfc397600ecadf14028ef3677d3411da87fa24084ab1636db40R36
olpaw
reviewed
May 19, 2025
tests/tck-build-logic/src/main/java/org/graalvm/internal/tck/ContributionTask.java
Outdated
Show resolved
Hide resolved
dnestoro
force-pushed
the
dnestoro/add-contributing-task
branch
from
940d567 to
e17f44c
Compare
dnestoro
force-pushed
the
dnestoro/add-contributing-task
branch
from
5b91052 to
cd9800e
Compare
melix
reviewed
May 23, 2025
There was a problem hiding this comment.
This is an interesting abuse of Gradle tasks :)
My biggest concern is that the scaffolding assumes that the tests are already written, which sounds backwards. For a scaffolding task, I would expect that the task creates the structure for me, then I can implement tests.
| private final Map<String, ContributingQuestion> questions = new HashMap<>(); | ||
|
|
||
| private void initializeWorkingDirectories(){ | ||
| testsDirectory = Path.of(getProject().file(CoordinateUtils.replace("tests/src/$group$/$artifact$/$version$", coordinates)).getAbsolutePath()); |
There was a problem hiding this comment.
This uses getProject() at task execution time. Be aware that this is deprecated (because of configuration cache) so will not work anymore at some point in time. Instead, you should use the project's layout directory.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What does this PR do?
Adds interactive task that helps users to contribute to the metadata repository. The user only needs to provide path to tests and answer few questions in order to make a fully usable pull request.
For reviewers: