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

Use JAXB from Maven #413

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

eraserhd
Copy link

Closes #412

This should work on all the JDKs. I've tested it on 8, 10, and 11.

Closes oracle-samples#412

This should work on all the JDKs.
Copy link
Collaborator

@mrrodriguez mrrodriguez left a comment

Choose a reason for hiding this comment

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

This seems reasonable to me. I believe I heard of this approach taken elsewhere too.

@WilliamParker
Copy link
Collaborator

As I understand it the need for these dependencies comes from our (relatively old) version of ClojureScript. According to CLJS-2377 and bhauman/lein-figwheel#612 (comment) ClojureScript has now released a version that doesn't have this dependency anymore, so if Clara bumped its dependency to that ClojureScript version we wouldn't need to declare this dependency. However, we'd be blind to anything that broke Clara in older ClojureScript versions unless we did further testing. We could create an "old ClojureScript" build similar to the changes made in #358 to test against newer Clojure/ClojureScript, have that build add these dependencies, and then bump the version of ClojureScript in the primary build to a newer one. This would ensure that we maintained compatibility with older ClojureScript versions.

The reason I see for doing it this way is that as is, we'd be blind to any actual runtime dependency in the JVM version on these additional dependencies since all tests would pass and users would then be unable to use the jar (unless they added the dependencies). I'm a bit hesitant to assume that no Clara JVM dependencies, now or in the future, won't have an actual dependency on these sorts of "systems level" dependencies being provided. That said, I don't see it as a huge risk and in any event it is something that would quickly become apparent upon attempted use of Clara, so if what I'm describing won't work/has downsides I'm missing I don't mind merging this too much.

As an ancillary benefit, any performance testing we do against ClojureScript should really be against a newer version, even if we maintain compatibility against older versions.

Thoughts? I'm particularly interested in thoughts from anyone who keeps up with the ClojureScript ecosystem more than I do.

@mrrodriguez @eraserhd

@mrrodriguez
Copy link
Collaborator

mrrodriguez commented Dec 19, 2018

@WilliamParker It makes more sense to me for Clara to build against fairly modern versions of clj and cljs as the default and have a few "older" versions profiles to test out level of a minimum required version.

Perhaps something like 1.9 of clj, with some later 1.9.x of cljs as the default, then a profile for more cutting edge clj 1.10 and cljs 1.10.x and a profile for a minimum tested version we consider Clara to claim to have compatibility with.

We do have to keep in mind that cljs moves quite a bit more quickly than clj and in its fairly fast paced releases, there are sometimes things that break, but then are later resolved (fairly rare still). So If you say, min support for cljs v1.8.x, it isn't too feasible to build against all versions of cljs from that minimum upwards. Just have to test a few versions and interpolate that things seem to be fine, but haven't actually been ran at every release along the spectrum.

@eraserhd
Copy link
Author

I figure this was a good thing for me to resolve for the next release, but here is the issue:

I can update Clojure and ClojureScript in the :recent-clj profile, and then I can run the unit tests without loading new libraries for JDK11. This is good...

But then the PhantomJS tests fail, though, and it appears to be a problem with PhantomJS's ES6 support. The issue is here: ariya/phantomjs#13895 .

Then I discovered (did not know) that PhantomJS is officially dead, and Everyone Should Use Headless Chrome. So this seems like a little bit of work, including remembering how to Travis CI again.

@mrrodriguez
Copy link
Collaborator

Hi @eraserhd
I have a long running issue I keep failing to solve and then not getting back to @ #388

The CLJS implementation really needs an overhaul due to pitfalls in how it is done.
Along with this, phantomjs should stop being used in favor of something supported as you mentioned.

I think lein doo with chrome headless would be sufficient, but we need to get the Travis CI setup to work for this.

What worries me some is that locally I've ran into a lot of CLJS sort of failures doing things like compiling and testing due to the issues w/how CLJS rules are compiled via a sneaky/not-really-supported clojure.core/eval during CLJS compilation.

Perhaps I can try to revisit this yet again, or try to just do a single issue to stop using phantom and see if it still works as-is for now...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Better options for Java 9 compatibility in project.clj
3 participants