kitodo / kitodo-production

Kitodo.Production is a workflow management tool for mass digitization and is part of the Kitodo Digital Library Suite.
http://www.kitodo.org/software/kitodoproduction/
GNU General Public License v3.0
64 stars 63 forks source link

Add org.codehaus.mojo:flatten-maven-plugin (fixes #6153) #6158

Closed stweil closed 3 months ago

stweil commented 3 months ago

This plugin is required for the Maven CI friendly multi module setup, see https://maven.apache.org/maven-ci-friendly.html#install-deploy.

Fixes: #6153

stweil commented 3 months ago

With this fix it is now possible to run commands for single modules, either from the source root directory (mvn clean install -pl Kitodo-API) or from the module directory (cd Kitodo-API && mvn clean install).

henning-gerhardt commented 3 months ago

I'm not reviewing nor testing this. You introduce an issue and you are now trying to resolve this issue with more software and more complex configuration including level up the software complexity and maintenance level instead of reverting this change which is not working in this project.

With this fix it is now possible to run commands for single modules, either from the source root directory (mvn clean install -pl Kitodo-API) or from the module directory (cd Kitodo-API && mvn clean install).

This was before your change was introduced already possible as I used this way to run the module tests while migrating Junit 4 to Junit 5. This is now broken too with your change? Thanks a lot for this.

stweil commented 3 months ago

I can only repeat myself. I am sorry that my previous commit, which introduced the recommended `revision' property, was incomplete because I did not know all the use cases (they were neither documented nor part of the CI tests). This commit here completes that incomplete previous commit. The new code follows the Maven standard for multi-module setups as described in the official Maven documentation https://maven.apache.org/maven-ci-friendly.html.

It's now up to release management to decide if they want to go backwards or forwards.

solth commented 3 months ago

Fixes: 144b93f ("Use special property revision to set the version only once")

This is not the correct syntax to link a pull request as a solution to an issue. (You cannot "fix" a commit, only an issue on GitHub). The pull request description should read "Fixes #6153" instead.

stweil commented 3 months ago

This is the standard used by a huge number of projects (which also require Signed-off-by, see example). In addition I mentioned the fixed pull rquest in the PR description. Which subject line would you prefer instead of "Add org.codehaus.mojo:flatten-maven-plugin (fixes issue #6153)"?.

solth commented 3 months ago

This is the standard used by a huge number of projects

What does that matter? The issue you want to fix with this pull request is not linked, even though GitHub does support linking issues to pull requests.

Bildschirmfoto 2024-08-01 um 12 31 17

Which subject line would you prefer instead of "Add org.codehaus.mojo:flatten-maven-plugin (fixes issue #6153)"?.

fixes issue #... will not link the pull request to the issue, because "issue" is not a keyword. It should read fixes #... or resolves #... instead. See https://docs.github.com/en/get-started/writing-on-github/working-with-advanced-formatting/using-keywords-in-issues-and-pull-requests for details.

solth commented 3 months ago
Bildschirmfoto 2024-08-01 um 13 00 10

The syntax does not work in the title, but only in the pull request description. I changed it for you.

stweil commented 3 months ago

I updated the subject text now, but cannot check whether GitHub now shows the desired information. Maybe it's only visible for maintainers of this repository.

solth commented 3 months ago

Maybe it's only visible for maintainers of this repository.

No, it's visible for everyone.

On the issue page:

Bildschirmfoto 2024-08-01 um 13 03 59

An on the issue list: Bildschirmfoto 2024-08-01 um 13 04 54

You just changed it in the wrong place (subject/title instead of description), I fixed that for you.

stweil commented 3 months ago

You just changed it in the wrong place (subject/title instead of description), I fixed that for you.

Thanks!

solth commented 3 months ago

With this branch, I do not get the error @henning-gerhardt described in #6153 anymore, so it seems to work. Since I didn't encounter that error originally myself, though (and therefor prematurely approved and merged #6127), I would like @henning-gerhardt to confirm this indeed fixes the problem for him as well before moving forward and merging this pull request. If he encounters another problem with this solution I will merge #6156 instead and revert the original changes. @stweil you can then open a new pull request to comprehensively introduce the revision property after the next release.

henning-gerhardt commented 3 months ago

I tested this changes with my local development setup and inside a Debian 11 VM. My mentioned issues should be solved as running a flyway validation or similar flyway command inside the Kitodo-DataManagement module did work or at least I did no errors regarding to solve dependencies inside the module. After a mvn clean install the pom files in the ~/.m2 directory looks correct. Running tests only in a specific module is possible again.

But I still have some issues with this solution: there are .flattened-pom.xml files generated in every module directory and even they are black listed inside the .gitignore file so they did not trouble GIT they could made trouble. Why this files are not deleted after they are not used anymore? This files must only be created in the install phase and maybe on later phases but should get be deleted afterwards.

If this "solution" made more trouble in the future I did not know as the solution did not look good to me and my experience knowledge level of maven.

I would still prefer the following way for the next release:

// Edit: I'm not against this changes but I would prefer this kind of changes in the beginning until 75% of a major release development and not in the remaining 25% of a development cycle / or before a new release is made.

solth commented 3 months ago

Alright. Since this still seems to need some more time in the oven - even though the main quirks are now resolved - I will follow @henning-gerhardt's recommendation and merge #6156 to revert the changes already made to the master. @stweil you can open a new pull request containing all necessary changes that will than be considered after the next release.