Skip to content
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

Additional Scijava conflicts #96

Merged
merged 8 commits into from
Mar 14, 2024
Merged

Additional Scijava conflicts #96

merged 8 commits into from
Mar 14, 2024

Conversation

elect86
Copy link
Contributor

@elect86 elect86 commented Mar 14, 2024

As titled, added additional Scijava conflicts from https://github.com/scijava/pom-scijava/blob/7749f3d2e41f8516d2c0b269e7541763569e5490/pom.xml

Layout is weird, my Idea was indenting with 12 spaces instead of 8, I had to manually fix every string to match your original one.

Also, it might be worth to re-order some entries alphabetically and maybe have HIGHEST_VERSION by default unless specified? This will require some work with the constructors, I don't know honestly if it's worth, though, but Java is so verbose..

Ps: I noticed Gradle was also warning for the missing default name on :plugins, so I added that

Copy link
Member

@jjohannes jjohannes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @elect86. I have a few formatting comments.

Thank you for bringing up the points about organizing the CapabilityDefinitions.java file. We'll discuss this unrelated to this PR. We already have an issue for this (#70).

If you run a full quickCheck, there will be a complaint about not having the new entries in the samples/.... Maybe you can add them there (there should be a message that tells you where). If this does not work out – because the samples suffer from Gradle bug gradle/gradle#14220 – I can have a look at that later.

@jjohannes jjohannes added the a:rule One or more metadata rules that should be added or updated label Mar 14, 2024
@elect86
Copy link
Contributor Author

elect86 commented Mar 14, 2024

If you run a full quickCheck, there will be a complaint about not having the new entries in the samples/.... Maybe you can add them there (there should be a message that tells you where). If this does not work out – because the samples suffer from Gradle bug gradle/gradle#14220 – I can have a look at that later.

I'm hitting other crashes, sample-all and sample-all-deactivated logs

@jjohannes
Copy link
Member

We need a better guide on how to add new rules (#13)...
After you added the dependencies to sample-all and sample-all-deactivated you need to adjust the expected outputs:

https://github.com/gradlex-org/java-ecosystem-capabilities/blob/main/samples/sample-all/build.out
https://github.com/gradlex-org/java-ecosystem-capabilities/blob/main/samples/sample-all-deactivated/build.out

The changes will indicate if the rues behave as expected. I can also do the final adjustments when I try out the changes if you run into trouble there.

@elect86
Copy link
Contributor Author

elect86 commented Mar 14, 2024

@jjohannes
Copy link
Member

Manually?

You can either add everything by hand or overwrite the complete file with the output you get from the failing test. And then verify the changes in the Git diff.

- fixed JavaxTransactionApiRule::FIRST_JAKARTA_VERSION
@elect86
Copy link
Contributor Author

elect86 commented Mar 14, 2024

You can either add everything by hand or overwrite the complete file with the output you get from the failing test. And then verify the changes in the Git diff.

I have still quickCheck crashing with the same logs, I wouldn't trust the outputs..

@jjohannes
Copy link
Member

As, I said we need to work on documenting this contribution process better. 🙂
I can take it from here.

@elect86
Copy link
Contributor Author

elect86 commented Mar 14, 2024

As, I said we need to work on documenting this contribution process better. 🙂 I can take it from here.

thanks dear

I'll follow your changes closely, I'm curious to know what I did wrong/missed

Copy link
Member

@jjohannes jjohannes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I corrected the places that were not really conflicts (no overlapping classes).

The remaining additions seem to be correct.

Thank you @elect86. I will merge this.

Usually, we aim for releasing contributions to this plugin in a timely matter. But since we are in the transition to 2.0 for which we still need to clarify some things, it may take a bit until this gets released (goal for me is middle of April).

@jjohannes jjohannes merged commit b0c0cb9 into gradlex-org:main Mar 14, 2024
7 checks passed
@jjohannes
Copy link
Member

Commit I forgot to push to the PR (added on main): c1ce064

@jjohannes jjohannes added this to the 2.0 milestone Mar 14, 2024
@elect86
Copy link
Contributor Author

elect86 commented Mar 14, 2024

Usually, we aim for releasing contributions to this plugin in a timely matter. But since we are in the transition to 2.0 for which we still need to clarify some things, it may take a bit until this gets released (goal for me is middle of April).

Wouldn't you even consider a 1.5.3 or a 2.0-alpha?

@elect86
Copy link
Contributor Author

elect86 commented Mar 14, 2024

I pulled your changes, now I don't have those crashes anymore

What was that in the end? I cant figure it out

I still have some warnings, though

WARNING: An illegal reflective access operation has occurred
WARNING: Illegal reflective access by org.codehaus.groovy.vmplugin.v9.Java9 (file:/home/elect/.gradle/wrapper/dists/gradle-8.7-rc-1-bin/az2fcb0xtiluzp7jjh506zedb/gradle-8.7-rc-1/lib/groovy-3.0.17.jar) to method java.util.Collections$UnmodifiableCollection.toString()
WARNING: Please consider reporting this to the maintainers of org.codehaus.groovy.vmplugin.v9.Java9
WARNING: Use --illegal-access=warn to enable warnings of further illegal reflective access operations
WARNING: All illegal access operations will be denied in a future release
WARNING: An illegal reflective access operation has occurred
WARNING: Illegal reflective access by org.codehaus.groovy.vmplugin.v9.Java9 (file:/home/elect/.gradle/wrapper/dists/gradle-8.7-rc-1-bin/az2fcb0xtiluzp7jjh506zedb/gradle-8.7-rc-1/lib/groovy-3.0.17.jar) to method java.util.Collections$UnmodifiableCollection.toString()
WARNING: Please consider reporting this to the maintainers of org.codehaus.groovy.vmplugin.v9.Java9
WARNING: Use --illegal-access=warn to enable warnings of further illegal reflective access operations
WARNING: All illegal access operations will be denied in a future release
WARNING: An illegal reflective access operation has occurred
WARNING: Illegal reflective access by org.codehaus.groovy.vmplugin.v9.Java9 (file:/home/elect/.gradle/wrapper/dists/gradle-8.7-rc-1-bin/az2fcb0xtiluzp7jjh506zedb/gradle-8.7-rc-1/lib/groovy-3.0.17.jar) to method java.util.Collections$UnmodifiableCollection.toString()
WARNING: Please consider reporting this to the maintainers of org.codehaus.groovy.vmplugin.v9.Java9
WARNING: Use --illegal-access=warn to enable warnings of further illegal reflective access operations
WARNING: All illegal access operations will be denied in a future release
WARNING: An illegal reflective access operation has occurred
WARNING: Illegal reflective access by org.codehaus.groovy.vmplugin.v9.Java9 (file:/home/elect/.gradle/wrapper/dists/gradle-8.7-rc-1-bin/az2fcb0xtiluzp7jjh506zedb/gradle-8.7-rc-1/lib/groovy-3.0.17.jar) to method java.util.Collections$UnmodifiableCollection.toString()
WARNING: Please consider reporting this to the maintainers of org.codehaus.groovy.vmplugin.v9.Java9
WARNING: Use --illegal-access=warn to enable warnings of further illegal reflective access operations
WARNING: All illegal access operations will be denied in a future release
Task :checkstyleTest

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a:rule One or more metadata rules that should be added or updated
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants