Ci friendly #77

lasselindqvist · 2018-09-23T18:36:56Z

Here is one possible implementation of CI friendly variable resolver.

This has some things I don't personally like. For example CiInterpolatorImpl just basically replicates StringSearchInterpolator because first I tried to extend it and overwrite just some of the methods but that lead to weird classloading issues that I didn't have the energy to solve.

Also resolving variables inside profile dependencies doesn't work as it should but that is because of the multiple other problems in profile resolving in the flatten plugin.

Now I have found out that there might be some better solutions to flattening CI variables in poms. Like https://github.com/fmarot/cifriendly-maven-plugin which seems to be a much simpler approach to solving just this one problem. I haven't tried that plugin though.

Anyway, I just wanted to leave this so that other people can access this particular solution if it is what they need in their projects.

khmarbaise · 2018-09-23T20:19:33Z

src/main/java/org/codehaus/mojo/flatten/FlattenMojo.java

-import java.util.Collection;
-import java.util.List;
-import java.util.Properties;
-
 /**


Can you please undo the reformatting...cause that covers the real changes...

Should be better now.

fmarot · 2018-09-23T21:35:21Z

Lol ! I am the author of https://github.com/fmarot/cifriendly-maven-plugin and was eagerly awaiting your solution to flatten only the version number as being part of flatten-maven-plugin would have made it the de-facto standard for Maven.
In the past I had experimented with flatten-maven-plugin and have had some "surprises" (It was a long time ago but I think it interfered with some OS dependant properties depending on the build OS) so making sure it would only interfere with one specific property was exactly what I was looking for.
Regarding my plugin the flatten/unflatten goals do the job but I do not like having to modify files before building (and the maven release plugin dislikes it even more). My plugin also include since recently a rip-off of this plugin https://github.com/jgitver/jgitver where it acts as a maven extension to substitute original poms with new ones where the version is calculated at runtime. Beware: I would not call my plugin production-ready but can improve it if need be.

While we're at it and @khmarbaise may be looking :) if I may, I think that Maven really need a standard and simple solution to "just replace the version number when it is a variable" without relying on a plugin, especially the flatten plugin that does much more than that. And goof support in the IDE (when having multiple projects) is also needed so it would help (I thnik) if this feature was in the core Maven.

Sorry to have a bit hijacked the thread...

lasselindqvist · 2018-09-24T06:49:35Z

Yes. It would be nice to have this as part of the "official" Flatten plugin as it is more likely for people to use it. So I this functionality makes it into the plugin in one way or the other.

Btw, my local build runs the new integration test fine, but the Travis CI apparently does not. I cannot see it or test it, but maybe it has the classloading issue that I got rid of locally by not extending the existing classes.

I don't currently have much time for contributing but might take a look at the profile handling in the future.

bertramn · 2018-09-26T15:35:48Z

Not sure how valid below is but if I put something like this into a module in a multi-module pom:

<properties>
  <jackson.version>2.9.6</jackson.version>
</properties>

<dependencyManagement>
  <dependencies>
    <dependency>
      <groupId>com.fasterxml.jackson</groupId>
      <artifactId>jackson-bom</artifactId>
      <version>${jackson.version}</version>
      <type>pom</type>
      <scope>import</scope>
    </dependency>
  </dependencies>
</dependencyManagement>

I get these type of errors:

[ERROR] Failed to execute goal org.codehaus.mojo:flatten-maven-plugin:1.1.0-SNAPSHOT:flatten (flatten) on project multi-module-parent: 1 problem was encountered while building the effective model for com.example:multi-module-parent:2.0.10-SNAPSHOT
[ERROR] [FATAL] Non-readable POM /Users/fred/.m2/repository/com/fasterxml/jackson/jackson-bom/${jackson.version}/jackson-bom-${jackson.version}.pom: /Users/fred/.m2/repository/com/fasterxml/jackson/jackson-bom/${jackson.version}/jackson-bom-${jackson.version}.pom (No such file or directory) @ /Users/fred/.m2/repository/com/fasterxml/jackson/jackson-bom/${jackson.version}/jackson-bom-${jackson.version}.pom

lasselindqvist · 2018-09-26T17:00:26Z

This is caused by the block
if (flattenMode == FlattenMode.resolveCiFriendliesOnly) {
defaultModelBuilder.setModelInterpolator(new CiModelInterpolator());
}
on lines 764-766 in FlattenMojo.java. It doesn't belong there and was left accidentally after testing the solution. I made a test for the parent dependency case and will publish the changes soon. Also I will make a test case for multi-module poms.

lasselindqvist · 2018-09-26T17:56:51Z

The unnecessary block was removed.
One integration test complete-multimodule-parent-pom-cifriendly was added for multi-module projects.
One integration test inherit-parent-dependencyManagement-cifriendly was added for cases where dependency versions are inherit from parents with version defined in properties.
FlattenMode had to be changed so that version is resolved instead of interpolating, otherwise the default revision property is used instead of the one given by the invoker. Also the parent block has to be resolved instead of interpolating, otherwise the revision variable in the parent version does not get resolved.

lasselindqvist · 2018-09-26T18:06:20Z

Based on the Travis CI build log I would guess that the build problem is caused by the fact that the Travis CI build does not use the invoker.properties file unlike my local build.

patrickjamesbarry · 2018-10-05T21:09:04Z

Eagerly waiting on this new feature! Thanks for putting some work behind it!

lasselindqvist · 2018-10-06T08:42:07Z

The JDK8 build seems to work now (https://travis-ci.org/mojohaus/flatten-maven-plugin/builds/437944243) when the revision is given in the maven-invoker-plugin configuration instead of invoker.properties. Maybe it is a security feature in Travis CI or a bug/feature in maven-invoker-plugin.

patrickjamesbarry · 2018-10-08T02:04:01Z

Does the ci job just need to be re-kicked? Looks like the build issue with older jdks were addressed in #75.

lasselindqvist · 2018-10-08T06:00:46Z

I will rebase this pull request once #75 has been merged to master branch and then we will get a new build.

…arent with dependencies

…on config This way of invoking works in Travis CI.

hohwille · 2018-11-15T23:52:55Z

Thanks for your work on this and all the added tests.
Could you shed some light into resolveCiFriendliesOnly.
Does Ci here stand for continuous integration? If yes, why?
I actually do not really get the point of the name resolveCiFriendliesOnly and do not think it is intuitive.
I assume you are aware that you can do create any custom config without modifying the plugin itself?

lasselindqvist · 2018-11-16T07:23:30Z

The CI friendly is a reference to https://maven.apache.org/maven-ci-friendly.html. CI does stand for continuous integration. The name could be something else, but I don't really have any better suggestions for it.
You can achieve close to the same functionality by just resolving the parent, version and dependencies, but this implementation resolves only the three ci-friendly variables and is a bit more correct if you need to have other unresolved variables in your pom.

patrickjamesbarry · 2018-11-28T13:21:10Z

Is this PR ready to merge?

lasselindqvist · 2018-12-10T18:54:29Z

@hohwille are there any intentions to merge or decline this?

deathrowe · 2018-12-13T19:08:12Z

All I want for Christmas... is for this to merge!!! 😄

hohwille

So if I got things right, you want to replace only the three variables that you predefined and hardcoded into this new flatten mode. It would also be insightful to understand more about the undesired side-effects of flatten-maven-plugin that you want to avoid with this new mode (if I got you right you were talking about system properties like maybe java.home or something that otherwise get accidentally replaced if used in POM - is that the case? Should instead then that be considered as a bug and be fixed?).

My biggest concern is that the name of the new mode and especially the 3 hardcoded variables with their names and meanings do not seem to be following a convincing design.
This might just be my personal point of view. I have no idea how many people were involved in this and came up with the "Maven CI Friendly Versions" documentation published on the maven site maybe after long discussions and smart thoughts I might not get.
I see that you want to have this feature deadly and I might seem as a stupid guy not reacting for ages and now complaining with picky reviews.
@khmarbaise / @mojohaus/owners do you agree with my point of view or am I just picky. I do not want to be a spoilspot here...

hohwille · 2018-12-13T20:40:25Z

pom.xml

@@ -219,7 +219,7 @@
              </properties>
              <goals>
                <goal>clean</goal>
-                <goal>${project.groupId}:${project.artifactId}:${project.version}:flatten</goal>
+                <goal>${project.groupId}:${project.artifactId}:${project.version}:flatten -Drevision=1.2.3.4</goal>


having a fixed revision for all tests seems kind of unflexible but that can be resolved later...

This is necessary if we want to run the integration tests in Travis. For some reason it doesn't use the ones given in invoker.properties even though in local builds they are used.
I am not sure if the problem is with Travis or the Invoker plugin.

Okay, got it. You explained that earlier. Sorry, that I did not got it directly... Considered done. If someone finds a nicer solution can still be improved after merge...

hohwille · 2018-12-13T20:42:14Z

src/it/projects/complete-multimodule-parent-pom-cifriendly/module-with-parent/pom.xml

@@ -0,0 +1,13 @@
+<project xmlns="http://maven.apache.org/POM/4.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+	xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd">
+	<modelVersion>4.0.0</modelVersion>


indendation is 2 in other existing files.

hohwille · 2018-12-13T20:45:03Z

src/it/projects/inherit-parent-dependencyManagement-cifriendly/invoker.properties

@@ -0,0 +1 @@
+invoker.mavenOpts = -Drevision=1.2.3.4


yep, this is the nice solution. Can we strip the revision system property from the plugins toplevel pom then?

Referring to the other answer, this is what I tried first and forgot to remove it.

if this is working outside travis (event with out having it in pom) please leave it here. Otherwise please cleanup.

hohwille · 2018-12-13T20:53:47Z

src/main/java/org/codehaus/mojo/flatten/FlattenMode.java

+    fatjar,
+
+    /** Only resolves variables revision, sha and changelist. Keeps everything else. */
+    resolveCiFriendliesOnly;


as discussed in the issue the name is not very good. Already resolveCiVaraiblesOnly would be a little better. Still thinking of something better. After all who defined this (https://maven.apache.org/maven-ci-friendly.html). To tell it straigth away:

the linked documentation states about sha1 you write sha here.

sha/sha1 is a particular hash algorithm. It is also already getting obsolete and is attacked. If you are naming a variable you should not use the name of the algorithm esp. if you set it as system property. How about checksum or hash?

Who came up with the name changelist? If that is for -SNAPSHOT or an empty string this is also entirely odd. A Changelist is something like this: https://en.wikipedia.org/wiki/Version_control#Common_vocabulary also it can easily be mistaken for a changelog (there is maven changes plugin and maven changelog plugin).

This is my actual concern...

I have no knowledge about the origins of the "Maven CI Friendly Versions" documentation, so I cannot answer about it. The purpose is to just resolve the variables that are stated there, but I think mostly the use case would be for "revision".
"sha" should be "sha1".
I am fine with any naming for the variable.

Just to make sure there is no misunderstanding (I am awaiting this PR): the guys at the head of Maven (@khmarbaise & al) themselves came up with 3 harcoded variable names that are possible to be accepted as variable part of the version. This is what is usually called "CI Friendly Maven". This may not be the best choice, there may have been different alternative implementations, but to this day this is the current status of Core Maven. Knowing the forward compatible history of Maven, I doubt they will change. So there is really no need to try to find alternatives here. Even non core plugin should just reuse what Maven user are (or will be) familiar with, the way of naming things (CIFriendly, sha1, changelist & revision) should be the same.
My 2 cents

@fmarot could you shed in some light here? Do you agree to some of my points or do you have some arguments for your PoV?

@lasselindqvist as you are so superb fast and active in updating the PR:
would it be possible that you link to the original documentation from the javadoc then like this:

See Maven CI Friendly for further details.

yes, each subject one after another:

resolveCiFriendliesOnly vs resolveCiVariablesOnly : I prefer the former to keep it linked with the Maven Core vocabulary of "CI Friendly". But it is not very important

sha1 VS sha in the comment : @lasselindqvist already corrected it

rename sha1 to a more generic word: this is a no go as Maven Core uses explicitly "sha1" as one of the 3 allowed keywords.

changelist : same as above. the Flatten plugin has no choice but to comply with the wording of Maven Core. Otherwise users would not understand.
(just trying to help here ;) thanks for your work both of you)

@fmarot thanks for the quick feedback. I was not aware if this is just something a single guy came up with - but if maven core already adopted this I do not want to fight anything here. I just want to care about design and quality and avoide foolish decisions ;)
@khmarbaise can you confirm this.
IMHO we could then merge this and maybe I might even be able to push out a new release before xmas.

@mfriedenhagen confirmed on dev list:
https://groups.google.com/forum/#!topic/mojohaus-dev/1SQSAERVg1c

hohwille · 2018-12-13T20:58:03Z

src/main/java/org/codehaus/mojo/flatten/cifriendly/CiInterpolatorImpl.java

+
+
+	    /**
+	     * {@inheritDoc}


check your IDE settings (Eclipse templates). {@inheritDoc} JavaDoc tag is obsolete since java 1.6 and replaced by @Override annotation.

hohwille · 2018-12-13T21:07:52Z

src/main/java/org/codehaus/mojo/flatten/cifriendly/CiInterpolatorImpl.java

+	            {
+	                result.append( input, endIdx + 1, input.length() );
+	            }
+


woho. Can't this ~120 lines of code be reduced to a view lines using regex, findFirst and replace? In the end you are only looking for 3 fixded given expressions. Also you could directly indexOf or replace for that 3 variables if they are hardcoded anyhow... Just looks overcomplicated to me. I might also be missing something. Just a suggestion, nothing that could not be accepted as is. A regular JUnit for this would be helpful and more explanatory as well...

CiInterpolatorImpl just basically replicates StringSearchInterpolator from plexus-interpolation with small changes so that it only replaces those three variables. It is ugly, but works with the same behavior as the resolving of other variables in the plugin. Using regex might detect and resolve circular and nested variables differently.

Perfectly fine then. Get the point. Maybe a link to the "copied" implementation (StringSearchInterpolator) in the javadoc ({@link package.name.StringSearchInterpolator}) with such hint would be lovely.

Change 'sha' to 'sha1' in resolveCiFriendliesOnly comment

invoker.mavenOpts = -Drevision=1.2.3.4 since Travis CI builds does not use them anyway so it is misleading

Replace tabs with 2 spaces as all the other XML files use 2 tabs as the indentation.

Add a reference to https://maven.apache.org/maven-ci-friendly.html in resolveCiFriendliesOnly documentation.

Link to StringSearchInterpolator and explain the difference to the original in the class Javadoc.

hohwille

@lasselindqvist thanks for the quick reactions and improvements to my review feedback.

As the CI friendly variables are already defined by maven and out in the wild for 2 years this seems fine to me now. Thanks to all contributing and clarifying.

hohwille · 2018-12-17T22:14:04Z

All I want for Christmas...

Here is your package. Please vote:
https://groups.google.com/forum/#!topic/mojohaus-dev/u28wlFOuSrU

Also please note that you broke the javadoc and I had to rebuild manually with doclint disabled.
Further you forgot to document the new mode in the JavaDoc of the flattenMode parameter:
http://www.mojohaus.org/flatten-maven-plugin/flatten-mojo.html#flattenMode

I was not so picky for the review to not block this any further. However, it would be lovely if you could provide a PR with these additional cleanups. In case you would do that, do not rush - its Christmas time so first enjoy being with your family. Can be done next year as well...

lasselindqvist mentioned this pull request Sep 23, 2018

Add new flattenMode called versionOnly #74

Closed

khmarbaise reviewed Sep 23, 2018

View reviewed changes

lasselindqvist force-pushed the ci-friendly branch 3 times, most recently from 02f238f to 803a1fe Compare September 24, 2018 06:37

bertramn mentioned this pull request Sep 26, 2018

Keep parent node #78

Closed

lasselindqvist force-pushed the ci-friendly branch from 803a1fe to 1df4b08 Compare September 26, 2018 17:53

lasselindqvist force-pushed the ci-friendly branch from 1234193 to c605580 Compare October 6, 2018 09:56

lasselindqvist added 4 commits November 14, 2018 19:06

Implement CI friendly resolver

d55aa74

Add integration test for CI friendly resolver

902802b

Add integration tests for multi-module project and a project with a p…

5f7e684

…arent with dependencies

Instead of invoker.properties, give parameter in the main pom executi…

2a20cdf

…on config This way of invoking works in Travis CI.

lasselindqvist force-pushed the ci-friendly branch from c605580 to 2a20cdf Compare November 14, 2018 17:06

hohwille requested changes Dec 13, 2018

View reviewed changes

Change 'sha' to 'sha1' in comment

bee984e

Change 'sha' to 'sha1' in resolveCiFriendliesOnly comment

lasselindqvist added 4 commits December 13, 2018 23:46

Remove invoker.mavenOpts

42779f3

invoker.mavenOpts = -Drevision=1.2.3.4 since Travis CI builds does not use them anyway so it is misleading

Replace tabs with 2 spaces

22a740e

Replace tabs with 2 spaces as all the other XML files use 2 tabs as the indentation.

Refer to "Maven CI Friendly" site in Javadoc

e520f8f

Add a reference to https://maven.apache.org/maven-ci-friendly.html in resolveCiFriendliesOnly documentation.

Add better Javadoc to CiInterpolatorImpl

bce0406

Link to StringSearchInterpolator and explain the difference to the original in the class Javadoc.

hohwille approved these changes Dec 14, 2018

View reviewed changes

hohwille merged commit 00e7cec into mojohaus:master Dec 14, 2018

hohwille mentioned this pull request Dec 14, 2018

handling of <version>${revision}</version> #63

Closed

hohwille added this to the 1.1.0 milestone Dec 14, 2018

hohwille mentioned this pull request Dec 17, 2018

Possibility to just flatten CI-friendly properties? #51

Closed

lasselindqvist mentioned this pull request Dec 18, 2018

Add resolveCiFriendliesOnly to the flattenMode Javadoc table #83

Merged

Ci friendly #77

Ci friendly #77

Conversation

lasselindqvist commented Sep 23, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fmarot commented Sep 23, 2018

lasselindqvist commented Sep 24, 2018

bertramn commented Sep 26, 2018

lasselindqvist commented Sep 26, 2018

lasselindqvist commented Sep 26, 2018

lasselindqvist commented Sep 26, 2018

patrickjamesbarry commented Oct 5, 2018

lasselindqvist commented Oct 6, 2018

patrickjamesbarry commented Oct 8, 2018

lasselindqvist commented Oct 8, 2018

hohwille commented Nov 15, 2018

lasselindqvist commented Nov 16, 2018

patrickjamesbarry commented Nov 28, 2018

lasselindqvist commented Dec 10, 2018

deathrowe commented Dec 13, 2018

hohwille left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lasselindqvist Dec 13, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hohwille left a comment

Choose a reason for hiding this comment

hohwille commented Dec 17, 2018

hohwille left a comment •

edited

Loading

lasselindqvist Dec 13, 2018 •

edited

Loading