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

[MRELEASE-1154] [REGRESSION] MRELEASE-1109 breaks release of Maven Su… #229

Closed
wants to merge 1 commit into from

Conversation

michael-o
Copy link
Member

…refire

This reverts commit 9e0713b (MRELEASE-1109).

This closes #229

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 [MRELEASE-XXX] - Fixes bug in ApproximateQuantiles,
    where you replace MRELEASE-XXX with the appropriate JIRA issue. Best practice
    is to use the JIRA issue title in 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 -Prun-its to make sure basic checks pass. A more thorough check will
    be performed on your pull request automatically.

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.

@michael-o michael-o requested review from cstamas and kwin August 23, 2024 10:09
* All Maven properties allowed to be referenced in parent versions via expressions
* @see <a href="https://maven.apache.org/maven-ci-friendly.html">CI-Friendly Versions</a>
*/
private static final List<String> CI_FRIENDLY_PROPERTIES = Arrays.asList("revision", "sha1", "changelist");
Copy link

Choose a reason for hiding this comment

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

Be aware that the list of supported properties is extensible

}
}
} else {
if (CI_FRIENDLY_PROPERTIES.contains(property)) {
Copy link

Choose a reason for hiding this comment

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

More than one property is supported here!

@michael-o
Copy link
Member Author

@laeubi You know that this is a revert?

@laeubi
Copy link

laeubi commented Aug 27, 2024

@laeubi You know that this is a revert?

Yes I just wanted to point out problematic things in the current (now reverted) implementation that shows it is either incomplete i not even wrong in its current form.

@kwin
Copy link
Member

kwin commented Aug 27, 2024

Any further insights what exactly causes the issue? https://issues.apache.org/jira/browse/MRELEASE-1154 does not even contain the full stack trace....

@michael-o
Copy link
Member Author

Any further insights what exactly causes the issue? https://issues.apache.org/jira/browse/MRELEASE-1154 does not even contain the full stack trace....

Just try to release Surefire, it will immediately fail.

@michael-o
Copy link
Member Author

@gnodet Since you are also on the CI friendy track. Which PR do you prefer? This one or #230? I believe that what we have in Release now is incomplete/broken, I'd prefer to revert it until we have something better. WDYT?

@gnodet
Copy link

gnodet commented Aug 31, 2024

@gnodet Since you are also on the CI friendy track. Which PR do you prefer? This one or #230? I believe that what we have in Release now is incomplete/broken, I'd prefer to revert it until we have something better. WDYT?

This is definitely open to discussion, but I'm thinking right now is that friendly should be static after the file models have been read. So CI-friendly versions can be handled by a transformer that would be called between the file model being read and the file model being validated.
In addition, I think the default implementation should limit to user properties, but not use project properties. The problem with project properties is that you only have the real values from the effective model, but the file model. And that's really what causes all kind of problems in Maven, as the CI version is actually handled during the file -> raw transformation.
If we go that way, that would mean the best location for those properties would be [root]/.mvn/maven.properties. The release plugin would have to be adapted, but given the model validation would ensure the versions are constants in the file model, I would not necessarily restrict the release plugin to any kind of key for substitution. However, don't really see how it could cope with something like the Jenkins one: ${revision}${changelist}... which placeholder should be updated ?

@michael-o could you briefly describe the two alternatives here ?

@michael-o
Copy link
Member Author

@michael-o could you briefly describe the two alternatives here ?

It all started with: https://issues.apache.org/jira/browse/MRELEASE-1109 which causes https://issues.apache.org/jira/browse/MRELEASE-1151 and led to https://issues.apache.org/jira/browse/MRELEASE-1153. Now, I have discovered yet another regression https://issues.apache.org/jira/browse/MRELEASE-1154 which requires https://issues.apache.org/jira/browse/MRELEASE-1109 to reverted altogether because I was not able to release Maven Surefire. The alternative in #230 provided by @kwin simply warns about non-resolvable properties. I am not really sure whether this is the right way. I know for sure that the revert will restore the previous behavior, but will also the incomplete CI friendly support. I prefer no support rather than incomplete one.

@laeubi
Copy link

laeubi commented Aug 31, 2024

however, don't really see how it could cope with something like the Jenkins one: ${revision}${changelist}... which placeholder should be updated ?

Tycho version plugin supports updating such properties with a little restriction that only one of them can be updated (usually the last part) that has to be a Number by default.

e.g. if ${revision}${changelist} maps to <revision>1.0</revision> and <changelist>-SNAPSHOT</changelist> and one updates the version to 1.1-SNAPSHOT it first checks the changelist is a suffix of the new version then leave this alone, now check revision what is not a suffix anymore so change revision -> 1.1, this already covers the most usual cases.

@gnodet
Copy link

gnodet commented Aug 31, 2024

however, don't really see how it could cope with something like the Jenkins one: ${revision}${changelist}... which placeholder should be updated ?

Tycho version plugin supports updating such properties with a little restriction that only one of them can be updated (usually the last part) that has to be a Number by default.

e.g. if ${revision}${changelist} maps to <revision>1.0</revision> and <changelist>-SNAPSHOT</changelist> and one updates the version to 1.1-SNAPSHOT it first checks the changelist is a suffix of the new version then leave this alone, now check revision what is not a suffix anymore so change revision -> 1.1, this already covers the most usual cases.

What's the use case for using multiple placeholders such as ${revision}${changelist}, especially when/if we open those properties a bit more to any user defined properties. If I look at the Jenkins use case, revision is the usual version, and changelist is -SNAPSHOT, unless it's an automatic deploy in which case changelist is computed from the git revision.
So the real use case afaik, and I think Tycho has a similar requirement, is that the version for a given component is computed at runtime and not hardcoded in the POM.
I don't think there's a real need to support two components here. What's the benefit here ?

@laeubi
Copy link

laeubi commented Aug 31, 2024

What's the use case for using multiple placeholders such as ${revision}${changelist}

If you want to have multi part version where you can change each part individually see

https://maven.apache.org/maven-ci-friendly.html --> "f you like to have more flexibility you can use a combination of the different properties like this"

And project properties itself are also not allowed to contain any other things but need to be defined either in the parent or directly in the pom.

So the real use case afaik, and I think Tycho has a similar requirement, is that the version for a given component is computed at runtime and not hardcoded in the POM

Not really at runtime, but can be set "from the outside" as for Jenkins so if you have a CI/CD-Pipeline that you don't need to change versions all time (in the SCM) but still can make sure they are unique to a build.

I don't think there's a real need to support two components here. What's the benefit here ?

See above the benefit is that one can have multipart versions and each part can be given on the commandline and takes precedence over everything else regardless of lookup-order.

@kwin
Copy link
Member

kwin commented Aug 31, 2024

There is lots of issues mixed here in the discussion. Although MRELEASE-1109 only supports single expressions this can be extended innsome follow up PR. However I fail to see how not supporting even simple ci-friendly versions is better than supporting only simple versions. The regression regarding unresolveable properties is fixed by #230. I am happy to process any constructive feedback to #230. Just reverting is IMHO the worst solution.

@michael-o
Copy link
Member Author

I'll get back to this next week.

@michael-o
Copy link
Member Author

The alternative has been merged now.

@michael-o michael-o closed this Oct 1, 2024
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.

4 participants