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

fix: fix class and resource loading in maven plugin #20465

Merged
merged 10 commits into from
Nov 20, 2024

Conversation

mcollovati
Copy link
Collaborator

Description

Run Flow mojos using an isolated class loader that includes both project and plugin dependencies, with project dependencies taking precedence. This ensures that classes are always loaded from the same class loader at runtime, preventing errors where a class might be loaded by the plugin's class loader while one of its parent classes is only available in the project’s class loader (see #19616).

Additionally, this approach prevents the retrieval of resources from plugin dependencies when the same artifact is defined within the project (see #19009).

This refactoring also introduces caching for ClassFinder instances per execution phase, allowing multiple goals configured for the same phase to reuse the same ClassFinder. It also removes the need to instantiate a ClassFinder solely for Hilla class checks, reducing the number of scans performed during the build.

Fixes #19616
Fixes #19009
Fixes #20385

Type of change

  • Bugfix
  • Feature

Checklist

  • I have read the contribution guide: https://vaadin.com/docs/latest/guide/contributing/overview/
  • I have added a description following the guideline.
  • The issue is created in the corresponding repository and I have referenced it.
  • I have added tests to ensure my change is effective and works as intended.
  • New and existing tests are passing locally with my change.
  • I have performed self-review and corrected misspellings.

Additional for Feature type of change

  • Enhancement / new feature was discussed in a corresponding GitHub issue and Acceptance Criteria were created.

Run Flow mojos using an isolated class loader that includes both project and
plugin dependencies, with project dependencies taking precedence. This ensures
that classes are always loaded from the same class loader at runtime, preventing
errors where a class might be loaded by the plugin's class loader while one of
its parent classes is only available in the project’s class loader (see #19616).

Additionally, this approach prevents the retrieval of resources from plugin
dependencies when the same artifact is defined within the project (see #19009).

This refactoring also introduces caching for ClassFinder instances per
execution phase, allowing multiple goals configured for the same phase to reuse
the same ClassFinder. It also removes the need to instantiate a ClassFinder
solely for Hilla class checks, reducing the number of scans performed during
the build.

Fixes #19616
Fixes #19009
Fixes #20385
@mcollovati
Copy link
Collaborator Author

If this PR gets merged, the only change in Hilla should be fixing mock configuration in hilla-maven-plugin tests

Copy link

github-actions bot commented Nov 13, 2024

Test Results

1 157 files  + 1  1 157 suites  +1   1h 31m 49s ⏱️ - 1m 46s
7 502 tests + 4  7 449 ✅ + 4  53 💤 ±0  0 ❌ ±0 
7 880 runs  +41  7 817 ✅ +41  63 💤 ±0  0 ❌ ±0 

Results for commit 684a080. ± Comparison against base commit 2977de2.

♻️ This comment has been updated with latest results.

@vaadin-bot vaadin-bot added +1.0.0 and removed +0.0.1 labels Nov 15, 2024
Copy link
Member

@tltv tltv left a comment

Choose a reason for hiding this comment

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

Reflector class is in two places. My comments apply in both.

Could Reflector and also some methods in BuildDevBundleMojo and FlowModeAbstractMojo go in shared flow-plugin-base module? There's already dependency to flow-plugin-base and both mojo's seem to use classes from there. See BuildFrontendUtil for example.

@mcollovati
Copy link
Collaborator Author

Could Reflector and also some methods in BuildDevBundleMojo and FlowModeAbstractMojo go in shared flow-plugin-base module? There's already dependency to flow-plugin-base and both mojo's seem to use classes from there. See BuildFrontendUtil for example.

I was thinking the same, but followed the current solution of copy/pasting code between mojos.
I'll try to refactor as you suggest.

@mcollovati
Copy link
Collaborator Author

Looking at the code again, what blocks me from moving Reflector to the common module is that it references Maven API but the module is used also by gradle plugin.

@tltv
Copy link
Member

tltv commented Nov 18, 2024

Looking at the code again, what blocks me from moving Reflector to the common module is that it references Maven API but the module is used also by gradle plugin.

OK, we can just keep the copy now for this PR and not let that block merging.

@vaadin-bot vaadin-bot added +0.0.1 and removed +1.0.0 labels Nov 18, 2024
Copy link

sonarcloud bot commented Nov 18, 2024

@mshabarov mshabarov merged commit 0a7c507 into main Nov 20, 2024
26 checks passed
@mshabarov mshabarov deleted the issues/19616-maven-plugin-classloading-attempt-3 branch November 20, 2024 07:13
@vaadin-bot
Copy link
Collaborator

Hi @mcollovati and @mshabarov, when i performed cherry-pick to this commit to 24.5, i have encountered the following issue. Can you take a look and pick it manually?
Error Message:
Error: Command failed: git cherry-pick 0a7c507
error: could not apply 0a7c507... fix: fix class and resource loading in maven plugin (#20465)
hint: After resolving the conflicts, mark them with
hint: "git add/rm ", then run
hint: "git cherry-pick --continue".
hint: You can instead skip this commit with "git cherry-pick --skip".
hint: To abort and get back to the state before "git cherry-pick",
hint: run "git cherry-pick --abort".

@vaadin-bot
Copy link
Collaborator

Hi @mcollovati and @mshabarov, when i performed cherry-pick to this commit to 24.4, i have encountered the following issue. Can you take a look and pick it manually?
Error Message:
Error: Command failed: git cherry-pick 0a7c507
error: could not apply 0a7c507... fix: fix class and resource loading in maven plugin (#20465)
hint: After resolving the conflicts, mark them with
hint: "git add/rm ", then run
hint: "git cherry-pick --continue".
hint: You can instead skip this commit with "git cherry-pick --skip".
hint: To abort and get back to the state before "git cherry-pick",
hint: run "git cherry-pick --abort".

mcollovati added a commit that referenced this pull request Nov 20, 2024
Run Flow mojos using an isolated class loader that includes both project and
plugin dependencies, with project dependencies taking precedence. This ensures
that classes are always loaded from the same class loader at runtime, preventing
errors where a class might be loaded by the plugin's class loader while one of
its parent classes is only available in the project’s class loader (see #19616).

Additionally, this approach prevents the retrieval of resources from plugin
dependencies when the same artifact is defined within the project (see #19009).

This refactoring also introduces caching for ClassFinder instances per
execution phase, allowing multiple goals configured for the same phase to reuse
the same ClassFinder. It also removes the need to instantiate a ClassFinder
solely for Hilla class checks, reducing the number of scans performed during
the build.

Fixes #19616
Fixes #19009
Fixes #20385
mcollovati added a commit that referenced this pull request Nov 20, 2024
Run Flow mojos using an isolated class loader that includes both project and
plugin dependencies, with project dependencies taking precedence. This ensures
that classes are always loaded from the same class loader at runtime, preventing
errors where a class might be loaded by the plugin's class loader while one of
its parent classes is only available in the project’s class loader (see #19616).

Additionally, this approach prevents the retrieval of resources from plugin
dependencies when the same artifact is defined within the project (see #19009).

This refactoring also introduces caching for ClassFinder instances per
execution phase, allowing multiple goals configured for the same phase to reuse
the same ClassFinder. It also removes the need to instantiate a ClassFinder
solely for Hilla class checks, reducing the number of scans performed during
the build.

Fixes #19616
Fixes #19009
Fixes #20385
mcollovati added a commit to vaadin/hilla that referenced this pull request Nov 20, 2024
ZheSun88 added a commit to vaadin/hilla that referenced this pull request Nov 20, 2024
Refactoring required by changes in vaadin/flow#20465.

Co-authored-by: Zhe Sun <[email protected]>
vaadin-bot pushed a commit to vaadin/hilla that referenced this pull request Nov 20, 2024
Refactoring required by changes in vaadin/flow#20465.

Co-authored-by: Zhe Sun <[email protected]>
vaadin-bot pushed a commit to vaadin/hilla that referenced this pull request Nov 20, 2024
Refactoring required by changes in vaadin/flow#20465.

Co-authored-by: Zhe Sun <[email protected]>
taefi pushed a commit to vaadin/hilla that referenced this pull request Nov 20, 2024
…2912)

test: refactor mock setup in maven plugin tests (#2911)

Refactoring required by changes in vaadin/flow#20465.

Co-authored-by: Marco Collovati <[email protected]>
Co-authored-by: Zhe Sun <[email protected]>
taefi added a commit to vaadin/hilla that referenced this pull request Nov 20, 2024
…2913)

test: refactor mock setup in maven plugin tests (#2911)

Refactoring required by changes in vaadin/flow#20465.

Co-authored-by: Marco Collovati <[email protected]>
Co-authored-by: Zhe Sun <[email protected]>
Co-authored-by: Soroosh Taefi <[email protected]>
mcollovati added a commit that referenced this pull request Nov 20, 2024
Run Flow mojos using an isolated class loader that includes both project and
plugin dependencies, with project dependencies taking precedence. This ensures
that classes are always loaded from the same class loader at runtime, preventing
errors where a class might be loaded by the plugin's class loader while one of
its parent classes is only available in the project’s class loader (see #19616).

Additionally, this approach prevents the retrieval of resources from plugin
dependencies when the same artifact is defined within the project (see #19009).

This refactoring also introduces caching for ClassFinder instances per
execution phase, allowing multiple goals configured for the same phase to reuse
the same ClassFinder. It also removes the need to instantiate a ClassFinder
solely for Hilla class checks, reducing the number of scans performed during
the build.

Fixes #19616
Fixes #19009
Fixes #20385
mcollovati added a commit that referenced this pull request Nov 20, 2024
Run Flow mojos using an isolated class loader that includes both project and
plugin dependencies, with project dependencies taking precedence. This ensures
that classes are always loaded from the same class loader at runtime, preventing
errors where a class might be loaded by the plugin's class loader while one of
its parent classes is only available in the project’s class loader (see #19616).

Additionally, this approach prevents the retrieval of resources from plugin
dependencies when the same artifact is defined within the project (see #19009).

This refactoring also introduces caching for ClassFinder instances per
execution phase, allowing multiple goals configured for the same phase to reuse
the same ClassFinder. It also removes the need to instantiate a ClassFinder
solely for Hilla class checks, reducing the number of scans performed during
the build.

Fixes #19616
Fixes #19009
Fixes #20385
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants