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

[MNG-8072] add ConsumerPomFile methods #1437

Open
wants to merge 1 commit into
base: maven-3.9.x
Choose a base branch
from

Conversation

laeubi
Copy link

@laeubi laeubi commented Mar 10, 2024

Currently there is some vague definition of a "build pom" versus a "consumer pom":

there are also some plugins around to work with this e.g.

but there are some issues:

  1. Maven makes some assumptions about its "file", e.g setting the file
  2. changes the basedir
  3. Even though there is a setPomFile method that do not change the
  4. basedir e.g. a parent ref by default would be broken
  5. once we change the file, there is a mismatch between the pom on disk
  6. and the model
  7. it is not possible for a plugin to know the original file and the
  8. consumer file, e.g. if I want to deploy the original file e.g. with classifier "pom-build"
  9. because of this usually "consumer pom plugins" generate the the new
  10. pom to a new file in the project root, where it actually not belongs to and leaves the file even after mvn clean if no special actions are taken.

This enhance the MavenProject with one more method set/getConsumerPom that fills this gap, then plugins working on the consumer pom can set the file there and where it then is used by other plugins.

Following this checklist to help us incorporate your
contribution quickly and easily:

  • Make sure there is a JIRA issue filed
    for the change (usually before you start working on it). Trivial changes like typos do not
    require a JIRA issue. Your pull request should address just this issue, without
    pulling in other changes.
  • Each commit in the pull request should have a meaningful subject line and body.
  • Format the pull request title like [MNG-XXX] SUMMARY,
    where you replace MNG-XXX and SUMMARY with the appropriate JIRA issue.
  • Also format the first line of the commit message like [MNG-XXX] SUMMARY.
    Best practice is to use the JIRA issue title in both the pull request title and in the first line of the commit message.
  • Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
  • Run mvn clean verify to make sure basic checks pass. A more thorough check will
    be performed on your pull request automatically.
  • You have run the Core IT successfully.

If your pull request is about ~20 lines of code you don't need to sign an
Individual Contributor License Agreement if you are unsure
please ask on the developers list.

To make clear that you license your contribution under
the Apache License Version 2.0, January 2004
you have to acknowledge this by using the following check-box.

Currently there is some vague definition of a "build pom" versus a
"consumer pom":

    https://maven.apache.org/studies/consumer-pom/
    https://cwiki.apache.org/confluence/display/MAVEN/Build+vs+Consumer+POM

there are also some plugins around to work with this e.g.

    https://tycho.eclipseprojects.io/doc/latest/tycho-packaging-plugin/update-consumer-pom-mojo.html
    https://www.mojohaus.org/flatten-maven-plugin/plugin-info.html

but there are some issues:

    Maven makes some assumptions about its "file", e.g setting the file
changes the basedir
    Even though there is a setPomFile method that do not change the
basedir e.g. a parent ref by default would be broken
    once we change the file, there is a mismatch between the pom on disk
and the model
    it is not possible for a plugin to know the original file and the
consumer file, e.g. if I want to deploy the original file e.g. with
classifier "pom-build"
    because of this usually "consumer pom plugins" generate the the new
pom to a new file in the project root, where it actually not belongs to
and leaves the file even after mvn clean if no special actions are
taken.

This enhance the MavenProject with one more method set/getConsumerPom
that fills this gap, then plugins working on the consumer pom can set
the file there and where it then is used by other plugins.
@rmannibucau
Copy link
Contributor

Hi @laeubi , wonder how consistent it is with https://github.com/apache/maven/blob/master/maven-core/src/main/java/org/apache/maven/internal/transformation/impl/DefaultConsumerPomArtifactTransformer.java#L94, shouldn't it hook there? We should also ensure the API is clearly defined for that part if we enable the consumer pom to not be handled automatically - guess we can let it be automatic for now since you can decorate the transformer but not 100% sure of the use case - typically flatten does not need this new API.

@laeubi
Copy link
Author

laeubi commented Mar 10, 2024

I currently only looking at 3.9.x where I can't see this is used right now?

Also here the problem is that it only ever will allow one transformer, iterative transformation is not possible and I can't access the artifact / file until it is finalized already.

And integration would be quite trivial, e.g in this case simply replace project.getFile() by project.getConsumerPomFile() and you are done :-)

@rmannibucau
Copy link
Contributor

I currently only looking at 3.9.x where I can't see this is used right now?

We should always start from master otherwise the method will be lost in a coming version and will be pointless for almost all users I fear.

Also here the problem is that it only ever will allow one transformer, iterative transformation is not possible and I can't access the artifact / file until it is finalized already.

Guess it is a matter of extension wiring but ultimately we can discuss moving the transformation but doing it before limits the side effects of the new models which is probably good but my point was more to not add a new consumerPom file which is not about consumer pom and just yet another attached artifact - we already have this feature without this new method.

simply replace project.getFile() by project.getConsumerPomFile() and you are done :-)

Waiting your PR on all existing plugins using getFile() ;). Switching a method is not that trivial there. For example you break flatten plugin.

@laeubi
Copy link
Author

laeubi commented Mar 10, 2024

but my point was more to not add a new consumerPom file which is not about consumer pom and just yet another attached artifact - we already have this feature without this new method.

But you can't modify an artifact (or should not) + there is no way for a plugin to participate as part of the mojo-execution with an extension but this is required as usually you want ti to be configured and you want to control order, e.g. usually one want maybe first tycho inject some things, then clear others with flatten-maven, maybe then finally something what maven do by default....

Waiting your PR on all existing plugins using getFile() ;). Switching a method is not that trivial there. For example you break flatten plugin.

It won't break anything ... plugins not supporting this will simply never set this and then getConsumerPom() will return the "traditional" file like today. Also not "all" plugins need to be adjusted, just these that care about build vs consumer pom...

@rmannibucau
Copy link
Contributor

there is no way for a plugin to participate as part of the mojo-execution

Yes, this is why I mentionned we can need to revisit a but the current API. Once again my main concern was that the "consumerPom" API has no link with consumer pom at all and we must converge to maven 4 (even if we backport to 3.9) cause otherwise your API will be dead, even in 3.x when we'll backport things.
Ultimately it looks more - in v3 world - to attached artifacts so it is doable without the new API to me - short term.
Long term we need to find a better lifecycle but can need some list discussion?

It won't break anything ... plugins not supporting this will simply never set this and then getConsumerPom() will return the "traditional" file like today. Also not "all" plugins need to be adjusted, just these that care about build vs consumer pom...

exactly so if you do you are no more compatible with the chain you describe earlier since the right pom will not be consumed downside so we need to be vigilant and see if it is an issue to break all the existing ecosystem or if we don't care plugins using this new (to be defined) API will not be compatible with existing plugins.

@laeubi
Copy link
Author

laeubi commented Mar 10, 2024

Whatever we choose "will not be compatible with existing plugins", but without anything offered no plugin can be adjusted so we are in an endless cycle...

Also feel free to propose an alternative I just want:

  1. simple and 3.9.x compatible I don't want to wait for glorious future of Maven 4 where everything will be perfect an clean
  2. must work incrementally (e.g. plugin A can create an initial consumer pom and plugin B can further enhance/adjust that)
  3. no need for extensions, delegations, side effects (e.g. modify the original pom location is an undesired side-effect)

@rmannibucau
Copy link
Contributor

  1. for me it is the promise whatever great you do you will not be able to use it so "-1" (dont see it as a blocker more as a "you can but dont cry later") to work on anything not on master
  2. think it is thinking wrong, consumer pom should be automatically provided by the transformation of the transformed pom which guarantee the v4.0.0 compat - if it is incremental we loose that. So I think we should work on the native model then let the consumer pom be automatically handled
  3. +1

