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

8086: Some refactoring for jmc/core #495

Closed
wants to merge 2 commits into from

Conversation

RealCLanger
Copy link
Collaborator

@RealCLanger RealCLanger commented Jun 11, 2023

I worked on some improvements/refactoring for jmc/core. In detail:

Better handling of Spotless and Checkstyle:
- Fix calling spotless/checkstyle directly in submodules by refering to a $jmc.config.path variable
- Integrate spotless into the regular build workflow

Cleanup of core/pom.xml:
- URL cleanups
- Plugin version bumps

Lighter test projects:
- They don't need to be p2 plugin projects since they only test basic functionality but not in the context of Eclipse/OSGI modules
- Disable some maven plugin executions that are not needed there


Progress

  • Commit message must refer to an issue
  • Change must be properly reviewed (1 review required, with at least 1 Committer)

Issue

  • JMC-8086: Some refactoring for jmc/core (Bug - P4)

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jmc.git pull/495/head:pull/495
$ git checkout pull/495

Update a local copy of the PR:
$ git checkout pull/495
$ git pull https://git.openjdk.org/jmc.git pull/495/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 495

View PR using the GUI difftool:
$ git pr show -t 495

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jmc/pull/495.diff

Webrev

Link to Webrev Comment

@bridgekeeper
Copy link

bridgekeeper bot commented Jun 11, 2023

👋 Welcome back clanger! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@openjdk openjdk bot added the rfr label Jun 11, 2023
@mlbridge
Copy link

mlbridge bot commented Jun 11, 2023

@RealCLanger RealCLanger mentioned this pull request Jun 14, 2023
2 tasks
Copy link
Collaborator

@bric3 bric3 left a comment

Choose a reason for hiding this comment

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

Looks good to me there's a nitpick about the usage of property in the javadoc plugin configuration

core/pom.xml Outdated Show resolved Hide resolved
core/pom.xml Show resolved Hide resolved
@openjdk
Copy link

openjdk bot commented Jun 15, 2023

@RealCLanger Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration. See OpenJDK Developers’ Guide for more information.

@@ -410,6 +418,11 @@
</bottom>
</configuration>
</plugin>
<plugin>
<groupId>org.apache.maven.plugins</groupId>
<artifactId>maven-install-plugin</artifactId>
Copy link
Member

@aptmac aptmac Jun 15, 2023

Choose a reason for hiding this comment

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

I can reproduce this now; something (I believe the maven-install-plugin) is causing the test jars to not end up where they're expected. I ran a super clean build (by additionally nuking my ~/.m2 to start from scratch), and ran into application build errors because application couldn't find the test jars.

[ERROR] Failed to execute goal on project org.openjdk.jmc.ui.common: Could not resolve dependencies for project org.openjdk.jmc:org.openjdk.jmc.ui.common:eclipse-plugin:9.0.0-SNAPSHOT: The following artifacts could not be resolved: org.openjdk.jmc:common.test:jar:9.0.0-SNAPSHOT, org.openjdk.jmc:flightrecorder.test:jar:9.0.0-SNAPSHOT: Could not find artifact org.openjdk.jmc:common.test:jar:9.0.0-SNAPSHOT -> [Help 1]

I played around with this PR a bit today, and ended up at a point where if I remove the maven-install-plugin usage that has been added to core, my jars will end up in my m2 repo.

IIRC the CI on GitHub caches the maven repo, so if just testing this PR there's no overwriting of the test jars so they'll continue to be there for the application build.

With maven-install-plugin:
pull-495-before

Without:
pull-495-after

Copy link
Member

Choose a reason for hiding this comment

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

Although, the removal of those test jars is one of the features of this PR.

Does this need additional changes to application poms? For example, the application/pom.xml (as well as uitests and test.jemmy) lists a dependency on common.test [0] and flightrecorder.test, which is causing the error. Simply removing those two dependencies and the build gets quite a bit farther, but eventually fails trying to resolve testlib.

[ERROR] Failed to execute goal org.eclipse.tycho:tycho-compiler-plugin:3.0.4:validate-classpath (default-validate-classpath) on project org.openjdk.jmc.rjmx.test: Execution default-validate-classpath of goal org.eclipse.tycho:tycho-compiler-plugin:3.0.4:validate-classpath failed: org.osgi.framework.BundleException: Bundle org.openjdk.jmc.rjmx.test cannot be resolved:org.openjdk.jmc.rjmx.test [128]
[ERROR]   Unresolved requirement: Require-Bundle: org.openjdk.jmc.testlib

[0] https://github.com/openjdk/jmc/blob/master/application/pom.xml#L147

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh, this is a good catch. My change unveils an issue in the dependencies. The only test project that application tests should need from core is testlib. And also, testlib should only be seen in test projects but not in other application modules. I cleared this out and cleaned up some more dependency definitions in core.

I didn't see it (and the GHA build) because the m2 repository was not pruned before the build and the existing modules like common.test, which we would not install any more after this change, were still cached.

@openjdk
Copy link

openjdk bot commented Jun 16, 2023

@RealCLanger Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration. See OpenJDK Developers’ Guide for more information.

@openjdk
Copy link

openjdk bot commented Jun 17, 2023

@RealCLanger Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration. See OpenJDK Developers’ Guide for more information.

@openjdk
Copy link

openjdk bot commented Jun 17, 2023

@RealCLanger Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration. See OpenJDK Developers’ Guide for more information.

@openjdk
Copy link

openjdk bot commented Jun 17, 2023

@RealCLanger Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration. See OpenJDK Developers’ Guide for more information.

@RealCLanger
Copy link
Collaborator Author

