Make Monex compatible with eXist-db 6.x.x and 7.x.x #298
Conversation
|
@adamretter Thanks for the PR. Lots to unpack here. We actually decided to not maintain a single version compatible with v6 and v7, have a separate branch for work on the v7 release see #285 . The command to have cypress talk to another port would be to add |
adamretter
force-pushed
the
feature/elemental-existdb-compatibility
branch
from
6188742 to
8a42507
Compare
@duncdrum Will that be re-considered? IMHO Having two separate versions, branches and releases doesn't make a lot of sense when 6.x.x and 7.x.x are almost identical, and it will cost more you with a higher maintenance overhead.
It looks like the Monex v7 branch is very incomplete still and doesn't yet work with eXist-db 7.0.0-SNAPSHOT. IMHO It's better to take this one which is simpler and is ready to go now. |
adamretter
force-pushed
the
feature/elemental-existdb-compatibility
branch
from
8a42507 to
fb0bda3
Compare
adamretter
force-pushed
the
feature/elemental-existdb-compatibility
branch
from
fb0bda3 to
9dfa93a
Compare
There was a problem hiding this comment.
LGTM, Thanks @adamretter for PR, the CI changes are much appreciated.
I have built and tested with locally with exist-db 6.1.0 and 6.4.0 and it working well.
I do hope we can get the CI to green as soon as possible, and keep it that way from now on.
adamretter
force-pushed
the
feature/elemental-existdb-compatibility
branch
from
9dfa93a to
ff21a67
Compare
|
@adamretter regarding compatibility, you made quite some changes in the JMX servlet. Are the proposed changes compatible with eXist? |
@dizzzz Can you be more specific please... which changes are you referring to exactly?
Yes, as already discussed here - #298 (comment) |
|
@adamretter if you may recall, you made changes in the JMX servlet recently. |
There was a problem hiding this comment.
Ok I m not even half-way through the changes.
- Tests are failing for v7, since that is the whole point of this PR, that's a blocker.
- This PR could be very much simplified, if it wouldn't try to combine v6 and v7. Which we already decided not to do, even after reconsideration. I suggest focusing on the changes from
db-api-7which are necessary for v7 and target the WIP branch I already mentioned. If there is a need fordb-api-6changes let's deal with them separately. - Any declared processor dependency must also be tested. Irrespective of processor. I m happy to discuss options here. But as it stands elemental v6 and exist v7 are declared but not tested. This is another blocker.
- The introduction of multiple parent poms increases the maintenance burden, we should simplify not add complexity.
- While we are patiently waiting for a response to the license issue around elemental
- Demanding PR reviews on slack is not helping
|
|
||
| ############################## | ||
| ## Node / JS | ||
| ############################## |
There was a problem hiding this comment.
We really don't want to include videos and screenshots in the repo
There was a problem hiding this comment.
@duncdrum They are not included in the repo. In this PR instead there is a bugfix so that instead of writing them into the src folder, we corrected the config so they are written into the target folder (which is excluded in .gitignore)
|
|
||
| <properties> | ||
| <!-- Version of eXist-db and Elemental 6.x.x that Monex is compatible with --> | ||
| <db.api.6.version>6.0.0</db.api.6.version> |
There was a problem hiding this comment.
Undocumented version, where to I find the api version of exist 6.3.0? see my earlier comment about not maintaining a single version compatible with different major versions of exist
There was a problem hiding this comment.
Undocumented version, where to I find the api version of exist 6.3.0
@duncdrum This is the Java API for eXist-db 6.0.0! You can find it here: https://github.com/eXist-db/exist/tree/eXist-6.0.0/exist-core/src/main/java/org/exist
The Java API does not break between major Semver versions of eXist-db, and so eXist-db Java API version 6.0.0 should be stable and applicable to all 6.x.x releases of eXist0db.
There was a problem hiding this comment.
@adamretter my point exactly, a source-code collection of 1704 items for users to dig through counts as undocumented. Especially when api version, Java API, nor 6.0.0 appear in that collection of items in a relevant context.
There was a problem hiding this comment.
@duncdrum I am sorry, but I cannot understand what you are saying.
How is the API of eXist-db undocumented exactly? There is Javadoc on all public methods, and that Javadoc is also published every time a release of eXist-db is made.
What is the concrete problem with documentation that you see here in this PR specifically (that did not already exist before this PR)?
| <properties> | ||
| <!-- Version of eXist-db and Elemental 6.x.x that Monex is compatible with --> | ||
| <db.api.6.version>6.0.0</db.api.6.version> | ||
| <db.api.6.impl.version>6.1.0</db.api.6.impl.version> |
There was a problem hiding this comment.
s.a how is this different from the one before, where is is defined, and why do we need it in addition to what comes after
There was a problem hiding this comment.
@duncdrum This is the XQuery Implementation for eXist-db 6.1.0! You can find it here: https://github.com/eXist-db/exist/tree/eXist-6.0.0/exist-core/src/main/java/org/exist/xquery
@duncdrum This is the minimum version of eXist-db that the XQuery code of Monex is expected to run on, i.e. Monex requires features that were introduced in eXist-db 6.1.0.
It is different to the Java API version, because:
- the Java API version of eXist-db is stable across the entire 6.x.x line
- version 6.0.0 did not have some XQuery function that Monex now seems to require.
These two version numbers of Java API and XQuery features have always been present in Mondex, see: https://github.com/eXist-db/monex/blob/master/pom.xml#L51
| <version>1.4.01</version> | ||
| </dependency> | ||
| <dependency> | ||
| <!-- NOTE(AR) Could be swapped out for eXist-db 7.0.0 if it is released, but leaving it as Elemental does no harm either --> |
There was a problem hiding this comment.
that's why we publish snaphshots to the GitHub maven registry
There was a problem hiding this comment.
@duncdrum That's fine to publish snapshots, but it won't help here! You cannot feasibly publish a release version of a Maven artifact that contains a dependency that declares a SNAPSHOT version; therefore by extension, there would be no value in testing in CI against a SNAPSHOT version.
that's why we publish snaphshots to the GitHub maven registry
Also, but a separate issue, FYI - that hasn't been updated for 5 months now (since January 03 2025) - https://github.com/eXist-db/exist/packages/2360109
| <version>1.12.0</version> | ||
| <relativePath /> | ||
| <groupId>org.exist-db.apps</groupId> | ||
| <artifactId>monex-parent</artifactId> |
There was a problem hiding this comment.
I don't understand how introducing a Monex-parent that depends on the exist-apps-parent is actually helping. Sounds like a maintenance burden to me.
There was a problem hiding this comment.
I don't understand how introducing a Monex-parent that depends on the exist-apps-parent is actually helping.
This is a large topic... It might help if you read up on:
- Maven Parent POM
- Multi-module reactor projects
- Good Maven Project design principles
Sounds like a maintenance burden to me.
@duncdrum Actually it is the opposite. Rather than having to have 5 copies (in this project - there are 5 modules) of the same code, you need only have that code once, and include it. This requires 4x less maintenance, and 4x less chance that things get out of sync (i.e. updated in one place and not another).
| </plugin> | ||
| </plugins> | ||
| </build> | ||
| NOTE(AR) Could be swapped out for eXist-db 7.0.0 if it is released, but leaving it as Elemental does no harm either |
There was a problem hiding this comment.
Testing against elemental while ignoring the :latest images which we use for testing since exist v5 makes no sense, and is a showstopper for me.
There was a problem hiding this comment.
@duncdrum I don't think testing against latest (i.e. a SNAPSHOT makes any sense here), each version of Monex, even a version that is still being developed needs to be targeting a stable version of eXist-db. It's also kind of related to the earlier explanation I gave here - #298 (comment)
If you want to have a CI build target that is also checking if Monex still works against an upcoming SNAPSHOT of eXist-db, that would be fine, but it should be an additional CI job, with IMHO a soft-fail as you are trying to hit a moving target.
|
|
||
| <profile> | ||
| <!-- | ||
| Cypress is used for integration tests, this profile enables to run Cypress and Elemental 7.x.x with recording support (see cypress.io), e.g. for CI. |
There was a problem hiding this comment.
As above - #298 (comment)
.github/workflows/ci.yml
Outdated
| path: target/ | ||
| retention-days: 1 | ||
|
|
||
| test6: |
There was a problem hiding this comment.
test6 and test7 can run in parallel and should be matrix expanded.
| @@ -1,12 +1,13 @@ | |||
| { | |||
| "baseUrl": "http://localhost:8080/exist", | |||
| "baseUrl": "http://localhost:10086/exist", | |||
There was a problem hiding this comment.
whats wrong with 8080? Every single app can be tested against exist's default port, why change it?
There was a problem hiding this comment.
@duncdrum Three good reasons:
-
When running the integration tests, if you already have eXist-db running on your system elsewhere (as many of us do), then the tests fail as eXist-db in Docker for this project cannot be started on the same port
-
You cannot use the same port twice, if you want to run the Integration Tests in parallel, which is exactly what happens if you are using a modern concurrent enabled version of Maven, then the two integration tests can collide as they try to use the same port.
-
It is generally accepted good practice to use a different port during integration testing so it doesn't clash with any other instances. Normally this would be the next free randomly assigned port, but there isn't a good mechanism to use a random port number with the docker-maven-plugin. We have taking this approach for a long time in the main eXist-db project (for an example, see: https://github.com/eXist-db/exist/blob/develop/exist-core/src/main/java/org/exist/test/ExistWebServer.java#L126C74-L126C86)
There was a problem hiding this comment.
We test all stock apps against the default ports. I'm quite aware of how this works. Thanks for the three paragraph explanation. Random ports hidden inside maven plugins will make testing this much more cumbersome, and inconsistent with the other apps. If you have an exist running on 8080 simply install what you want to test there. The problem you are painting doesn't exist.
There was a problem hiding this comment.
I'm quite aware of how this works. Thanks for the three paragraph explanation.
You asked two questions:
- "whats wrong with 8080?"
- "Every single app can be tested against exist's default port, why change it?"
I answered your questions! If you were aware of how this works and didn't want an answer, why did you ask?
Random ports hidden inside maven plugins will make testing this much more cumbersome
This is no more or less hidden that what we already have, it's just a port number configured in the same place as before. The difference is that this is now non-conflicting ports, and can be configured via an external parameter which it previously could not be!
The problem you are painting doesn't exist.
Maybe that is true only for you.
|
@duncdrum I have addressed your commons on the code directly above where they appeared. The other outstanding questions/comments you posed in your most recent review, I can answer here:
Yes, we already noted that at the start of the PR here. This issue is one of 'chicken and egg':
If it helps to satisfy CI. It could be possible, but not desirable, to release the next version of Elemental right now with the same broken Monex, watch the CI go green for this PR and get merged, and then immediately release another newer version of Elemental with an updated Monex that includes our contributions. To do that we would need some sort of assurance from the Monex project that they would incorporate our changes and create a release in a timely manner.
There has been no explanation about this. We have questions:
The issue as we see it is, there are known bugs in eXist-db 6.4.0 and a 6.5.0 would be desirable for many users. There are also known bugs in Monex on eXist-db 6.4.0 (some of which have already been fixed but not yet released). The Java code component of Monex is tiny in comparison to the XQuery and JavaScript code. Therefore it would seem to make sense to have a single code base where the largest part (XQuery and JavaScript) is compatible with multiple versions of eXist-db, and the small version specific bit (Java) is pluggable.
The branch I think you are referring to is now 7 months old without activity. It doesn't have any meaningful changes to help move to eXist-db 7.0.0-SNAPSHOT. I would suggest instead that that branch should be closed as abandoned. This branch should superceede it as it is (a) complete, (b) works today, and *(c) could be merged and released - which would benefit all eXist-db users.
As above, the amount of Java code is minimal compared to the amount of XQuery and JavaScript code, so having multiple branches with multiple copies of the XQuery and JavaScript code where you need to make fixes and add features in one or more branches seems like unnecessary overhead. What happens when you get to eXist-db 8.0.0-SNAPSHOT? Do you plan to then maintain 3 separate branches of the same XQuery and JavaScript code due to some very small and isolated Java changes?
I am not quite sure what you mean. This has been tested. Do you mean it has to also pass CI here. If we can get this PR merged and released, we can then release Elemental 7.2.0 very quickly (same hour?), which would make CI pass here.
I am not sure if you have this backwards, or I am misunderstanding what you are saying?
I think there may be confusion around what Parent POMs are and how they work. I have tried to explain above in your review comment on this that their entire raison-d-etre is to reduce maintenance burden and complexity. All Maven multi-module projects have a parent pom (technically all Maven projects have a Parent POM (that is implicit)).
I think you might be referencing the purported licensing concern raised about the as yet unreleased Elemental 8.x.x. Please explain why that 'license issue' has any bearing on these contributions to Monex.
We have made no such demands. |
|
Aside of the group-id/artifact-id in the parent (which has been addressed), I have the following remarks.
|
@dizzzz I have now addressed that in this commit 0b3dc65 which requires this PR: eXist-db/exist-apps-parent#93
@dizzzz I did consider the two options here:
I went with (2) as it makes it easier to locate the correct class by name when searching from the CLI or an IDE. If you prefer though, I can switch to (1) - just let me know to confirm which way you want to go please?
@dizzzz 7.0.0-SNAPSHOT is using Jetty 11. That can be tested here. What do you see the issue as? |
NOTE - This first requires a version 2.0.0 of [exist-apps-parent](https://github.com/eXist-db/exist-apps-parent) to be produced by merging the PR - [eXist-db/exist-apps-parent#93](eXist-db/exist-apps-parent#93) and creating a release of it.
adamretter
force-pushed
the
feature/elemental-existdb-compatibility
branch
2 times, most recently
from
81cee54 to
fa89a0e
Compare
adamretter
force-pushed
the
feature/elemental-existdb-compatibility
branch
2 times, most recently
from
85d5880 to
2d0a190
Compare
adamretter
force-pushed
the
feature/elemental-existdb-compatibility
branch
from
2d0a190 to
26d63b2
Compare
|
exist 7 is failing on CI |
|
@duncdrum That's because eXist-db 7.0.0-SNAPSHOT needs some bug fixes to make it work again with Monex. We already made those in Elemental 7.2.0, you can take those if you like. |
|
@adamretter I m not sure how I would take bug fixes to make your pr compatible with exist 7. Bug fixes are of course always welcome. |
@duncdrum It's linked in the updates from GitHub above! You simply cherry-pick the commits from this PR and include our LGPL 2.1 license header in each file you change: evolvedbinary/elemental#10
As explained before, due to reasons outside of our control, we (Evolved Binary) will no longer contribute code directly to eXist-db, only to the Apps and Supporting Libraries. |
There was a problem hiding this comment.
There are still problems:
- it doesn't run on
:latestaka exist7. As long as it doesn't there is nothing to review. - random ports break convention with all other apps. Testing and will make testing harder
- Why was our copyright changed to 2017?
- Node version should be
22 - I still consider the combined v6 v7 build overly complex and a maintenance nightmare.
|
@adamretter do you really think it is within your control to tell me to go cherry pick some changes from elemental for your PR? In case you missed it, there are some concerns about its licensing from our side. |
@duncdrum No one said it was within my control, nor have I claimed that! You wrote: "I m not sure how I would take bug fixes". As you said you were not sure how to do it, I simply tried to provide a clear technical answer to help you do what you seemed to be asking how to do. I don't understand the hostility here! The fixes are there and free for you to take, take them or leave them - it is of course the choice of the Monex developers!
I believe the concerns from "your side" were around the licensing in Elemental 8.x.x, right? The PR I linked is for Elemental 7, which is under the same LGPL license you are using here. So what is the licensing concern that you have with Elemental 7 exactly? |
@duncdrum Okay... who will fix eXist-db 7 to make it work? I have already offered the code under the LGPL license...
That convention is broken.
This is unsubstantiated. Please provide a technical explanation of exactly how it becomes harder? What is the workflow that this makes more difficult?
It should not have been. That was a mistake in an earlier PR caused by an issue in the
Changing the Node version is out of scope for this PR. It can be done elsewhere in a new PR.
Two weeks ago at the Community Call, there was a Demo of this, and a Vote held. I am sorry but everyone else disagreed with you, and you were outvoted. As a group, it was decided to move forward with this - so let's do that please! |
This PR seems to be blocked now. I would have at least expected an actionable issue description on exist-db. |
@line-o I am not clear what you mean... can you explain please? |
| </dependency> | ||
| <dependency> | ||
| <!-- NOTE(AR) (Inherited from monex-parent) Could be swapped out for eXist-db 7.0.0 if it is released, but leaving it as Elemental does no harm either --> | ||
| <groupId>xyz.elemental.fork.org.exist-db</groupId> |
There was a problem hiding this comment.
This specific line took me/us hours to find out that this is actually eXist-6 what's used to build/link here, not something in the direction of v7;
It gives a runtime Type conflict in exist v7 (and yes the snapshot is there for quite some time already!)
In this context, I find the PR far too complex to review and understand.... :-(
There was a problem hiding this comment.
7.0.0-SNAPSHOT build is published here https://github.com/eXist-db/exist/packages/2360109
There was a problem hiding this comment.
This specific line took me/us hours to find out that this is actually eXist-6 what's used to build/link here, not something in the direction of v7;
@dizzzz I don't understand - this has nothing to do with eXist-db or Elemental v6. Can you explain a bit more please?
It gives a runtime Type conflict in exist v7 (and yes the snapshot is there for quite some time already!)
What version of eXist-db 7.0.0-SNAPSHOT are you using? There is no conflict with the latest version of eXist-db 7.0.0-SNAPSHOT.
There was a problem hiding this comment.
My report is actually complete. It is all about a Type conflict.
|
When testing this branch locally I found the hard-coded port number in the base path to be very inconvenient. Especially since it was not the standard port number 8080. I propose that the cypress tests read the port from an environment variable that can be set per run but they default to 8080. |
@dizzzz Yes! As already stated here - there is a bug in eXist-db 7.0.0-SNAPSHOT that stops it working correctly with Monex (as it is today in the last release of Monex). This was previously fixed in Elemental. Someone in the eXist-db team needs to fix that in eXist-db |
@line-o With this PR it can already be set from the command line to anything you want! |
6 participants
I worked out how to have a single codebase for Monex and make it work with eXist-db 6.x.x and 7.0.0-SNAPSHOT (and therefore by extension Elemental 6.x.x and 7.x.x).
All tests are passing against 6.x.x and 7.x.x.
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.
NOTE PR #303 should be merged to
masterfirst, and then this PR should be rebased atopmasterbefore being re-pushed and merged.NOTE The
CI / Integration Test (DB API 7.x.x)currently fails here as the latest SNAPSHOT of Elemental is not available to CI yet, but if you build and test locally you will see this does also pass.Closes #293
Closes #285