You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@maven.apache.org by GitBox <gi...@apache.org> on 2022/01/27 08:59:55 UTC

[GitHub] [maven-release] cstamas opened a new pull request #100: Handle edge case for m-p-p

cstamas opened a new pull request #100:
URL: https://github.com/apache/maven-release/pull/100


   Seems m-release-p with all it's logic does not
   cover an obvious edge case: to build a maven-plugin
   (any, but think just about building m-p-p itself),
   you MUST use other version (a released, other than current
   project), that MAY be define as a property/inherited/etc.
   
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [maven-release] cstamas edited a comment on pull request #100: Handle edge case for m-p-p

Posted by GitBox <gi...@apache.org>.
cstamas edited a comment on pull request #100:
URL: https://github.com/apache/maven-release/pull/100#issuecomment-1025186600


   > AFAIK, many maven-plugin-plugin releases were done until now using maven-release-plugin, so I don't see what you are trying to "fix"
   
   Wrong. Let me fix your statement: "many maven-plugin-plugin releases were done until now using maven-release-plugin **thanks to workarounds and hacks**". And this stands not only for m-p-p but for others as well like m-surefire-p.
   
   The signs that "something is wrong":
   * https://github.com/apache/maven-plugin-tools/blob/master/maven-plugin-plugin/pom.xml#L274
   * https://github.com/apache/maven-plugin-tools/blob/master/maven-plugin-plugin/pom.xml#L299
   * https://github.com/apache/maven-surefire/blob/master/maven-surefire-plugin/pom.xml#L94
   and so on...
   
   I hope we both agree that in case of 2 (or more) plugins refs for same version is "best practice" to define the plugin version as property? And it is the m-resolver-p (according to all these "comments" above) that prevents it, hence, it forces you to work AGAINST best practice, that IMHO is worth fixing. Or in short, your statement is correct due all these sprinkled workarounds (devs forced to abandon best practices and leaving notes in POMs for future).


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [maven-release] cstamas commented on pull request #100: Handle edge case for m-p-p

Posted by GitBox <gi...@apache.org>.
cstamas commented on pull request #100:
URL: https://github.com/apache/maven-release/pull/100#issuecomment-1023953381


   To recap: I really have no idea WHAT and WHY m-release-p does what it does (tampering with version placeholders of plugins/deps having GA same as currently built project), can anyone shed any light on this?


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [maven-release] hboutemy commented on pull request #100: Handle edge case for m-p-p

Posted by GitBox <gi...@apache.org>.
hboutemy commented on pull request #100:
URL: https://github.com/apache/maven-release/pull/100#issuecomment-1025195141


   I don't like the habit people have at defining property for plugin version, no, but it's a habit that has been taken by many.
   
   hard-coding some plugin names in maven-release-plugin source code looks as much as a hack as the hacks you're trying to avoid: this is what made me start the discussion, because I think we don't yet get what the real issue is
   
   it seems the issue is when the current released project requires to use a previous version
   
   I must confess that testing maven-release-plugin is hard, and doing unit or integration tests = what I'd like to see before fixing a not clearly defined issue (with hard-coded values that make me uncomfortable)


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [maven-release] cstamas edited a comment on pull request #100: Handle edge case for m-p-p

Posted by GitBox <gi...@apache.org>.
cstamas edited a comment on pull request #100:
URL: https://github.com/apache/maven-release/pull/100#issuecomment-1023946463


   @hboutemy so, when you build a `maven-plugin` you MUST use m-p-p to build it. Moreover, when you build m-p-p itself, you MUST use m-p-p to build it. 
   
   OTOH, in a bit more complex (or repeated declarations, like buildPlugin + reportPlugin) use of a plugin, it is "best practice" to define it's version using property, and use `${placeholder}` for m-p-p plugin (and corresponding report plugin) definition in POM. BUT, m-release-p tries to resolve that placeholder, and comes to these IF/ELSE branches that FAILS the build.
   
   As I see, m-release-p tampers properties if it finds ANYTHING that has GA of "this" project (currently released), and does some extra work with placeholders, BUT at the same time, it misses a lot, like inherited properties (at least comments suggests that) etc. All this prevents building m-p-p IF m-p-p used to build it has version defined as property.
   
   If you generalize this, you get the MRELEASE-799 where GA of currently built project is used with other version as dep/plugin (like they do for some sort of testing).


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [maven-release] hboutemy edited a comment on pull request #100: Handle edge case for m-p-p

Posted by GitBox <gi...@apache.org>.
hboutemy edited a comment on pull request #100:
URL: https://github.com/apache/maven-release/pull/100#issuecomment-1023941068


   I don't get why m-plugin-p or m-surefire-p would need to be an edge case known from m-release-p: please explicit


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [maven-release] cstamas commented on pull request #100: Handle edge case for m-p-p

Posted by GitBox <gi...@apache.org>.
cstamas commented on pull request #100:
URL: https://github.com/apache/maven-release/pull/100#issuecomment-1025200573


   Sometime it's unavoidable, like when you want to use same (prev) version in build/plugins and report/plugins for example.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [maven-release] slawekjaranowski commented on pull request #100: Handle edge case for m-p-p

Posted by GitBox <gi...@apache.org>.
slawekjaranowski commented on pull request #100:
URL: https://github.com/apache/maven-release/pull/100#issuecomment-1022998714


   looks like similar case for surefire ...
   https://github.com/apache/maven-surefire/blob/master/pom.xml#L468-L470


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [maven-release] cstamas closed pull request #100: Handle edge case for m-p-p

Posted by GitBox <gi...@apache.org>.
cstamas closed pull request #100:
URL: https://github.com/apache/maven-release/pull/100


   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [maven-release] cstamas commented on pull request #100: Handle edge case for m-p-p

Posted by GitBox <gi...@apache.org>.
cstamas commented on pull request #100:
URL: https://github.com/apache/maven-release/pull/100#issuecomment-1023946463


   @hboutemy so, when you build a `maven-plugin` you MUST use m-p-p to build it. Moreover, when you build m-p-p itself, you MUST use m-p-p to build it. 
   
   OTOH, in a bit more complex (or repeated declarations, like buildPlugin + reportPlugin) use of a plugin, it is "best practice" to define it's version using property, and use `${placeholder}` for m-p-p plugin (and corresponding report plugin) definition in POM. BUT, m-release-p tries to resolve that placeholder, and comes to these IF/ELSE branches that FAILS the build.
   
   As |I see, m-release-p tampers properties if it finds ANYTHING that has GA of "this" project (currently released), and does some extra work with placeholders, BUT at the same time, it misses a lot, like inherited properties (at least comments suggests that) etc. All this prevents building m-p-p IF m-p-p used to build it has version defined as property.
   
   If you generalize this, you get the MRELEASE-799 where GA of currently built project is used with other version as dep/plugin (like they do for some sort of testing).


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [maven-release] cstamas commented on pull request #100: Handle edge case for m-p-p

Posted by GitBox <gi...@apache.org>.
cstamas commented on pull request #100:
URL: https://github.com/apache/maven-release/pull/100#issuecomment-1023951291


   > I don't get why m-plugin-p or m-surefire-p would need to be an edge case known from m-release-p: please explicit
   
   So, just like "to build m-p-p you need m-p-p", to test "m-surefire-p you need m-surefire-p" and in a moment you define used m-surefire-p version as property, m-release-p explodes.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [maven-release] hboutemy commented on pull request #100: Handle edge case for m-p-p

Posted by GitBox <gi...@apache.org>.
hboutemy commented on pull request #100:
URL: https://github.com/apache/maven-release/pull/100#issuecomment-1025183774


   AFAIK, many maven-plugin-plugin releases were done until now using maven-release-plugin, so I don't see what you are trying to fix


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [maven-release] cstamas commented on pull request #100: Handle edge case for m-p-p

Posted by GitBox <gi...@apache.org>.
cstamas commented on pull request #100:
URL: https://github.com/apache/maven-release/pull/100#issuecomment-1025430072


   Hm, in that case (report/plugin does not need a version) my use case is not needed, true... and yes, as you said, this is a "not so well known"... I assumed maven "is same" on every spot, but it seems is not.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [maven-release] cstamas commented on pull request #100: Handle edge case for m-p-p

