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

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

Merged
merged 2 commits into from Sep 25, 2018

Conversation

Projects
None yet
4 participants
@asdf2014
Copy link
Member

asdf2014 commented Aug 13, 2018

Try to fix #6160

@asdf2014

This comment has been minimized.

Copy link
Member Author

asdf2014 commented Aug 13, 2018

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

This comment has been minimized.

Copy link
Contributor

jihoonson commented Aug 13, 2018

@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

This comment has been minimized.

Copy link
Member Author

asdf2014 commented Aug 14, 2018

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

This comment has been minimized.

Copy link
Contributor

jihoonson commented Aug 14, 2018

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

This comment has been minimized.

Copy link
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.

@jihoonson
Copy link
Contributor

jihoonson left a comment

@asdf2014 thanks! I left some more comments.

@@ -174,6 +83,111 @@
</plugins>
</build>
<profiles>
<profile>

This comment has been minimized.

@jihoonson

jihoonson Aug 16, 2018

Contributor

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.

This comment has been minimized.

@asdf2014

asdf2014 Aug 17, 2018

Author Member

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?

This comment has been minimized.

@jihoonson

jihoonson Aug 17, 2018

Contributor

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.

This comment has been minimized.

@asdf2014

asdf2014 Aug 18, 2018

Author Member

Okay, i got it.

This comment has been minimized.

@asdf2014

asdf2014 Aug 18, 2018

Author Member

Done.

<activation>
<activeByDefault>false</activeByDefault>
<property>
<name>tar</name>

This comment has been minimized.

@jihoonson

jihoonson Aug 16, 2018

Contributor

Would you explain how this property is being used?

This comment has been minimized.

@asdf2014

asdf2014 Aug 17, 2018

Author Member

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>

This comment has been minimized.

@jihoonson

jihoonson Aug 17, 2018

Contributor

Thanks. Please document this option as well.

This comment has been minimized.

@asdf2014

asdf2014 Aug 18, 2018

Author Member

Okay.

This comment has been minimized.

@asdf2014

asdf2014 Aug 18, 2018

Author Member

Added.

@@ -22,7 +22,7 @@ mvn clean package
This will compile the project and create the Druid binary distribution tar under
`distribution/target/druid-VERSION-bin.tar.gz`.

This will also create a tarball that contains `mysql-metadata-storage` extension under
After adding `-Pdist` option, this will also create a tarball that contains `mysql-metadata-storage` extension under

This comment has been minimized.

@jihoonson

jihoonson Aug 16, 2018

Contributor

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

This comment has been minimized.

@asdf2014

asdf2014 Aug 17, 2018

Author Member

Okay, i will improve this.

This comment has been minimized.

@asdf2014

asdf2014 Aug 18, 2018

Author Member

Done.

@jihoonson

This comment has been minimized.

Copy link
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:
...
```

This will compile the project and create the Druid binary distribution tar under
`distribution/target/druid-VERSION-bin.tar.gz`.

This will also create a tarball that contains `mysql-metadata-storage` extension under
The difference between `mvn clean package` and `mvn clean package -Pdist` is that

This comment has been minimized.

This comment has been minimized.

@asdf2014

asdf2014 Aug 30, 2018

Author Member

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

@jihoonson

This comment has been minimized.

Copy link
Contributor

jihoonson commented Sep 10, 2018

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

@asdf2014

This comment has been minimized.

Copy link
Member Author

asdf2014 commented Sep 20, 2018

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

huosen.jjy
@jihoonson
Copy link
Contributor

jihoonson left a comment

@asdf2014 thanks! Please check my last comment.

<argument>
-Ddruid.extensions.hadoopDependenciesDir=${project.build.directory}/hadoop-dependencies
</argument>
<argument>io.druid.cli.Main</argument>

This comment has been minimized.

@jihoonson

jihoonson Sep 21, 2018

Contributor

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

Same for other places below.

This comment has been minimized.

@asdf2014

asdf2014 Sep 25, 2018

Author Member

Done. 😅

@jihoonson jihoonson added this to the 0.13.0 milestone Sep 21, 2018

@fjy

This comment has been minimized.

Copy link
Contributor

fjy commented Sep 25, 2018

@asdf2014 can we finish this up?

@asdf2014

This comment has been minimized.

Copy link
Member Author

asdf2014 commented Sep 25, 2018

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

@jihoonson
Copy link
Contributor

jihoonson left a comment

LGTM. Thanks @asdf2014!

@QiuMM

QiuMM approved these changes Sep 25, 2018

@fjy fjy merged commit e5d9fcf into apache:master Sep 25, 2018

2 checks passed

Inspections: pull requests (Druid) TeamCity build finished
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@asdf2014 asdf2014 deleted the asdf2014:skip_exec branch Sep 26, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.