search

clojure-emacs / cider-nrepl

A collection of nREPL middleware to enhance Clojure editors with common functionality like definition lookup, code completion, etc.
https://docs.cider.mx/cider-nrepl
670 stars 175 forks source link

[ci] Refactor CI wrt JDK8 and Clojure 1.9 compatibility #841

Closed alexander-yakushev closed 5 months ago

alexander-yakushev commented 5 months ago

Similar to https://github.com/nrepl/nrepl/pull/305

  1. Replaced exclusions with more precise matrix sets.
  2. Removed clj 1.9 together with JDK higher than 8. The motivation for it is that Clojure 1.10 contains important fixes for compatibility with Java9+ (see https://github.com/clojure/clojure/blob/master/changes.md#11-java). Anyone who cares enough about using newer JDKs must eventually upgrade Clojure as well.
vemv commented 5 months ago

Thanks!

Given that unfortunately it's red, please limit the changes to removing clj 1.8 and 1.9 from the excludes matrix.

The rest of the improvements can be added immediately afterwards.

alexander-yakushev commented 5 months ago

I have a feeling that it's red because of differences in JDKs. Let me try to drill it down.

bbatsov commented 5 months ago

@vemv As the legacy parser targets JDK 8 do we need to run its tests against anything else? Seems like another opportunity to simplify the CI matrix.

vemv commented 5 months ago

I agree it's an unlikely combination, however it's a possible one and results in extended test coverage - important for the intrincate things we do from time to time.

alexander-yakushev commented 5 months ago

So, the problem seems to be in the invocation of this method here: https://github.com/clojure-emacs/cider-nrepl/blob/master/src/cider/nrepl/middleware/slurp.clj#L57. Files.probeContentType is platform-dependent and seems to be broken in the Temurin JDK8 image for whatever reason. "Files.probeContentType returns null" is a common Google query.

Any suggestions what should I do with it? I tried to install a couple extra system libraries to the image but it didn't help. Does disabling that particular test in JDK8 sounds an acceptable solution?

alexander-yakushev commented 5 months ago

Given that unfortunately it's red, please limit the changes to removing clj 1.8 and 1.9 from the excludes matrix. The rest of the improvements can be added immediately afterwards.

I'd rather do it all in one go if you see the ultimate result as beneficial. And if I manage to get the tests passing.

vemv commented 5 months ago

Good catch, thanks! Appreciated.

Please let's operate in baby steps - let's merge the smallest diff that is green, create an issue for the File thing, etc

I'd rather do it all in one go if you see the ultimate result as beneficial

The end result will be great no doubt but I'd highly appreciate operating in an always-green, digestible manner.

I also have other duties, so smaller diffs make it easier for me to context-switch, else I'd have to wait till the weekend or so.

alexander-yakushev commented 5 months ago

Reduced the scope of this PR. Only exclusions refactored, and Clj1.8*JDK11+ testing dropped.

vemv commented 5 months ago

Looking good, thanks.

I'll have a look again at the new introduced matrix to make sure nothing was unawarely dropped

(Feel free to double-check as well - these can happen)

alexander-yakushev commented 5 months ago

I noticed that plaintest and smoketest matrices are otherwise identical, so I merged them together. Can undo if there are external reasons to keep them separate.

bbatsov commented 5 months ago

The PR looks good to me in its current state.

So, the problem seems to be in the invocation of this method here: https://github.com/clojure-emacs/cider-nrepl/blob/master/src/cider/nrepl/middleware/slurp.clj#L57. Files.probeContentType is platform-dependent and seems to be broken in the Temurin JDK8 image for whatever reason. "Files.probeContentType returns null" is a common Google query.

I've been thinking from time to time whether we should retire slurp, as it's almost a POC that we never really developed into what it was supposed to be. Without it, however, people won't be able to render images in the REPL, so it'd be nice if we came up with a simpler alternative as slurp + content-type were supposed to be generic and handle all sorts of rich data formats. (see https://github.com/clojure-emacs/cider-nrepl/pull/517)

vemv commented 5 months ago

Thanks for the refactoring!

Re: slurp, it would SGTM to disable the related test if orchard.misc/java-api-version is 8.

After all it seems pretty unlikely that someone would notice the bug (hitting that or branch + using an old temurin)