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

Fix nar-download in multi-module builds #144

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Fix nar-download in multi-module builds #144

wants to merge 2 commits into from

Conversation

mr-mbk
Copy link

@mr-mbk mr-mbk commented Oct 24, 2014

In my multi-module aggregator-based build, I have a shared library NAR.

I would like to incorporate it into one of the other modules (an RPM) by using the nar-download goal.

However, this does not work correctly because nar-download looks for the JAR + NAR in the local repository (rather than in the module's target directory).

I am not certain that this is a universally correct fix, but it does work for me.

@buildhive
Copy link

NAR plugin for Maven » nar-maven-plugin #281 SUCCESS
This pull request looks good
(what's this?)

@dscho
Copy link
Member

dscho commented Oct 24, 2014

First of all: thank you for your contribution! From a cursory inspection, it looks correct to me. The only problem I could see is that the .nar dependency might not be packaged at that state, therefore it might be possible that the file is still not at the location where the NAR plugin expects it.

I would like to ask one favor, though: I am a little bit picky when it comes to the form of Pull Requests because it makes maintaining the project much, much easier. Imagine that six months down the road, somebody would like to know why we switched to the dependency.getFile() call, claiming that it would be better to do it differently. If we can easily find the commit that introduced the change, and if that commit message provides the information why we did it, it is relatively easy to tell how to proceed from there. If instead a commit message use the File associated with the dependency is found, it is really hard to go from there.

Also, whenever a bug fix is accompanied by a regression test, it is not only much, much easier to guarantee that the bug will not be reintroduced with future Pull Requests, it is also relatively easy for the occasional reader of the commit history to augment the understanding gained from the rationale of the commit message with a practical example gleaned from the regression test.

Therefore, I would like to ask you to modify this Pull Request in the following form:

  1. squash the two commits into one (the changes touch different code, but in essentially the same way)
  2. just copy the rationale you provided in the Pull Request message into the commit message, it makes for a really great explanation what triggered the change
  3. add a commit that modifies, say, it0014 to include a nar-download step and then add a verify.bsh (similar to it0026') to make sure that the nar-download step succeeded.

For steps 1 and 2, I am sure you will find this documentation helpful.

Thanks again!

@dscho
Copy link
Member

dscho commented Dec 1, 2014

@mr-mbk are you still on it?

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.

5 participants