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

Remove NonStandardTagHook #416

Merged
merged 2 commits into from
Jan 30, 2023
Merged

Remove NonStandardTagHook #416

merged 2 commits into from
Jan 30, 2023

Conversation

basil
Copy link
Member

@basil basil commented Jan 27, 2023

This hook is dead code (or should be, at least) because the common case is for the SCM section of pom.xml to be accurate to avoid having to guess at the tag. BOM test passed, so I think this is likely safe.

basil added a commit to basil/bom that referenced this pull request Jan 27, 2023
@basil basil marked this pull request as ready for review January 27, 2023 04:54
@basil basil requested a review from a team as a code owner January 27, 2023 04:54
@basil basil requested a review from jtnord January 27, 2023 04:54
@jtnord
Copy link
Member

jtnord commented Jan 27, 2023

because ... SCM section of pom.xml to be accurate to avoid having to guess at the tag.

not sure I fully follow this. the tag for sure should be in the SCM and should be correct - but to get the tag you need the release pom...
(as it is for the case of -m-r-p) transformed as part of the release.

I am also not sure why anything is guessing - the information for m-r-p and JEP-229 releases is already in the HPI which we have access to (embedded in the war or via the remote g:a:v?

I have no idea what is going on with electric-flow I'm taking an educated guess that this removal will break that - but it should be possible to write a hook for this specific plugin so I am investigating that, additionally the maintainers have agreed to switch to using the standard tag for future releases, so if that can happen soon there may not even be a need for me to investigate the private hook.

@basil
Copy link
Member Author

basil commented Jan 27, 2023

not sure I fully follow this

Looks like your proprietary test passed, including for electricflow, so not sure I fully follow why this change isn't approved yet. Like a lot of things in this codebase, I never hit the code when I set a breakpoint so I figured it was probably dead code.

@basil
Copy link
Member Author

basil commented Jan 27, 2023

not sure I fully follow this. the tag for sure should be in the SCM and should be correct - but to get the tag you need the release pom...

I finally figured out how this works after a lot of stepping in the debugger. Apparently in WAR mode it reads the MANIFEST.MF for each plugin to get the group ID, artifact ID, version, and dependencies. Then it later reads pom.xml from each HPI's META-INF/maven/pom.xml to get the SCM URL. 😄

@basil
Copy link
Member Author

basil commented Jan 27, 2023

(And yes, in case you're wondering, reading pom.xml is completely unnecessary because maven-hpi-plugin already puts the SCM URL in MANIFEST.MF as Plugin-ScmUrl: one of the many inefficiencies I've discovered today.)

@jtnord
Copy link
Member

jtnord commented Jan 30, 2023

because maven-hpi-plugin already puts the SCM URL in MANIFEST.MF as Plugin-ScmUrl: one of the many inefficiencies I've discovered today.)

The scm.url which is what the hpi-plugin uses is not intended to be a URL for checkout/cloning but for browsing by humans. For GitHub we are mostly OK (with some mangling of the URL which exists) - but this is not always the case. (additionally it may not actually be set remotely correctly by plugins as it is not required to be overridden by children) - something that can be fixed over time (but we still need the tag which is in the pom.xml unless we want to go guessing - which I do not think is something we want)

@basil basil merged commit 2db71d8 into master Jan 30, 2023
@basil basil deleted the nonstandardplugins branch January 30, 2023 19:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants