'runOnlyOnce' coupled with Maven's --projects param makes plugin not run at all

[fmarot]

Describe the bug

In the case of a multi-module Maven project, when we use the 'runOnlyOnce=true' parameter coupled with 'injectAllReactorProjects=true', we expect the plugin to run once to calculate all git variables and we expect those variables to be available in children projects.

But this is not the case when we run Maven on a specific sub-project:
mvn install --projects mySubProject

neither when we use this command to resume from a specific sub-project:
mvn install -rf mySubProject

Steps to Reproduce

• Here is the config in the parent pom:

			<plugin>
				<groupId>pl.project13.maven</groupId>
				<artifactId>git-commit-id-plugin</artifactId>
				<version>2.2.5</version>
				<executions>
					<execution>
						<id>get-the-git-infos</id>
						<goals>
							<goal>revision</goal>
						</goals>
						<phase>initialize</phase>
						<configuration>
							<generateGitPropertiesFile>false</generateGitPropertiesFile>
							<runOnlyOnce>true</runOnlyOnce>
							<injectAllReactorProjects>true</injectAllReactorProjects>
							<verbose>true</verbose>
							<skipPoms>false</skipPoms>
						</configuration>
					</execution>
				</executions>
			</plugin>

Expected behavior

mvn install --projects mySubProject

we expect to see the verbose logs of all the variables calculated.
But in fact we get this:

[INFO] --- git-commit-id-plugin:2.2.5:revision (get-the-git-infos) @ mySubProject ---
[INFO] runOnlyOnce is enabled and this project is not the top level project, skipping execution!

Additional context

version tested is the last released: 2.2.5

My analysis:

The code in GitCommitMojo looks like

if (runOnlyOnce) {
        if (!session.getExecutionRootDirectory().equals(session.getCurrentProject().getBasedir().getAbsolutePath())) {
          log.info("runOnlyOnce is enabled and this project is not the top level project, skipping execution!");
          return;
        }
      }

=> so the code expects to run at least once from Maven's ExecutionRootDirectory. But there it does not happen.

My suggestion is to set a variable (user variable ?) the first time the plugin is ran and when this variable is already set, we bypass the execution in case 'runOnlyOnce=true'
I will provide a Pull Request soon.

[TheSnoozer]

Hi,
thanks for posting this issue here!
I'll need to investigate if there are better alternatives to actually runOnlyOnce in such cases.

Based on the doc the plugin is somewhat behaving as desired:

In a multi-module build, only run once. This means that the plugins effects will only execute once, for the parent project.

I'm not sure yet if setting some variables (e.g. user variables) would be the most reliable way. Especially when you run maven in parallel (e.g. mvn -T 4 clean) this might be prone to race conditions.

[fmarot]

@TheSnoozer you're right regarding the parallel execution. However I feel like the risk is lower than the benefits but I totally understand your concerns. I'll be interested if there is a best way to correct this problem.

[TheSnoozer]

I need to look into that more carefully.

Seems our runOnlyOnce configuration got inspired by https://blog.sonatype.com/2009/05/how-to-make-a-plugin-run-once-during-a-build/

The article states that the mavenAssemblyPlugin has adopted that...however they 'cheated' and called the option runOnlyAtExecutionRoot (see http://maven.apache.org/plugins/maven-assembly-plugin/single-mojo.html#runOnlyAtExecutionRoot).

[fmarot]

Your link to the Sonatype blog reminded me that I already discussed this exact same problem on the Maven mailing-list ( http://maven.40175.n5.nabble.com/Make-custom-plugin-executed-only-once-in-multimodule-inherited-false-aggregator-td5921630.html ) and... it seems there is no real solution :/

[fmarot]

I've just realized that during the build, the Mojo is instantiated multiple times (once per module) but it is always the same class so maybe we could have a thread-safe static instance of something preventing concurrent access. It would be better that user variables.
I fear there may be edge cases where different classes are used through different classloaders (maybe if you define multiple versions of the same plugin in different modules part of the same reactor) but is this really cases we have to consider ?

[TheSnoozer]

Your link to the Sonatype blog reminded me that I already discussed this exact same problem on the Maven mailing-list ( http://maven.40175.n5.nabble.com/Make-custom-plugin-executed-only-once-in-multimodule-inherited-false-aggregator-td5921630.html ) and... it seems there is no real solution :/

Unfortunately I don't see any obvious solution either and a popular search engine didn't yield any obvious results.

The issue is simply that maven has it's many pitfalls.
You need to consider parallel execution, multiple configurations across different child poms, multiple executions within a single child pom, ...maven might even behave differently when called with mvn clean package VS mvn com.test.plugins:testPlugin:deployMojo (example https://issues.apache.org/jira/browse/MNG-6260). On top of that all you also need to consider to support essentially all 3.X maven versions since none of them are marked as 'end of life'. So even if something might become available in 3.6.X we might not be able to integrate it into the plugin, cause anyone should really be able to choose a maven version that is not 'end of life' (also consider legacy CI systems that never get updated).

I've just realized that during the build, the Mojo is instantiated multiple times (once per module) but it is always the same class so maybe we could have a thread-safe static instance of something preventing concurrent access. It would be better that user variables.
I fear there may be edge cases where different classes are used through different classloaders (maybe if you define multiple versions of the same plugin in different modules part of the same reactor) but is this really cases we have to consider ?
Thanks for the investigation. Mhh...in my past experience with maven it always bit me hard when I tried to something that is 'not the maven way'. At least from my perspective in the maven world you rather don't want to invent "hacks". When already think about edge cases...in all fairness I can't speak for every user of the plugin and I can't even tell how many users are out there. There is simply no possible way to tell how the plugin is used, what edge cases to consider and what could safely be ignored.

I truly can understand your thinking behind raising this issue and would be happy to integrate it in some (reliable) way into the plugin. However at this stage I'd rather tend to say that the actual runOnlyOnce feature is not really well supported within maven. I'd prefer not to invent a hack to have such a support in the plugin. If there is an official way, sure let's do that. However unfortunately there doesn't seem to be an obvious one available.

Feel free to add further comments if you actually find a working solution, but messing around with different classloaders and all cases I listed above seems a bit unpredictable for me (feel free to convince me wrong).

For now I would say, yes the option is badly worded, but does exactly what is described in the documentation.

[fmarot]

Hello @TheSnoozer , and thanks for your long reply.
I have a few remark though:

1- I've exeprimented on a large project (100 Maven modules) with running 'mvn initialize' with runOnlyOnce=true/false.
runOnlyOnce=true --> 40seconds
runOnlyOnce=false --> 9 minutes (!)
So my conclusion is that runOnlyOnce=true is a feature I cannot do without.

2- I'd rather have the plugin also run when a developper runs 'mvn install --projects submodule'. If it does not run, with this standard Maven command (which is recommanded over running the command directly inside the submodule) the developpers will randomly have git properties not initialized which may lead to jars containing incomplete infos.
This is really worrying for me: I'd rather be the plugin be consistent in some classis use-case rather it be consistent in some very strange use-case (see 4.2 and the potential problems when there would be multiple versions of the plugin in the same reactor)

3- So my conclusion is we must do something for this classic use-case.

4- Problem is how to do it ?
4.1- User property base implementation

• if we have the user-properties based implementation we risk in rare case the plugin to execute multiple times.
  • in most situations where there is a single git repo, this is not a problem because computed properties will always be the same.
  • in situations where there are multiple git repos, it may lead to overwritten properties of the 1st git repo with the ones from the second git repo.
    This is coherent with the current documentation stating that "This probably won't 'do the right thing' if your project has more than one git repository".

4.2- synchronized static variable in Mojo implementation

• this should do the job when there is a single versiobn of the plugin defined in the reactor.
• problem arise when multiple versions of the plugin are defined in the reactor: they all will be executed once... But I tend to think it is not a very standard configuration. Who would want to do that ?! It could 'just' be documented as "we do not support using multiple versions of the plugin in the same reactor".

5- the plugin is going toward it's 3.0 release so it seems to me like the right time a change a bit of a feature that may break something (even though I do not see how it could break stuffs but I acknowlege there are many edge cases). But Itotally understand that you may not want to take the risk to break stuffs.

Regards

update1: for info, here is how the buildnumber-maven-plugin does with its 'getRevisionOnlyOnce' param: https://github.com/mojohaus/buildnumber-maven-plugin/blob/7faf13a6a67c5e7762f7cf3609178fedfd47c8d3/src/main/java/org/codehaus/mojo/build/CreateMojo.java#L273

[TheSnoozer]

Thanks for the lengthy reply and your understanding :-)
I understand we need to do something and I agree to the question is how we do it.

Hello @TheSnoozer , and thanks for your long reply.
I have a few remark though:

1- I've exeprimented on a large project (100 Maven modules) with running 'mvn initialize' with runOnlyOnce=true/false.
runOnlyOnce=true --> 40seconds
runOnlyOnce=false --> 9 minutes (!)
So my conclusion is that runOnlyOnce=true is a feature I cannot do without.

Mhh 400 submodules sounds a bit excessive, but in theory there should be nothing preventing you to do that. Just out of curiosity do you have <useNativeGit>true</useNativeGit> or <useNativeGit>false</useNativeGit> (default) configured? 9 minutes just to gather properties is wrong and unaccetable.

2- I'd rather have the plugin also run when a developper runs 'mvn install --projects submodule'. If it does not run, with this standard Maven command (which is recommanded over running the command directly inside the submodule) the developpers will randomly have git properties not initialized which may lead to jars containing incomplete infos.
This is really worrying for me: I'd rather be the plugin be consistent in some classis use-case rather it be consistent in some very strange use-case (see 4.2 and the potential problems when there would be multiple versions of the plugin in the same reactor)

Having no properties when they are desired is really the worst case. We don't want that to happen under any circumstances.

3- So my conclusion is we must do something for this classic use-case.

4- Problem is how to do it ?
4.1- User property base implementation

* if we have the user-properties based implementation we risk in rare case the plugin to execute multiple times.
  
  * in most situations where there is a single git repo, this is not a problem because computed properties will always be the same.
  * in situations where there are multiple git repos, it may lead to overwritten properties of the 1st git repo with the ones from the second git repo.
    This is coherent with the current documentation stating that "This probably won't 'do the right thing' if your project has more than one git repository".

Agree, when anyone uses the option it must be very clear that this might screw things badly when someone has multiple repositories.

4.2- synchronized static variable in Mojo implementation

* this should do the job when there is a single versiobn of the plugin defined in the reactor.

* problem arise when multiple versions of the plugin are defined in the reactor: they all will be executed once... But I tend to think it is not a very standard configuration. Who would want to do that ?! It could 'just' be documented as "we do not support using multiple versions of the plugin in the same reactor".

5- the plugin is going toward it's 3.0 release so it seems to me like the right time a change a bit of a feature that may break something (even though I do not see how it could break stuffs but I acknowledge there are many edge cases). But Itotally understand that you may not want to take the risk to break stuffs.

Trust me I love breaking stuff :-P

Regards

update1: for info, here is how the buildnumber-maven-plugin does with its 'getRevisionOnlyOnce' param: https://github.com/mojohaus/buildnumber-maven-plugin/blob/7faf13a6a67c5e7762f7cf3609178fedfd47c8d3/src/main/java/org/codehaus/mojo/build/CreateMojo.java#L273

Altogether (mainly due to backward compatibility) I would not mess with the runOnlyOnce option. I'd rather mark it as deprecated and introduce it as new property runOnlyAtExecutionRoot and invent a new name for the request you have.

My current assumptions and thinking would be introduce a new option enableCaching (or some similar name). That new options would run 'once' and populate all it's properties to all reactors (similar to what <injectAllReactorProjects>true</injectAllReactorProjects> would do). From the plugin side what we then could do is still execute in each submodule, however before asking git for any properties we could check if the properties are already available in the project. If they are available, we we assume it's the right one and use them as cache and simply generate the configured output file.

Just thinking out loud about some corner cases (just a braindump)

• the plugin allows to specify a property prefix via <prefix>custom.git.prefix</prefix>, if a reactor/submodule would specify a custom prefix the plugin would run again - that's my main reasoning to currently call it a cache, since there could be a cache hit (no need to run again), or a cache miss (well run whatever again). So in practice if you configure all the properties to have the same name, it would act as 'runOnlyOnce'
• if we want to support multiple repositories, the only thing that IMHO needs to be ensured is that the prefixes across the different repositories don't get mixed up (and a configuration as outlined Option to accept multiple git directories  #137 (comment) should remain working, again the cache doesn't restrict the execution to once, rather once per configured property which IMHO allows greater flexibility)
• I'm yet not sure how and if we should deal with separate configurations across multiple repositories. An Example would be that a submodule configures the length of the abbreviated git commit it (git.commit.id.abbrev) with <abbrevLength>7</abbrevLength> and another to 8. Since our cache would work on the property name the plugin might fail to identify such case and would result in a property from the first run (which one was the first run? that might become a lot of fun to troubleshoot). In case the user really wants to have a different properties, the workaround is to give the plugin a different prefix in the different configurations
• I'm also not sure yet about the <excludeProperties> and <includeOnlyProperties>. These configuration are currently documented as a filter to prevent certain properties to end up in the generated properties file. Technically the documentation only says that we prevent the exposure to the properties file, but in really i think (would need to check) if a property is filtered out, they are also not exposed to the reactor project. If we want the cache to work, that might mean that such properties suddenly get exposed to the reactor project (security concerns?). However for the final generated property there should be no difference, since the 'caching' really just should work for the properties stored in the reactor project. Generating a file should be a rather easy task that should not take alot of time. For this point I really can't speak for the users who originally wanted that option and thus I'm not sure if it would mean that we are not able to put them in our 'properties reactor cache'. However as stated above the documentation says that the properties are not stored inside the generated file . Open todo for me: check how the implementation is actually behaving here.
• The <injectAllReactorProjects>true</injectAllReactorProjects> and new cache option surely would rely on the fact that the properties that are stored are the 'right' ones - manipulation from any other plugin is not checked and we kinda just the user deal with it

What do you think about such caching-Option? Would that satisfy your request?

[TheSnoozer]

I just got reminded of this issue and I think a few of the runtime issues will be solved with the 3.0.0 release I'm currently preparing.

The 3.0.0 comes with

• selective running: Instead of determine all properties and then exclude properties why not choose selective running.
• a smarter way of skipping git invocation if properties are already computed (applies for multi-modules project where injectAllReactorProjects is set to true)

For details refer to the PR (#414) that I'm currently testing for the release :-)

Also for some inspiration:
I might have stumbled over some similar implementation we might need here. Essentially the sonar-scanner-maven plugin has some logic to execute at the very end. I guess it would be trivial to invert the logic to have this applied only for the first project.

However for now I have scheduled this for 3.0.1 since I think there is quite a lot stuff already in 3.0.0 that i really want to get shipped for a while now :-)

[TheSnoozer]

Fixed/Implemented via: #443

[delanym]

Extensions are always run only once, so use https://github.com/pascalgn/properties-maven-extension