Skip to content

Add maven.exec.xxx.skip option for exec-maven-plugin#6162

Merged
fjy merged 2 commits intoapache:masterfrom
asdf2014:skip_exec
Sep 25, 2018
Merged

Add maven.exec.xxx.skip option for exec-maven-plugin#6162
fjy merged 2 commits intoapache:masterfrom
asdf2014:skip_exec

Conversation

@asdf2014
Copy link
Copy Markdown
Member

Try to fix #6160

@asdf2014
Copy link
Copy Markdown
Member Author

It seems that those maven options are in effect. The travis job#415338345 failed again related to the test case timeout. Would you please help me to rebuild it? @jihoonson

@jihoonson
Copy link
Copy Markdown
Contributor

@asdf2014 thanks for taking this issue. I've restarted the failed one.

BTW, IMO, it would be better to have an option to make the distribution tarball rather than avoiding pulling some dependencies. As you can see in https://github.com/apache/incubator-druid/blob/master/distribution/pom.xml, mvn install does more things other than pulling libraries to build the distribution tarball. I think we can simply skip this if some option like -Pdist is not specified. Please check Hadoop's pom.xml as an example.

@asdf2014
Copy link
Copy Markdown
Member Author

Hi, @jihoonson . Thanks for your help. The travis job is succeeded. Yes, using profile to control this will be better, it has been changed.

@jihoonson
Copy link
Copy Markdown
Contributor

Thanks. Would you please add a doc for this? Probably you can add it to https://github.com/apache/incubator-druid/blob/master/docs/content/development/build.md.

Also, if you want, you can change the travis script to not make the distribution tarball, so that we can check it helps Travis jobs to succeed.

@asdf2014
Copy link
Copy Markdown
Member Author

asdf2014 commented Aug 16, 2018

@jihoonson Indeed, it is necessary to double check it. After adding the -Pdist option, it seems that I can see from the log that this option is already works, and the exec-maven-plugin plugin starts running.

Copy link
Copy Markdown
Contributor

@jihoonson jihoonson left a comment

Choose a reason for hiding this comment

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

@asdf2014 thanks! I left some more comments.

Comment thread distribution/pom.xml Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This profile only skips pulling dependencies. How about skipping the entire process to build the distribution tarball? I think -Pdist is more intuitive name for skipping the entire process.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Do you mean that the bundle-contrib-exts part should also be included in the dist profile? Or should we include all the build plugins, such as the maven-assembly-plugin and license-maven-plugin plugins?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I mean both. We don't have to execute assembling the tarball or downloading licenses for the install phase. Also, bundle-contrib-exts is currently not executed by default, but it should also be a part of building the distribution tarball.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Okay, i got it.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Done.

Comment thread distribution/pom.xml Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Would you explain how this property is being used?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Sure, we also can use -Dtar to execute this plugin. In addition, if we used both of name and value tags, then only -Dtar=dist will be in effect.

<property>  
    <name>tar</name>  
    <value>dist</value>  
</property>

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks. Please document this option as well.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Okay.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Added.

Comment thread docs/content/development/build.md Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We need to add more details about this option. The document should say what this option means and how different mvn clean install and mvn clean install -Pdist are. Also, please update this command to mvn clean install -Pdist

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Okay, i will improve this.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Done.

@jihoonson
Copy link
Copy Markdown
Contributor

jihoonson commented Aug 21, 2018

Thanks @asdf2014. Would you check this warning which is seen when building the project.

[INFO] Scanning for projects...
[WARNING]
[WARNING] Some problems were encountered while building the effective model for io.druid:distribution:pom:0.13.0-SNAPSHOT
[WARNING] 'profiles.profile[dist].plugins.plugin.(groupId:artifactId)' must be unique but found duplicate declaration of plugin org.codehaus.mojo:exec-maven-plugin @ io.druid:distribution:[unknown-version], /Users/jihoonson/Codes/druid/distribution/pom.xml, line 152, column 29
[WARNING]
[WARNING] It is highly recommended to fix these problems because they threaten the stability of your build.
[WARNING]
[WARNING] For this reason, future Maven versions might no longer support building such malformed projects.
[WARNING]
[WARNING] The project io.druid:druid-benchmarks:jar:0.13.0-SNAPSHOT uses prerequisites which is only intended for maven-plugin projects but not for non maven-plugin projects. For such purposes you should use the maven-enforcer-plugin. See https://maven.apache.org/enforcer/enforcer-rules/requireMavenVersion.html
[INFO] ------------------------------------------------------------------------
[INFO] Reactor Build Order:
...

Comment thread docs/content/development/build.md Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Hi, @jihoonson . Sorry for the delayed reply, this has been patched, PTAL.

@jihoonson
Copy link
Copy Markdown
Contributor

@asdf2014 sorry for the delayed review. Would you please resolve conflicts? I'll review again.

@asdf2014
Copy link
Copy Markdown
Member Author

Hi, @jihoonson . The conflict has been fixed. PTAL.

Copy link
Copy Markdown
Contributor

@jihoonson jihoonson left a comment

Choose a reason for hiding this comment

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

@asdf2014 thanks! Please check my last comment.

Comment thread distribution/pom.xml Outdated
<argument>
-Ddruid.extensions.hadoopDependenciesDir=${project.build.directory}/hadoop-dependencies
</argument>
<argument>io.druid.cli.Main</argument>
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We have changed all package names from io.druid to org.apache.druid. Please fix this.

Same for other places below.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Done. 😅

@jihoonson jihoonson added this to the 0.13.0 milestone Sep 21, 2018
@fjy
Copy link
Copy Markdown
Contributor

fjy commented Sep 25, 2018

@asdf2014 can we finish this up?

@asdf2014
Copy link
Copy Markdown
Member Author

Hi, @fjy . It's done. PTAL.

Copy link
Copy Markdown
Contributor

@jihoonson jihoonson left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks @asdf2014!

@fjy fjy merged commit e5d9fcf into apache:master Sep 25, 2018
@asdf2014 asdf2014 deleted the skip_exec branch September 26, 2018 01:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

'mvn install' involves building the binary which makes Travis more failed

4 participants