So, I believe I've fixed the further dependency issues now. It however makes the change a bit larger. What I did:
-> Move the testlib project out of the core/tests directory into core (direct child of org.openjdk.jmc:missioncontrol.core. With that, the maven module can be installed and subsequently be referenced/used without requiring org.openjdk.jmc:missioncontrol.core.tests, which we would not install any more.
-> Remove dependencies to org.openjdk.jmc:flightrecorder.test from org.openjdk.jmc:flightrecorder.rules.test and org.openjdk.jmc:flightrecorder.serializers.test since these test projects should be runnable on their own. This however leads to some duplication of test resources which could be checked in a later step

In application, we now drop the dependencies to the test projects in pom.xml (org.openjdk.jmc:missioncontrol.application) and then only add org.openjdk.jmc:testlib as additional dependency to the test projects.

@aptmac
Copy link
Member

aptmac commented Jun 19, 2023

Overall this is working a lot better now; a regular build of core through to application works as expected. With moving the testlib folder out of core/tests there are a handful of files that could use an update to their license headers though.

Just because I found it first here.. are you able to run the uitests?

With this PR being primarily changes to core I hadn't run the uitests on this, but now gave it a try since the testlib dependency affects jmc/application too. When getting into the uitest run it fails trying to find org.openjdk.jmc.rcp.application.app:

[..]
[INFO] missioncontrol.application.uitests 9.0.0-SNAPSHOT .. SUCCESS [  0.038 s]
[INFO] org.openjdk.jmc.test.jemmy 9.0.0-SNAPSHOT .......... FAILURE [  5.600 s]
[INFO] org.openjdk.jmc.browser.uitest 1.0.0-SNAPSHOT ...... SKIPPED
[INFO] org.openjdk.jmc.console.jconsole.uitest 1.0.0-SNAPSHOT SKIPPED
[INFO] org.openjdk.jmc.console.persistence.uitest 1.0.0-SNAPSHOT SKIPPED
[INFO] org.openjdk.jmc.console.uitest 1.0.0-SNAPSHOT ...... SKIPPED
[INFO] org.openjdk.jmc.flightrecorder.uitest 1.0.0-SNAPSHOT SKIPPED
[INFO] org.openjdk.jmc.rcp.application.uitest 1.0.0-SNAPSHOT SKIPPED
[INFO] missioncontrol.releng 9.0.0-SNAPSHOT ............... SKIPPED
[INFO] platform-definitions 9.0.0-SNAPSHOT ................ SKIPPED
[INFO] platform-definition-2023-03 9.0.0-SNAPSHOT ......... SKIPPED
[INFO] platform-definition-2022-12 9.0.0-SNAPSHOT ......... SKIPPED
[INFO] platform-definition-2022-09 9.0.0-SNAPSHOT ......... SKIPPED
[INFO] ------------------------------------------------------------------------
[INFO] BUILD FAILURE
[INFO] ------------------------------------------------------------------------
[INFO] Total time:  08:43 min
[INFO] Finished at: 2023-06-19T11:04:48-04:00
[INFO] ------------------------------------------------------------------------
[ERROR] Failed to execute goal org.eclipse.tycho:tycho-surefire-plugin:3.0.4:integration-test (default-integration-test) on project org.openjdk.jmc.test.jemmy: Could not find application "org.openjdk.jmc.rcp.application.app" in the test runtime. Make sure that the test runtime includes the bundle which defines this application. -> [Help 1]
[ERROR] 

After digging through the commits and testing, it looks like this is occurring for me after JMC-8086 [0]. (I should get in the habit of running these more often I guess..) Could it be related to the tycho dependency bump? Or is this just a failure that I'm seeing on my end? I can open a bug in JIRA if it's reproducible for other people.

[0] ae2fbf3

Edit: I'm not seeing this error on my Windows machine though. I'll poke around later this week but it's looking like I have some local issue going on. Thanks for checking!

@RealCLanger
Copy link
Collaborator Author

I could not recreate this uitests error, at least not when running the org.openjdk.jmc.test.jemmy project standalone. I would try to get clean tests and uitests on all my dev machines after the refactoring (also in application) is in.

As for copyright years: I would think if files are only moved around without changes, one doesn't need to modify the copyright years. That's at least my experience from OpenJDK. Please let me know what you'd prefer.

@openjdk
Copy link

openjdk bot commented Jun 19, 2023

@RealCLanger This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8086: Some refactoring for jmc/core

Reviewed-by: bdutheil, aptmac

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 1 new commit pushed to the master branch:

  • cedf50f: 8093: Rules dependant on GarbageCollectionInfoRule throw NPE if there is no gcInfo

Please see this link for an up-to-date comparison between the source branch of this pull request and the master branch.
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

@openjdk openjdk bot added the ready label Jun 19, 2023
@RealCLanger
Copy link
Collaborator Author

Thanks for the reviews.

/integrate

@openjdk
Copy link

openjdk bot commented Jun 20, 2023

Going to push as commit cf2fe2b.
Since your change was applied there has been 1 commit pushed to the master branch:

  • cedf50f: 8093: Rules dependant on GarbageCollectionInfoRule throw NPE if there is no gcInfo

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot added the integrated label Jun 20, 2023
@openjdk openjdk bot closed this Jun 20, 2023
@openjdk openjdk bot removed the ready label Jun 20, 2023
@openjdk openjdk bot removed the rfr label Jun 20, 2023
@openjdk
Copy link

openjdk bot commented Jun 20, 2023

@RealCLanger Pushed as commit cf2fe2b.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

@RealCLanger RealCLanger deleted the coretestsrefac branch June 20, 2023 04:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
3 participants