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

JAMES-1902 reorganizes blob modules #261

Closed
wants to merge 3 commits into from

Conversation

jeantil
Copy link
Contributor

@jeantil jeantil commented Nov 16, 2020

This PR also removes a couple duplicate dependencies that were present in the POM files.

Copy link
Contributor

@chibenwa chibenwa left a comment

Choose a reason for hiding this comment

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

That was a short follow up ;-)

We will launch tests on that tomorrow morning hanoi tz.

Thanks for your involvement.

@@ -25,7 +25,7 @@
<groupId>org.apache.james</groupId>
<artifactId>james-server-guice</artifactId>
<version>3.6.0-SNAPSHOT</version>
<relativePath>../pom.xml</relativePath>
<relativePath>../../pom.xml</relativePath>
Copy link
Contributor

Choose a reason for hiding this comment

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

Does it makes sense to keep the blob- prefix for folders inside a blob directory? Idem for the guice suffix.

Shouldn't the naming scheme be guice/blob/api ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

you are right, I wanted to minimize changes but should have included that bit.

Copy link
Contributor

Choose a reason for hiding this comment

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

Should we do the same for the queue-xx-guice modules, if any?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the queue-xxx- modules were already in clean directories ( neither prefix nor suffix)

`src/site/xdoc` recommends to run
`mvn com.github.ekryd.sortpom:sortpom-maven-plugin:sort\
-Dsort.keepBlankLines -Dsort.sortDependencies=groupId,artifactId\
-Dsort.nrOfIndentSpace=4 -Dsort.createBackupFile=false\
-Dsort.sortModules=true -Dsort.expandEmptyElements=false\
-Dsort.predefinedSortOrder="recommended_2008_06"`
to clean up poms

I did it for blob and this commit addresses sort failures for
`server/container/guice/queue` pom.xml which were incorrect in the
previous PR and the `server/container/guice/queue` pom.xml
@jeantil
Copy link
Contributor Author

jeantil commented Nov 16, 2020

Looking to make sure I had the proper sort order in the POM files I stumbled upon sortpom, wich led me to site/src/xdoc/contribute which helpfully provides:

mvn com.github.ekryd.sortpom:sortpom-maven-plugin:sort -Dsort.keepBlankLines -Dsort.sortDependencies=groupId,artifactId -Dsort.nrOfIndentSpace=4 -Dsort.createBackupFile=false -Dsort.sortModules=true -Dsort.expandEmptyElements=false  -Dsort.predefinedSortOrder="recommended_2008_06"

I'm not sure if this recommandation is still applicable because running it returned over 150 incorrectly sorted pom, I applied the fixes to pom files from :

  • server/container/guice/queue/*
  • server/container/guice/blob/*
  • server/container/guice/pom.xml

hopefully that's ok

@chibenwa
Copy link
Contributor

We did not run that for ages, but the rules still applies, and we should do a run... (conflicts likely expected!) You are very right...

Copy link
Contributor

@chibenwa chibenwa left a comment

Choose a reason for hiding this comment

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

A well invested lunch pause!

@jeantil
Copy link
Contributor Author

jeantil commented Nov 16, 2020

We did not run that for ages, but the rules still applies, and we should do a run... (conflicts likely expected!) You are very right...

Should it be configured to run automatically on every compile ?
something like

mvn compile -Psort

to make it easier to sort the modules ? then each new PR can commit the fixed poms it has modified
after a while a mass commit of usually untouched pom files and then a strict enforcement right into mvn compile ...

@chibenwa
Copy link
Contributor

Should it be configured to run automatically on every compile ?

And abort the build if needed? => 👍

Copy link
Contributor

@Arsnael Arsnael left a comment

Choose a reason for hiding this comment

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

Nice work, thanks!

@chibenwa
Copy link
Contributor

I just merged this work. Can we close the PR?

@jeantil jeantil closed this Nov 18, 2020
@jeantil jeantil deleted the extract-guice-modules branch December 3, 2020 12:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants