-
-
Notifications
You must be signed in to change notification settings - Fork 10
Fix Java issues on eXist-db 6.x.x #87
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
base: main
Are you sure you want to change the base?
Fix Java issues on eXist-db 6.x.x #87
Conversation
950fb46 to
1b5325f
Compare
|
@reinhapa I also took care of CI in here too. |
line-o
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR changes the parent POM to
<groupId>xyz.elemental.expath</groupId>
<artifactId>elemental-expath-package-parent</artifactId>
<version>1.0.0</version>
Where can this new "dependency" be reviewed?
@line-o Yes, unfortunately there are quite a few issues with the previous parent that was used (https://github.com/exist-db/exist-apps-parent). So I started from scratch and created the more modern
@line-o Same place as all the others:
|
|
here is the GitHub link https://www.github.com/evolvedbinary/elemental-expath-package-parent I am strongly in favour of fixing / changing the exist-db apps parent POM for packages hosted here. But I do know of at least one really big issue: It declares OSS-sonatype as its parent POM. |
1b5325f to
5dee06b
Compare
@line-o That is just one of several problems in there. Honestly, it was easier to just start again. There is nothing Elemental specific in the
@line-o Nobody should be doing that any more as the oss-parent has been deprecated now for many years and is unmaintained.
@line-o Yes, that is where almost all Open Source jar files are published. It is where developers expect to find libraries and is the default in the JVM ecosystem. It is also where all of the eXist-db releases have been published since 5.x.x. What is the problem? |
|
@adamretter @line-o why do we need a parent POM in the first place? I think this introduces unnecessary coupling. I see that only in a multi-module scenario within the same repository. |
@reinhapa Yes. Without it you end up copying and pasting a lot of config into each individual project's pom.xml file, which then over time become out of sync with each other and you have an even larger mess to maintain. |
aljopainter
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Quite a few changes that look good and simplify things. Tests working. Thanks.
|
Thanks @aljopainter for your review and kind feedback :-) |
marmoure
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM :) Tests are passing locally and on CI, the build is working.
Thanks @adamretter
5dee06b to
5088012
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The change of this file seems unrelated to the Java issues...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@reinhapa it is related to the packaging, assembly, and test changes. It simply reflects that the module now works on eXist-db and Elemental. Where do you see an issue please?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@adamretter I see mainly the addition of Elemental, except to be the reference to the version >= 6.0.0 for eXist-db (and in this case may be Elemental with the same version)
I appreciate your work, but those changes, are one of those things, that will make me hesitate to approve. Or would you allow eXist-db reference additions on Elemental in one of your repositories?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@reinhapa Thanks :-) I'm not sure if I understand your question/concern or not though. This PR has two concerns:
- Fix the module and make it work correctly with eXist-db 6 (including passing CI)
- Add a minimal processor declaration to the XAR descriptor so that it also works with Elemental.
Adding the processor declaration for Elemental means that this App supports both eXist-db and Elemental.
This is intentional, as I don't think it makes sense for Elemental to fork all of the eXst-db Apps. IMHO, it makes more sense where possible to have a single code base for an App that benefits from improvements by both eXist-db and Elemental developers contributing to it.
would you allow eXist-db reference additions on Elemental in one of your repositories
If we had a repo that contains an App or Library that is compatible with Elemental and eXist-db, then we would be happy for it to also contain a processor declaration for eXist-db and have that mentioned in the README.md.
I'm not sure if that answers your question/concern? If not, let me know and we can go from there...
| declare | ||
| %test:name("Hash binary with wrong algorithm, default format") | ||
| %test:assertError("err:CX21: The algorithm is not supported.") | ||
| %test:assertError("crypto:unknown-algorithm") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The error code that is raised is changed looking at the test. This would constitute a breaking change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's likely not the only change. The release of this will need to bump the major version number.
| declare | ||
| %test:name("Validate enveloped digital signature") | ||
| %test:assertTrue | ||
| %test:pending("Need to find a way to load the keystore.ks correctly from the filesystem") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To my understanding this test did pass in earlier versions. What changed so this test has to be skipped?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have you looked in detail at what this was actually testing previously?
…ignature() was not compatible with eXist-db 6.0.0 or newer Closes eXist-db#64
5088012 to
18391e7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@adamretter I see mainly the addition of Elemental, except to be the reference to the version >= 6.0.0 for eXist-db (and in this case may be Elemental with the same version)
I appreciate your work, but those changes, are one of those things, that will make me hesitate to approve. Or would you allow eXist-db reference additions on Elemental in one of your repositories?
The Java code previously did not compile against eXist-db 6.x.x. This PR fixes that.
Once we have 6.x.x working again, we would hope for a release of the Crypto module. After that we will backport code from our fork for Elemental to get this also working against eXist-db 7.0.0-SNAPSHOT, and then another release could be made...
NOTE - This first requires a version 2.0.0 of exist-apps-parent to be produced by merging the PR - eXist-db/exist-apps-parent#93 and creating a release of it.
For the first time in a very long time (3+ years?), now all of the tests pass 100% :-)
Closes #64
Closes #86
Additionally:
Closes #88
Closes #85
Closes #84
Closes #83
Closes #82
Closes #81
Closes #80
Closes #79
Closes #78
Closes #77