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

Generate the pom.xml as part of the build #224

Closed
wants to merge 1 commit into from

Conversation

rwinch
Copy link
Member

@rwinch rwinch commented Jan 28, 2013

@cbeams - We were not sure if we wanted to do this or not, but I put together a pull request to see what you thought.

//cc @philwebb @rstoyanchev

Previously the pom.xml was only generated when doing maven deployments.
This is nice since the metadata can be derived from the Gradle build.
However, there have been a issues that have come from using this
approach (i.e. SPR-10218).

Since the pom.xml files are so critical, it is nice to include these in
version control so that we can perform a diff each time they change.
This commit introduces a new task that generates the pom.xml files and
updates the .gititnore to include the pom.xml files.

Previously the pom.xml was only generated when doing maven deployments.
This is nice since the metadata can be derived from the Gradle build.
However, there have been a issues that have come from using this
approach (i.e. SPR-10218).

Since the pom.xml files are so critical, it is nice to include these in
version control so that we can perform a diff each time they change.
This commit introduces a new task that generates the pom.xml files and
updates the .gititnore to include the pom.xml files.
@cbeams
Copy link
Contributor

cbeams commented Feb 26, 2013

I've taken a look at this, and the biggest problem I'm seeing so far is regarding merge conflicts. For example, if 3.2.x contains generated poms with values of 3.2.2.BUILD-SNAPSHOT, merging 3.2.x into master becomes problematic, because the generated poms there already have values of 4.0.0.BUILD-SNAPSHOT.

It's true that we already have this problem with the 'version' value in build.properties, but this is just a single file, which makes it more manageable. I use Git's 'rerere' option to remember the merge conflict resolution for build.properties and automatically re-apply it every time I do a merge from 3.2.x into master, and we could conceivably have the same arrangement for all the pom files, but if someone besides me is doing that merge, then it starts to get ugly (i.e. it's going to feel non-trivial to that person to get it right).

The main value of checking in the generated poms, as I see it, is being able to quickly assess what's changed in the dependency manifest, which is particularly valuable at release preparation time. However, now that we have the automatic sorting of dependencies handled, I think that continuing to do more manual diffs against previous pom versions is a fine compromise.

I was going to mention that we could also leave the in place the new generatePom task, but forego tying it to the build, and leave any generated pom.xml artifacts as ignored by git, but in reality, any time you do a gradle install, the generated pom is written to build/poms/pom-default.xml anyway. Perhaps this is good enough?

Also, I noticed that the poms being generated from generatePom contain invalid <scope>optional</scope> elements, instead of the correct <scope>compile</scope><optional>true</optional>. However, when doing a regular gradle install, the poms published to build/poms/pom-default and ~/.m2/repository have the correct arrangement.

In short, I'm recommending we leave this change out, and just roll with the deterministically-ordered changes that have already been committed. Your thoughts?

@rwinch
Copy link
Member Author

rwinch commented Feb 26, 2013

I see your point about merging between branches.

Perhaps the sorting of the dependencies is enough to ensure we catch issues in the future. However, due to the number of issues with the pom files lately, I personally think the pom files are critical enough that they should probably be checked in so that the person that is updating the dependencies is certain to compare against the previous revision. Making the comparisons a more "in your face" experience is also good to catch issues in our build logic (i.e. merging of dependencies, re-mapping of provided/optional, etc).

Perhaps @rstoyanchev or @philwebb can weigh in their opinions too.

@philwebb
Copy link
Member

I would prefer to leave the POMs out of git but perhaps we can generate a difference report similar to JDiff when we release? I think as long as the release process spells out that you should compare the POMs that is enough.

@rstoyanchev
Copy link
Contributor

I would like the generated poms in. In addition to being "in your face" and I think we should be aware of issues as they crop up, the dependency visualization of the Eclispe POM editor is still a feature I miss.

That said the merge issue is unpleasant. Perhaps the use of git-rerere can be documented on the build wiki page so we can all refer to it. Would that remove the pain maybe?

@snicoll
Copy link
Member

snicoll commented Apr 18, 2014

@rwinch isn't this one outdated by now? Thanks.

@rwinch
Copy link
Member Author

rwinch commented Apr 18, 2014

I'm not sure that this is outdated so much as trying to figure out if we wanted to make the changes. It has been quite some time ago, but if I recall correctly (and looking at the chain) I think a majority did not want to include the poms in the build. Can anyone else chime in? Do we still want to keep poms out of the build? If so, I think this PR can be closed.

cc @cbeams @philwebb @jhoeller @rstoyanchev

@philwebb
Copy link
Member

I think the general consensus was to leave the generated POMs out. I'll close this for now.

@philwebb philwebb closed this Apr 22, 2014
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

Successfully merging this pull request may close these issues.

5 participants