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

[build] remove extraneous dependency #232

Merged
merged 1 commit into from
Mar 1, 2023

Conversation

line-o
Copy link
Member

@line-o line-o commented Jan 12, 2023

  • remove extraneous dependency on log4j-slf4j2-impl

    BASIC RULE Embedded components such as libraries or frameworks should not declare a dependency on any SLF4J binding/provider but only depend on slf4j-api.

    from https://www.slf4j.org/manual.html

@line-o line-o requested review from reinhapa, joewiz and adamretter and removed request for reinhapa and joewiz January 12, 2023 19:58
@adamretter
Copy link
Contributor

adamretter commented Jan 12, 2023

I think there are a number of problems with this PR at the moment:

  1. It attempts to roll the version of the package back from 4.0.0 to 3.0.6-SNAPSHOT
  2. It declares that eXist-db provides slf4j-api 1.7.33 - however eXist-db 6.1.0 does not provide that version of the slf4j-api. It provides slf4j-api 2.0.5
  3. It declares the minimum version of eXist-db required by Monex is 5.4.0, however the CI tests executed against eXist-db 5.4.0 fail. So I suspect that this never version of Monex does now indeed require eXist-db 6.1.0.

I think all in all, this PR could be closed. The latest released version of Monex (4.0.0) works well against eXist-db 6.1.0, and the older version of Monex works well against eXist-db 5.4.0 and 6.0.1.

@line-o line-o force-pushed the build/pom-cleanup branch from f654a43 to be27814 Compare January 12, 2023 20:28
@line-o line-o changed the title [build] fix snapshot version and dependencies [build] remove extraneous dependency Jan 12, 2023
package.json Outdated Show resolved Hide resolved
@line-o
Copy link
Member Author

line-o commented Jan 12, 2023

Test failures seem to be unrelated to exist-version

@line-o
Copy link
Member Author

line-o commented Jan 12, 2023

It declares that eXist-db provides slf4j-api 1.7.33 - however eXist-db 6.1.0 does not provide that version of the slf4j-api. It provides slf4j-api 2.0.5

Did you read my posts in the release channel @adamretter ?

@adamretter
Copy link
Contributor

Did you read my posts in the release channel @adamretter ?

@line-o Yes - as you saw I also responded to them.

@line-o
Copy link
Member Author

line-o commented Jan 20, 2023

What has to be done to get this PR merged?

@adamretter
Copy link
Contributor

I think all in all, this PR could be closed. The latest released version of Monex (4.0.0) works well against eXist-db 6.1.0, and the older version of Monex works well against eXist-db 5.4.0 and 6.0.1.

@line-o
Copy link
Member Author

line-o commented Jan 23, 2023

Your concerns were adressed. Removing the extraneous dependency is really needed.

@adamretter
Copy link
Contributor

Your concerns were adressed.

Can you explain/signpost? I haven't seen that yet...

@line-o
Copy link
Member Author

line-o commented Jan 23, 2023

I think there are a number of problems with this PR at the moment:
It attempts to roll the version of the package back from 4.0.0 to 3.0.6-SNAPSHOT

Did not see that 4.0.0 was released - does not roll back version

It declares that eXist-db provides slf4j-api 1.7.33 - however eXist-db 6.1.0 does not provide that version of the slf4j-api. It provides slf4j-api 2.0.5

Does not change slf4j-api version

It declares the minimum version of eXist-db required by Monex is 5.4.0, however the CI tests executed against eXist-db 5.4.0 fail. So I suspect that this never version of Monex does now indeed require eXist-db 6.1.0.

Does not do that anymore

@adamretter
Copy link
Contributor

adamretter commented Jan 23, 2023

  1. This PR appears to still change 4.0.0-SNAPSHOT to 4.0.1-SNAPSHOT in package.json, that doesn't seem to be related to an extraneous dependency.

  2. This PR appears to still change slf4j-api from provided to compiled cope. This doesn't make sense as eXist-db provides the slf4j-api. If you change this to compiled, this means that those classes will be compiled into this Xar which is not where they should be - not to mention the version mismatch.

@line-o
Copy link
Member Author

line-o commented Jan 23, 2023

The package.json is unrelated, yes. Is it a problem? I don't think so. If you want to provide a separate PR for this, go ahead.

This PR appears to still change slf4j-api from provided to compiled cope. This doesn't make sense as eXist-db provides the slf4j-api. If you change this to compiled, this means that those classes will be compiled into Xar which is not where they should be - not to mention the version mismatch.

You must be looking at a different diff.

@adamretter
Copy link
Contributor

You must be looking at a different diff.

I am looking a the one that GitHub shows... so the same as everyone else I guess:

Screenshot 2023-01-23 at 23 37 06

@line-o
Copy link
Member Author

line-o commented Jan 23, 2023

OK, ... I'll give you a moment then.

Copy link
Member

@reinhapa reinhapa left a comment

Choose a reason for hiding this comment

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

I guess that the version bump from 4.0.0 to 4.0.1 is ok as you only remove the unneeded slf4j activation library..

@line-o
Copy link
Member Author

line-o commented Jan 24, 2023

@reinhapa the version bump is actually just a leftover from the release not handling the package.json by itself see eXist-db/documentation#876

@reinhapa reinhapa merged commit 60b95b3 into eXist-db:master Mar 1, 2023
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.

4 participants