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 core export provider #764

Merged
merged 2 commits into from
Jan 17, 2023
Merged

Fix core export provider #764

merged 2 commits into from
Jan 17, 2023

Conversation

oehme
Copy link
Contributor

@oehme oehme commented Jan 5, 2023

Since apache/maven#616, the default CoreExportProvider no longer uses the provided CoreExports, but instead tries (and fails) to discover them itself.

This change fixes that by providing our own custom instance of CoreExportProvider. This allows core extension to contribute exported artifacts and exported packages again, like it used to do before the Maven 4.x upgrade.

Since apache/maven#616, the default
CoreExportProvider no longer uses the provided CoreExports,
but instead tries (and fails) to discover them itself.

This change fixes that by providing our own custom instance
of CoreExportProvider. This allows core extension to contribute
exported artifacts and exported packages again, like it used to
do before the Maven 4.x upgrade.
@oehme
Copy link
Contributor Author

oehme commented Jan 5, 2023

The underlying reason why the default logic doesn't work is because the discovery method calls ClassRealm.loadResourcesFromImport, which does not handle META-INF entries from imported realms. It looks up the name META-INF/maven/extension.xml and matches that against the import realms. The matching logic only accounts for classes/resources in packages, but doesn't account for META-INF. So it never never finds the extension.xml entries.

maven-embedder has the same problem now.

Really, loadResourcesFromImport is broken. It should iterate over all imports and try to load the resources from each of them, rather than trying to find a single one. This is a performance optimization that has gone too far and breaks correctness. But given that this bug and the workaround in MavenCli has existed for so long, I've opted to provide a quick fix to unblock further testing of the Maven daemon.

@ppalaga
Copy link
Contributor

ppalaga commented Jan 6, 2023

I have resterted the ubuntu-18.04 CI job. Not sure why it has shown up as cancelled.

@gnodet
Copy link
Contributor

gnodet commented Jan 9, 2023

@oehme thx for the detailed explanation. Would it be possible to add an IT to test this feature ?

@oehme
Copy link
Contributor Author

oehme commented Jan 11, 2023

Yes, I'll add one

@oehme
Copy link
Contributor Author

oehme commented Jan 11, 2023

@gnodet The test infrastructure assumes that every test case is a single project, but for my case I had to install the extension first before using it in the consuming build. I hacked around this by adding the project dir as an additional parameter, but I'd love suggestions for a more elegant solution.

@ppalaga
Copy link
Contributor

ppalaga commented Jan 12, 2023

The test infrastructure assumes that every test case is a single project, but for my case I had to install the extension first before using it in the consuming build. I hacked around this by adding the project dir as an additional parameter, but I'd love suggestions for a more elegant solution.

I have left a suggestion inline.

@oehme
Copy link
Contributor Author

oehme commented Jan 17, 2023

@ppalaga this should be good to go :)

Copy link
Contributor

@ppalaga ppalaga left a comment

Choose a reason for hiding this comment

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

I'll let @gnodet re-review and merge.

@oehme
Copy link
Contributor Author

oehme commented Jan 17, 2023

Would it be possible to get an m2 for this? I'd love to run our extensions full CI build against this to make sure we work properly when 1.0 comes out.

@ppalaga
Copy link
Contributor

ppalaga commented Jan 17, 2023

We actually used to publish the binaries produced by the given PR's CI run via Checks -> Artifacts
image

Let me check why they are not there anymore.

@gnodet
Copy link
Contributor

gnodet commented Jan 17, 2023

Would it be possible to get an m2 for this? I'd love to run our extensions full CI build against this to make sure we work properly when 1.0 comes out.

Yes, though I'd like to have a maven 4.0.0-alpha-4 release and then release mvnd 1.0.0-m2.

@ppalaga
Copy link
Contributor

ppalaga commented Jan 17, 2023

Here is a fix for GH Actions not publishing the PR's binaries #774

@gnodet gnodet merged commit 74df4b6 into apache:master Jan 17, 2023
@gnodet gnodet added this to the 1.0.0-m2 milestone Jan 17, 2023
@ppalaga
Copy link
Contributor

ppalaga commented Jan 18, 2023

@oehme, BTW if you now send some dummy PR, it will publish the distribution zips for 3 (of 4, excl. Apple M1) supported platforms under Checks > Artifacts. You could perhaps use those to test on your side?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants