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

Plugin triggers maven-checkstyle-plugin during build #567

Closed
kuhnroyal opened this issue Sep 26, 2016 · 16 comments
Closed

Plugin triggers maven-checkstyle-plugin during build #567

kuhnroyal opened this issue Sep 26, 2016 · 16 comments

Comments

@kuhnroyal
Copy link
Contributor

kuhnroyal commented Sep 26, 2016

Somehow the plugin triggers the maven-checkstyle-plugin which is bound to the validate phase during a build. Checkstyle is run twice and potentially fails the build during the second execution, since it now scans generated sources etc.
I can't track it down, if I remove the docker build, checkstyle is not triggered twice.

...
[INFO] >>> docker-maven-plugin:0.16.3:build (build) > initialize @ foo-bar >>>
[INFO] 
[INFO] --- maven-checkstyle-plugin:2.17:check (validate) @ foo-bar ---
[INFO] Starting audit...
Audit done.
[INFO] 
[INFO] <<< docker-maven-plugin:0.16.3:build (build) < initialize @ foo-bar <<<
[INFO] 
[INFO] --- docker-maven-plugin:0.16.3:build (build) @ foo-bar ---
...
                <plugin>
                    <groupId>io.fabric8</groupId>
                    <artifactId>docker-maven-plugin</artifactId>
                    <version>0.16.3</version>
                    <executions>
                        <execution>
                            <id>build</id>
                            <phase>package</phase>
                            <goals>
                                <goal>build</goal>
                            </goals>
                        </execution>
                        <execution>
                            <id>push</id>
                            <phase>deploy</phase>
                            <goals>
                                <goal>push</goal>
                            </goals>
                        </execution>
                    </executions>
                </plugin>
                <plugin>
                    <artifactId>maven-checkstyle-plugin</artifactId>
                    <executions>
                        <execution>
                            <id>validate</id>
                            <phase>validate</phase>
                            <goals>
                                <goal>check</goal>
                            </goals>
                        </execution>
                    </executions>
                </plugin>
@rhuss
Copy link
Collaborator

rhuss commented Sep 26, 2016

The problem is that the docker:build forks the lifecycle up to "initialize" which is one after "validate". These two phases are not bound to by default and hence are no-ops by default. @jgangemi and others had a use case for a pre-docker build hook like copying some resources, so we though it is a good idea to use this very early, unbound phase as such a hook.

However this means that for every d-m-p goal validate and initialize is called (that the drawback of forking the lifecycle).

A quick solution for you would be to bind checkstyle to a late phae, e.g. generate-sources or process-sources (see also the default lifecycle). Would this be possible ?

Otherwise we might reconsider to reduce the impact d-m-p has on the lifecycle.

@kuhnroyal
Copy link
Contributor Author

Good to know, I will move it behind the initialize lifecycle step.
This is enough for me atm but I think this could potentially interrupt a lot of builds with unexpected behavior.

@rhuss
Copy link
Collaborator

rhuss commented Sep 26, 2016

Yeah, that was my fear, too. But I got somehow convinced in #505 since both phases are typically unused. That doesn't mean I could be convinced back :)

@kuhnroyal
Copy link
Contributor Author

Well for me this information was enough to fix it. Maybe its enough if this gets a prominent place in the documentation.

@jgangemi
Copy link
Collaborator

it's odd that the source plugin doesn't cause this behavior as well b/c it does the same thing where it forks the lifecycle and calls initialize.

i still think this is a relatively safe operation. if you have plugins running during those two phases that aren't just bringing in external configuration (such as the properties plugin), you're most likely doing something wrong.

checkstyle, findbugs, etc all seem like they should occur later in the build, such as during verify.

@rhuss
Copy link
Collaborator

rhuss commented Oct 7, 2016

From the existing documentation:

All goals fork the lifecycle phase to call the lifecycle up to the initialize phase. This means you can bind other plugins to the phases 'initialize' or 'validate' for customization. By default no action is bound to these phases. See Introduction to the Build Lifecycle for more details about phases and lifecycles.

Note however that when you bind the goals about to lifecycle phases this will result in the initialize and validate phase called twice. Your bound plugins should be able to handle this.

Hope this is sufficient, so I'll close this issue.

@rhuss rhuss closed this as completed Oct 7, 2016
@josephlbarnett
Copy link

We ran into this breaking our builds because we bind checkstyle to the validate phase, so that it runs before the generate-sources phase (the generated sources violate our checkstyle rules, and excluding the generated sources from checkstyle proved to be difficult). We also bind a bunch of other plugins to validate and initialize, they're mostly idempotent but it definitely slows the build to run them multiple times (at least 3 times when we're doing docker:builds and docker:starts in a given module). Any chance this could be reconsidered?

@jgangemi
Copy link
Collaborator

how do you deal w/ generating the source jars, etc for your projects b/c that plugin does the exact same thing.

if it's really causing problems, i can give it up. i had been using the functionality to store some properties in a file and load then in using the properties plugin, but it won't kill me if i have to go back to defining those in the pom.

@josephlbarnett
Copy link

we use jar-no-fork and test-jar-no-fork for the source jars. which seems like the right idea here (having a fork version and a non-fork version). I'd pitch for introducing new goals that fork rather than changing the behavior of existing goals so that people who need the new behavior can opt in, but up to @rhuss

@rhuss
Copy link
Collaborator

rhuss commented Oct 25, 2016

@jbarnettwomply, sorry for being late. I think its a good idea to have two flavors of Mojos which can be bound to an execution phase or run directly from the command line. It doesn't make sense for all, though (e.g you probably never bind docker:watch or docker:log to an execution phase.

Also, I'd prefer it the other way around. I.e. a -nofork variation for those goals which need to be forked. The reason is, that adding these goals to <executions> is a one time cost, wherea calling e.g. mvn docker:build-fork is called much more often, so it should be the shorter goal name.

But lets discuss, which goals really need forking for @jgangemi 's use case. For me the only one really required is the BuildMojo because it always need a package phase to be called in order to find the artifacts to include (thats a Maven limitation btw).

So let me start the list of Mojos which would be nice to have a forked variant:

  • BuildMojo (see above)
  • SourceMojo (for the same reason)

Any other goals ?

So from my point of view it would be enough to introduce docker:build-nofork and docker:source-nofork wich then can be used in some execution binding.

wdyt ?

@josephlbarnett
Copy link

As long as we can bind non-forking versions of build, start, stop, and push, your proposal works for our use cases. I'll let @jgangemi speak to which Mojos he would like to have forking variants of.

@jgangemi
Copy link
Collaborator

my use case was to be able to put properties into an external file and load them w/ the maven-properties-plugin so that basically means i'd need to have all goals do this.

as i said before - i don't have any major issue w/ giving this up entirely. it's not the end of the world to put these properties into the pom itself, so i'm ok if this functionality goes away outside of what there is a steadfast requirement for.

@rhuss
Copy link
Collaborator

rhuss commented Oct 25, 2016

Ok, cool. Thanks @jgangemi ! I will then probably only fork the two goals mentioned above.

As service in return I will merge in your docker-compose PR this evening or so ;-) I've upgrade it to docker compose version 2, and I'm just about to add some documentation.

So you can expect a 0.17.1 soon :)

@jgangemi
Copy link
Collaborator

that would actually be pretty awesome! we have a need to read docker-compose files and this will be a huge help in that area!

rhuss added a commit that referenced this issue Oct 26, 2016
* Removed forking of every Mojo
* BuildMojo and SourceMojo are now forking to 'package'
* Add BuildNoForkMojo and SourceNoForkMojo
* Update documentation accordinglyg

Relates to #567 and #599

Signed-off-by: Roland Huß <[email protected]>
@rhuss
Copy link
Collaborator

rhuss commented Oct 28, 2016

Hey @jgangemi 0.17.1 is out with your Docker Compose implementation, slightly tuned ;-) Here are the docs --> https://dmp.fabric8.io/#docker-compose

I'm super happy for every feedback on that.

@jgangemi
Copy link
Collaborator

awesome! is that close to my original implementation w/ the services block? the docs say you can include items from the xml blocks, but doesn't give an example.

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

No branches or pull requests

4 participants