@laeubi
Copy link
Author

laeubi commented Mar 10, 2024

  1. You should not assume maven-core(-plugins) inability to adapt to never features to everyone, e.g. Tycho always uses latest maven and latest features
  2. "by default" Maven can't know what is desired for a consumer pom, and different users have different demands. It might can do some additional transformations (e.g. map 'new' constructs > 'old' constructs) but for example the initial model for Tycho is almost empty, then we will it dynamically of course not reflected in file as we don't want to dirty the build files of the user, what even maybe not exits at all as it is a pomless module... then tycho-update-consumer-pom performs some transformation (e.g. mapping p2 > real maven coordinates) and writes a new file, I can't see how maven could ever handle this automatically given that..
  3. ... where current solution has undesired side-effect and only looks at project.getFile() ....

@rmannibucau
Copy link
Contributor

You should not assume maven-core(-plugins) inability to adapt to never features to everyone, e.g. Tycho always uses latest maven and latest features

I didn't at all, even the opposite if you read carefully. I just highlight you create a concurrent feature of something existing in latest version of the project - indeed it needs a few adjustment but it does not need the proposed PR.

"by default" Maven can't know what is desired for a consumer pom

Then you don't speak about consumer pom which is well defined as a consummable pom by downstream builds (v4.0.0), anything else is not consumer pom and does not belong to core IMO - I suspect even if not sure you want some tycho meta but this is not about consumer pom IMHO, potentially main pom but consumer has another intent.

I can't see how maven could ever handle this automatically given that..

Consumer pom is built form the model, if you fill the model you get a valid consumer pom.

where current solution has undesired side-effect and only looks at project.getFile()

In maven 3 there isnt really a notion of consumer pom (we had some proto but no real API) so it is mainly about a single v4.0.0 pom you were able to mutate. Indeed it has side effects but similarly your consumer pom will have exactly the same since it has the exact same design and all plugins will not always agree on that (current pitfall).
This is why it is important to use the model IMHO and let the tranformer create a valid shareable and backward compat consumer pom.

@laeubi
Copy link
Author

laeubi commented Mar 10, 2024

Consumer pom is built form the model, if you fill the model you get a valid consumer pom.

Just look here:

and here:

and finally here:

for me it looks like it is reading this file, not the (in memory) pom ...

Then you don't speak about consumer pom which is well defined as a consummable pom by downstream builds (v4.0.0), anything else is not consumer pom and does not belong to core IMO - I suspect even if not sure you want some tycho meta but this is not about consumer pom IMHO, potentially main pom but consumer has another intent.

I'm defiantly talking about consumer pom, because consumers ( == plain maven) wants to have a pom that they can consume without Tycho (Tycho itself don't need a physical pom.xml). So if you build some jar with Tycho you want it to be consumable after you deployed it to central by others (non Tycho users) like a traditionally hand written pom.xml

@rmannibucau
Copy link
Contributor

for me it looks like it is reading this file, not the (in memory) pom ...

Right, it is an issue for any plugin contirbuting dependencies but we can solve it easily keeping a single consumer pom logic instead of duplicating it.

So if you build some jar with Tycho you want it to be consumable after you deployed it to central by others (non Tycho users) like a traditionally hand written pom.xml

Same than if you build with model v > 4.0.0 you want to be consumed by v4.0.0 consumers (older maven, coursier, gradle, plain ivy etc), so the same solution should converge IMHO.

@laeubi
Copy link
Author

laeubi commented Mar 10, 2024

That's why I started this PR, it maybe could be solved, but it currently is not solved ... so as soon as there is a solution this is probably obsolete ;-)

@elharo elharo changed the title MNG-8072 - add ConsumerPomFile methods [MNG-8072] add ConsumerPomFile methods Mar 18, 2024
Copy link
Contributor

@elharo elharo left a comment

Choose a reason for hiding this comment

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

needs tests

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.

3 participants