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

Update some dependencies to latest bugfix versions and more commits related to dependencies #6152

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

Conversation

stweil
Copy link
Member

@stweil stweil commented Jul 28, 2024

The updates were selected from this list:

mvn versions:display-dependency-updates | grep -- "->" | sort | uniq

@stweil
Copy link
Member Author

stweil commented Jul 28, 2024

The CI test failure is unrelated to this pull request, see #6138.

@stweil stweil force-pushed the dependencies branch 2 times, most recently from 43cc244 to f7d4ee9 Compare July 29, 2024 20:35
@stweil
Copy link
Member Author

stweil commented Jul 30, 2024

I added a commit which adds the missing "test" scope to some dependencies which are only relevant for the tests.

@stweil stweil force-pushed the dependencies branch 3 times, most recently from 53f242b to 6fd113a Compare August 5, 2024 13:02
pom.xml Outdated Show resolved Hide resolved
pom.xml Outdated Show resolved Hide resolved
@stweil stweil changed the title Update some dependencies to latest bugfix versions Update some dependencies to latest bugfix versions and more commits related to dependencies Aug 5, 2024
pom.xml Outdated Show resolved Hide resolved
pom.xml Outdated Show resolved Hide resolved
pom.xml Outdated Show resolved Hide resolved
@stweil stweil added the dependencies Pull requests that update a dependency file label Oct 7, 2024
@@ -56,6 +56,7 @@
<dependency>
<groupId>org.hamcrest</groupId>
<artifactId>hamcrest</artifactId>
<scope>test</scope>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not needed as defined in parent as test scope.

Copy link
Member Author

Choose a reason for hiding this comment

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

I only fixed issues reported by the dependency check ...

This also applies to the other locations where I added the test scope as suggested by this check.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I get no issues on running different dependency checks. So maybe your tooling is to new? Or how did you get this issues.

Copy link
Member Author

Choose a reason for hiding this comment

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

See commit description: "The missing scopes were reported by mvn dependency:analyze."

Copy link
Collaborator

@henning-gerhardt henning-gerhardt Oct 8, 2024

Choose a reason for hiding this comment

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

And again: I'm not getting this missing scopes report:

[INFO] --- maven-dependency-plugin:2.8:analyze (default-cli) @ kitodo-data-editor ---
Unable to process: org.kitodo.dataeditor.MetsKitodoConverter
Unable to process: org.kitodo.dataeditor.MetsKitodoWrapper
Unable to process: org.kitodo.dataeditor.VersionProvider
Unable to process: org.kitodo.dataeditor.ruleset.KeyView
Unable to process: org.kitodo.dataeditor.ruleset.xml.Ruleset
Unable to process: org.kitodo.dataeditor.ruleset.xml.EditingElement
Unable to process: org.kitodo.dataeditor.ruleset.xml.ConditionsMap
Unable to process: org.kitodo.dataeditor.ruleset.Rule
Unable to process: org.kitodo.dataeditor.ruleset.DivisionDeclaration
Unable to process: org.kitodo.dataeditor.ruleset.RulesetManagement
Unable to process: org.kitodo.dataeditor.ruleset.KeyDeclaration
Unable to process: org.kitodo.dataeditor.ruleset.Labeled
Unable to process: org.kitodo.dataeditor.ruleset.Settings
Unable to process: org.kitodo.dataeditor.ruleset.NestedKeyView
Unable to process: org.kitodo.dataeditor.entities.FileSec
Unable to process: org.kitodo.dataeditor.entities.PhysicalStructMapType
Unable to process: org.kitodo.dataeditor.entities.LogicalStructMapType
Unable to process: org.kitodo.dataeditor.JaxbXmlUtils
Unable to process: org.kitodo.dataeditor.MetsKitodoReader
Unable to process: org.kitodo.dataeditor.DataEditorTest
Unable to process: org.kitodo.dataeditor.MetsKitodoWriterTest
Unable to process: org.kitodo.dataeditor.ruleset.RulesetManagementIT
Unable to process: org.kitodo.dataeditor.MetsKitodoWrapperTest
[WARNING] Used undeclared dependencies found:
[WARNING]    commons-io:commons-io:jar:2.11.0:provided
[WARNING]    org.junit.jupiter:junit-jupiter-api:jar:5.9.2:test
[WARNING] Unused declared dependencies found:
[WARNING]    org.junit.jupiter:junit-jupiter-engine:jar:5.9.2:test
[WARNING]    org.hamcrest:hamcrest:jar:2.2-rc1:test

edit: With my knowledge it is not needed to adjust the scopes in sub-modules if they are the same as in the parent defined scope except you want an other use scope in a sub-module.

@@ -47,6 +47,7 @@
<dependency>
<groupId>org.hamcrest</groupId>
<artifactId>hamcrest</artifactId>
<scope>test</scope>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not needed as defined in parent as test scope.

@@ -43,6 +43,7 @@
<dependency>
<groupId>org.hamcrest</groupId>
<artifactId>hamcrest</artifactId>
<scope>test</scope>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not needed as defined in parent as test scope.

@@ -233,6 +233,7 @@
<dependency>
<groupId>org.xmlunit</groupId>
<artifactId>xmlunit-matchers</artifactId>
<scope>test</scope>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not needed as defined in parent as test scope.

Copy link
Member Author

@stweil stweil Oct 8, 2024

Choose a reason for hiding this comment

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

I don't see a test scope for xmlunit-matchers in my latest pom.xml.

Copy link
Collaborator

Choose a reason for hiding this comment

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

You add the right scope in your current pull request and as the scopes are passed to the sub modules you did not define it again. As already written in my post above.

pom.xml Outdated
<primefaces.extensions.version>8.0.5</primefaces.extensions.version>
<saxon.version>9.9.1-8</saxon.version>
<log4j.version>2.19.0</log4j.version>
<junit.version>5.9.2</junit.version>
<elasticsearch.version>7.17.10</elasticsearch.version>
<elasticsearch.version>7.17.24</elasticsearch.version>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why you are changing this version after pull request #6250 is already open which contains this change?

Copy link
Member Author

Choose a reason for hiding this comment

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

There would be a conflict otherwise.

Copy link
Collaborator

Choose a reason for hiding this comment

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

No.

Copy link
Member Author

Choose a reason for hiding this comment

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

@solth, @henning-gerhardt, GitHub's Dependabot suggests a syntax like <version>[2.14.0,)</version> (not here, but for commons-io:commons-io. Did you ever test this variant? Would [7.17.24,) instead of 7.17.24 work here and automatically use the very latest patch version?

Copy link
Collaborator

Choose a reason for hiding this comment

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

So far as I know this notation is not wanted as it introduce new issues as everyone is using different version of the same artifact.

Copy link
Collaborator

@henning-gerhardt henning-gerhardt left a comment

Choose a reason for hiding this comment

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

It is only a reading and compiling review. I can not test the changes itself in a working environment.

@henning-gerhardt
Copy link
Collaborator

Ignore my review and this should be reviewed by an other person.

The updates were selected from this list:

    mvn versions:display-dependency-updates | grep -- "->" | sort | uniq

Signed-off-by: Stefan Weil <[email protected]>
These updates were suggested by claude.ai.

Signed-off-by: Stefan Weil <[email protected]>
The missing scopes were reported by `mvn dependency:analyze`.

Signed-off-by: Stefan Weil <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants