eXist-db / monex

Monitoring Application for eXist-db
GNU Lesser General Public License v2.1
6 stars 19 forks source link

[build] remove extraneous dependency #232

Closed line-o closed 1 year ago

line-o commented 1 year ago
adamretter commented 1 year ago

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 commented 1 year ago

Test failures seem to be unrelated to exist-version

line-o commented 1 year ago

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 commented 1 year ago

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

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

line-o commented 1 year ago

What has to be done to get this PR merged?

adamretter commented 1 year ago

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 commented 1 year ago

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

adamretter commented 1 year ago

Your concerns were adressed.

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

line-o commented 1 year ago

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 commented 1 year ago
  1. This PR appears to still change 4.0.0-SNAPSHOT to 4.0.1-SNAPSHOT inpackage.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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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

line-o commented 1 year ago

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