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.

Sign up for GitHub

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

Jump to bottom

Ivy main/standalone: Patch to include 'makepom' function #71

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Ivy main/standalone: Patch to include 'makepom' function #71

wants to merge 1 commit into from

Conversation

aanno
Copy link
Contributor

@aanno aanno commented Mar 27, 2018

Hello,

I added the pomfile option to main/standalone. This allows creating an (maven) pom file from outside an ant task.

Example of use:

$ pwd
~/.ivy2/cache/org.typelevel/cats-core_2.11
$ java -jar ~/scm/github/ant-ivy/build/artifact/org.apache.ivy_2.5.0.alpha_20180327212209.jar -ivy ivy-1.0.1.xml -pomfile cats-core.xml
$ ls
cats-core-2.11.xml  ivy-1.0.1.xml  ivy-1.0.1.xml.original  ivydata-1.0.1.properties  jars  srcs

Feedback is welcome. What should I do to get this patch into mainline?

Kind regards,

aanno

jaikiran
jaikiran reviewed Mar 28, 2018
View reviewed changes
src/java/org/apache/ivy/Main.java
@@ -199,6 +201,10 @@ static CommandLineParser getParser() {
new OptionBuilder("cp").arg("cp")
.description("extra classpath to use when launching process").create())

.addCategory("maven compatibility options")
.addOption(new OptionBuilder("pomfile").arg("pomfile").countArgs(false)
Copy link
Member

Choose a reason for hiding this comment

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

A small suggestion - can you change this to something like:

new OptionBuilder("makepom").arg("pomfile")....

jaikiran
jaikiran reviewed Mar 28, 2018
View reviewed changes
src/java/org/apache/ivy/Main.java
@@ -199,6 +201,10 @@ static CommandLineParser getParser() {
new OptionBuilder("cp").arg("cp")
.description("extra classpath to use when launching process").create())

.addCategory("maven compatibility options")
.addOption(new OptionBuilder("pomfile").arg("pomfile").countArgs(false)
.description("makepom as standalone tasks").create())
Copy link
Member

Choose a reason for hiding this comment

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

I think the description should be a bit more clear and state that this generates a pom file for the resolved module.

Copy link
Contributor

Choose a reason for hiding this comment

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

IMHO pomfile is confusing (cf use of ivyfile and propertiesfile). I'd suggest writepomor something like that.

Copy link
Contributor

Choose a reason for hiding this comment

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

On the second thoughts, why not calling the option makepom 😉 ?

@jaikiran
Copy link
Member

@aanno Thank you for the patch. This mostly looks fine. I have added some review comments. Can you please also introduce a test case for this? Something that tests that these command options are recognized and the pom file does get created. There's a MainTest which you can refer to and add a new test method there.

asfgit pushed a commit that referenced this pull request Apr 4, 2018
@aanno @jaikiran
Added makepom option to main/standalone
73f7cf4 
This close PR #71 at github.com/apache/ant-ivy
@jaikiran
Copy link
Member

jaikiran commented Apr 4, 2018

@aanno, thank you for this PR. I have gone ahead and included your patch with minor modifications to upstream and also included a test along with it. I have also included your name in our release notes. Since I couldn't find your full name in your github profile, I used aanno, if you prefer to have your full name included in the release notes, please let me know and I'll update it accordingly.

@twogee
Copy link
Contributor

twogee commented Aug 6, 2018

@aanno could you please rebase and push the branch in order to close the PR?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jaikiran jaikiran jaikiran left review comments

@twogee twogee twogee left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@aanno @jaikiran @twogee