Posted by GitBox <gi...@apache.org>.
cstamas commented on pull request #100:
URL: https://github.com/apache/maven-release/pull/100#issuecomment-1025186600


   > AFAIK, many maven-plugin-plugin releases were done until now using maven-release-plugin, so I don't see what you are trying to "fix"
   
   Wrong. Let me fix your statement: "many maven-plugin-plugin releases were done until now using maven-release-plugin thanks to workarounds and hacks". And this stands not only for m-p-p but for others as well like m-surefire-p.
   
   The signs that "something is wrong":
   * https://github.com/apache/maven-plugin-tools/blob/master/maven-plugin-plugin/pom.xml#L274
   * https://github.com/apache/maven-plugin-tools/blob/master/maven-plugin-plugin/pom.xml#L299
   * https://github.com/apache/maven-surefire/blob/master/maven-surefire-plugin/pom.xml#L94
   and so on...
   
   I hope we both agree that in case of 2 (or more) plugins refs for same version is "best practice" to define the plugin version as property? And it is the m-resolver-p (according to all these "comments" above) that prevents it, hence, it forces you to work AGAINST best practice, that IMHO is worth fixing. Or in short, your statement is correct due all these sprinkled workarounds (devs forced to abandon best practices and leaving notes in POMs for future).


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [maven-release] hboutemy edited a comment on pull request #100: Handle edge case for m-p-p

Posted by GitBox <gi...@apache.org>.
hboutemy edited a comment on pull request #100:
URL: https://github.com/apache/maven-release/pull/100#issuecomment-1025183774


   AFAIK, many maven-plugin-plugin releases were done until now using maven-release-plugin, so I don't see what you are trying to "fix"


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [maven-release] cstamas commented on pull request #100: Handle edge case for m-p-p

Posted by GitBox <gi...@apache.org>.
cstamas commented on pull request #100:
URL: https://github.com/apache/maven-release/pull/100#issuecomment-1025564946


   Pushed this commit that for now circumvents limitations of m-release-p https://github.com/apache/maven-plugin-tools/pull/66/commits/dd668dd4504b5513a39c49fb7140136942ad0a32


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [maven-release] cstamas edited a comment on pull request #100: Handle edge case for m-p-p

Posted by GitBox <gi...@apache.org>.
cstamas edited a comment on pull request #100:
URL: https://github.com/apache/maven-release/pull/100#issuecomment-1023951291


   > I don't get why m-plugin-p or m-surefire-p would need to be an edge case known from m-release-p: please explicit
   
   So, just like "to build m-p-p you need m-p-p", "to test m-surefire-p you need m-surefire-p" and in a moment you define used m-surefire-p version as property, m-release-p explodes.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [maven-release] cstamas commented on pull request #100: Handle edge case for m-p-p

Posted by GitBox <gi...@apache.org>.
cstamas commented on pull request #100:
URL: https://github.com/apache/maven-release/pull/100#issuecomment-1023947690


   Am actually really unsure what m-release-p does with placeholders and why it does that in this first place.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [maven-release] hboutemy commented on pull request #100: Handle edge case for m-p-p

Posted by GitBox <gi...@apache.org>.
hboutemy commented on pull request #100:
URL: https://github.com/apache/maven-release/pull/100#issuecomment-1025202897


   for report plugin, you don't need to define version: it is taken from plugin or pluginManagement, a feature not so well known by users from maven-reporting-exec https://maven.apache.org/shared/maven-reporting-exec/
   
   the question remain to clearly define the problem and the solution before writing a list of "edge-case" plugins in maven-release-plugin source code


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [maven-release] hboutemy commented on pull request #100: Handle edge case for m-p-p

Posted by GitBox <gi...@apache.org>.
hboutemy commented on pull request #100:
URL: https://github.com/apache/maven-release/pull/100#issuecomment-1023941068


   I don't get why m-p-p would need to be an edge case known from m-release-p: please explicit


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org