Upgrade Eastwood #188

vemv · 2021-09-03T19:31:39Z

Brief

Long due one!

Some of the changes in Eastwood that are most relevant to f-s:

New reflection linter that makes our own linter redundant
The linters that we used to disable have been thoroughly fixed and are not expected to have false positives
Increased performance and reliability for REPL invocations
New ways of silencing faults
- https://github.com/jonase/eastwood#ignored-faults
- https://github.com/jonase/eastwood#ignoring-linter-sub-kinds

QA plan

For the Eastwood changes, I'd recommend linting large projects and verifying/fixing that the newly enabled linters aren't being problematic. Please report any false positives.

For the runner change, one can (formatting-stack.core/format) and observe that the background runner keeps doing its job.

Author checklist

Reviewer checklist

vemv · 2021-09-03T19:33:39Z

.circleci/config.yml

@@ -9,7 +9,7 @@ commands:
            - v2-dependencies-{{ checksum "project.clj" }}
            # fallback to using the latest cache if no exact match is found
            - v2-dependencies-
-      - run: lein with-profile -dev,+ci,+test,+refactor-nrepl deps
+      - run: lein with-profile -user,-dev,+ci,+test,+refactor-nrepl deps


I added -user everywhere, lately I've realised that this makes commands more reproducible. It saves time that people can copy-paste commands into their terminal, resting assured that ~/.lein won't interfere.

We use this in e.g. all the important https://github.com/clojure-emacs projects.

This is a good idea, for all my 'reproducible' commands (lein test-aliases and such) I include -user too :)

vemv · 2021-09-03T19:34:41Z

.circleci/config.yml

@@ -70,6 +70,8 @@ jobs:
          command: << parameters.lein_test_command >>
      - run:
          command: .circleci/e2e.sh
+      - run:
+          command: lein with-profile -user,+test eastwood


Good to self-lint. It's not very formatting-stacky to lint all files, but it's a good idea (self-reference tends to be logically weak)

I think this is a good change, but i'd rather implement it in terms of formatting-stack. I did some work to support cli invocation.

But i'd be fine with replacing this CI step when that's ready.

I'd suggest having both for the hinted reason: using f-s to lint f-s is somewhat weak. It can self-report being OK while it's not.

One would run the self-linting before Eastwood though, for fast feedback.

This could be an entire sequence:

f-s self-lint (kondo + eastwood, git diff only)

clj-kondo, whole project

eastwood, whole project

Maybe a little annoying for our standards of fast CI linting, but it could catch an issue the test suite didn't

In my suggestion it'd run the linter across the entire project as well. But we'll cross that bridge when we get there.

There could be an issue in strategies, in reporting, in interaction with a given linter impl... bunch of things that can go wrong

vemv · 2021-09-03T19:36:02Z

project.clj

@@ -131,14 +134,12 @@
                                     ;; (and shipped with this refactor-nrepl):
                                     :plugins      [[cider/cider-nrepl "0.24.0"]]}

-             :parallel-eastwood     {:jvm-opts ["-Dformatting-stack.eastwood.parallelize-linters=true"]}


Eastwood already parallelizes linters now

(f-s' own testing aided that decision!)

vemv · 2021-09-03T19:36:54Z

src/formatting_stack/linters/eastwood/impl.clj

@@ -5,45 +5,6 @@
   [formatting-stack.protocols.spec :as protocols.spec]
   [nedap.speced.def :as speced]))

-(speced/defn ^boolean? contains-dynamic-assertions?


This fix made its way into Eastwood ~1y ago

vemv · 2021-09-03T19:38:10Z

worker/formatting_stack/background.clj

+    (-> f
+        Thread.
+        ;; Important - daemonize this thread, otherwise under certain conditions it can prevent the JVM from exiting.
+        ;; (We exercise this implicitly via `lein eastwood` in CI)


As the comment hints, the thing being fixed here is that a non-daemonized thread can leave the JVM hanging forever.

I certainly have experieced that with f-s a bunch of times, it's very annoying.

vemv · 2021-09-03T19:38:26Z

worker/formatting_stack/kondo_classpath_cache.clj

@@ -24,10 +24,10 @@
  (apply list runner-choice body))

 ;; Use kondo's official default config dir, so that we don't bloat consumers' project layouts:
-(def cache-parent-dir ".clj-kondo")
+(def ^String cache-parent-dir ".clj-kondo")


Fixes found by Eastwood

vemv · 2021-09-03T19:48:27Z

test/functional/formatting_stack/linters/eastwood.clj

@@ -27,11 +27,11 @@

      "test-resources/eastwood_warning.clj"
      (matchers/in-any-order
-       [{:source              :eastwood/warn-on-reflection


Maybe the linter rename is a breaking change

OTOH any Eastwood upgrade can bring new warning types (e.g. :eastwood/boxed-math) so it to be expected that types can appear/disappear over time

Well, it would if we had silencing / blacklisting implemented (see #132). So I think this'll be fine :)

thumbnail · 2021-09-03T20:11:07Z

src/formatting_stack/linters/eastwood.clj

@@ -53,8 +45,7 @@
                            :filename            (if (string? uri-or-file-name)
                                                   uri-or-file-name
                                                   (-> ^File uri-or-file-name .getCanonicalPath)))))
-         (concat (impl/warnings->reports output)


I really like that the output-parsing is gone now 😬 !

vemv

Some notes to self, will fix along any feedback

src/formatting_stack/linters/eastwood.clj

vemv · 2021-09-07T12:08:42Z

Amended to use into

vemv · 2021-09-08T19:17:57Z

Removed the redundant config file. Strangely that revealed an unrelated failing test. I got to fix it by moving it to a different ns, prevening the git-cloning fixture from being run for it.

thumbnail · 2021-09-09T11:48:55Z

Strangely that revealed an unrelated failing test.

Yeah I looked into this for a bit and am not sure what is actually happening. Moving the test out of this test-ns works but doesn't fix the root cause (maybe the refresh-within-refresh-dirs-only-strategy doesn't work properly in the other git folder?). I'll look into this a bit further.

vemv · 2021-09-09T11:55:23Z

The fixture was making all-files (sut/all-files :files []) return files such as these:

/home/circleci/repo/git-integration-testing/src/formatting_stack/formatters/cljfmt.clj

whereas they were expected to be like this:

/home/circleci/repo/src/formatting_stack/formatters/cljfmt.clj

...so it seems to me that sut/namespaces-within-refresh-dirs-only is doing its job properly as git-integration-testing/src is not a refresh dir.

thumbnail · 2021-09-10T07:39:08Z

That makes sense; the test-fixture sets a different CWD to make sure the git-commands run in the right CWD; but that obviously doesn't work for the refresh dirs.

I'm currently checking why the test succeeded before a805eb0

edit: it seems to have found specifically (/Users/.../formatting-stack/git-integration-testing/resources/eastwood/config/formatting_stack.clj). Because that file doesn't have a namespace declaration 😅. That test was truly hanging by a thread.

Anyway; I'm not a big fan of the test being moved out of the 'idiomatic' namespace, we could just set refresh-dirs to a value during the test. This also makes sure we control the 'hidden' variable (refresh-dirs that is):

diff --git a/test/integration/formatting_stack/strategies.clj b/test/integration/formatting_stack/strategies.clj
index 01d0dbc..b7be982 100644
--- a/test/integration/formatting_stack/strategies.clj
+++ b/test/integration/formatting_stack/strategies.clj
@@ -5,6 +5,7 @@
    [clojure.set :as set]
    [clojure.string :as string]
    [clojure.test :refer [deftest is testing use-fixtures]]
+   [clojure.tools.namespace.repl :refer [refresh-dirs]]
    [formatting-stack.strategies :as sut]
    [formatting-stack.test-helpers :as test-helpers :refer [git-integration-dir]]
    [formatting-stack.util :refer [rcomp]]
@@ -231,16 +232,18 @@
         (cleanup-testing-repo!)))))
 
 (deftest namespaces-within-refresh-dirs-only
-  (speced/let [^{::speced/spec (rcomp count (partial < 100))}
-               all-files (sut/all-files :files [])
-               result (sut/namespaces-within-refresh-dirs-only :files all-files)]
-    (is (seq result)
-        "Returns non-empty results (since f-s itself has namespaces within `src`, `test`, etc)")
-
-    (is (< (count result)
-           (count all-files))
-        "Doesn't include files outside the refresh dirs")
-
-    (is (set/subset? (set result)
-                     (set all-files))
-        "Is a subtractive (and not additive) mechanism")))
+  (with-redefs [refresh-dirs [(str (io/file git-integration-dir "src"))
+                              (str (io/file git-integration-dir "test"))]]
+   (speced/let [^{::speced/spec (rcomp count (partial < 100))}
+                all-files (sut/all-files :files [])
+                result    (sut/namespaces-within-refresh-dirs-only :files all-files)]
+     (is (seq result)
+         "Returns non-empty results  (since f-s itself has namespaces within `src`, `test`, etc)")
+
+     (is (< (count result)
+            (count all-files))
+         "Doesn't include files outside the refresh dirs")
+
+     (is (set/subset? (set result)
+                      (set all-files))
+         "Is a subtractive (and not additive) mechanism"))))

vemv · 2021-09-13T03:18:23Z

Rebased and added a minor commit

Context: nedap#188 (comment)

vemv · 2021-09-13T18:39:46Z

Added a commit following PR feedback, and removed the old one

thumbnail

Seems good to me! Left 2 comments but nothing mayor.

thumbnail · 2021-09-15T13:04:55Z

src/formatting_stack/linters/eastwood.clj

-                         (catch Exception e
-                           (swap! exceptions conj e)))))]
+        exceptions (atom nil)]
+    (with-out-str


formatting-stack.util/silence might be helpful here, since you're not capturing the output:

formatting-stack/src/formatting_stack/util.clj

Line 154 in 6ca2d6b

(defmacro silence

thumbnail · 2021-09-15T13:05:52Z

src/formatting_stack/linters/eastwood.clj

@@ -12,36 +12,22 @@
   (java.io File)))

 (def default-eastwood-options
-  ;; Avoid false positives or undesired checks:
-  (let [linters (remove #{:suspicious-test :unused-ret-vals :constant-test :wrong-tag}


Are all of these linters currently disabled by default in eastwood? What if they'll be re-enabled in the future?

Are all of these linters currently disabled by default in eastwood? What if they'll be re-enabled in the future?

They've been fixed upstream (i.e. they shouldn't give false positives) so there's no further need of disabling them.

(Which is why QAing a few large projects is recommended)

Context: nedap#188 (comment)

vemv · 2021-09-15T14:59:40Z

Rebased again, now we have 3 commits. The first one includes thesilence feedback

thumbnail · 2021-09-16T09:25:25Z

Thanks a lot for the patience and the work!

vemv commented Sep 3, 2021

View reviewed changes

thumbnail reviewed Sep 3, 2021

View reviewed changes

vemv commented Sep 4, 2021

View reviewed changes

src/formatting_stack/linters/eastwood.clj Outdated Show resolved Hide resolved

src/formatting_stack/linters/eastwood.clj Outdated Show resolved Hide resolved

vemv force-pushed the upgrade-eastwood branch from cbbbf20 to 79397ba Compare September 7, 2021 12:08

vemv force-pushed the upgrade-eastwood branch from 655b8ef to 85d5e9f Compare September 13, 2021 03:17

vemv added a commit to reducecombine/formatting-stack that referenced this pull request Sep 13, 2021

Fix a flaky test

613bef6

Context: nedap#188 (comment)

vemv force-pushed the upgrade-eastwood branch from 85d5e9f to 613bef6 Compare September 13, 2021 18:35

thumbnail self-requested a review September 13, 2021 20:02

thumbnail approved these changes Sep 15, 2021

View reviewed changes

vemv added 3 commits September 15, 2021 16:58

Upgrade Eastwood

e318b4f

Add a name to the background/runner thread

710ed8f

Fix a flaky test

165edfb

Context: nedap#188 (comment)

vemv force-pushed the upgrade-eastwood branch from 613bef6 to 165edfb Compare September 15, 2021 14:58

thumbnail merged commit df434cc into nedap:master Sep 16, 2021

thumbnail mentioned this pull request Oct 14, 2021

v4.4.0 #195

Merged

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upgrade Eastwood #188

Upgrade Eastwood #188

vemv commented Sep 3, 2021 •

edited

Loading

vemv Sep 3, 2021 •

edited

Loading

thumbnail Sep 3, 2021

vemv Sep 3, 2021

thumbnail Sep 7, 2021

vemv Sep 7, 2021

thumbnail Sep 7, 2021

vemv Sep 7, 2021

vemv Sep 3, 2021

vemv Sep 3, 2021

vemv Sep 3, 2021

vemv Sep 3, 2021

vemv Sep 3, 2021

thumbnail Sep 3, 2021

thumbnail Sep 3, 2021

vemv left a comment

vemv commented Sep 7, 2021

vemv commented Sep 8, 2021

thumbnail commented Sep 9, 2021

vemv commented Sep 9, 2021

thumbnail commented Sep 10, 2021 •

edited

Loading

vemv commented Sep 13, 2021

vemv commented Sep 13, 2021

thumbnail left a comment

thumbnail Sep 15, 2021

thumbnail Sep 15, 2021

vemv Sep 15, 2021

vemv commented Sep 15, 2021

thumbnail commented Sep 16, 2021

Upgrade Eastwood #188

Upgrade Eastwood #188

Conversation

vemv commented Sep 3, 2021 • edited Loading

Brief

QA plan

Author checklist

Reviewer checklist

vemv Sep 3, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vemv left a comment

Choose a reason for hiding this comment

vemv commented Sep 7, 2021

vemv commented Sep 8, 2021

thumbnail commented Sep 9, 2021

vemv commented Sep 9, 2021

thumbnail commented Sep 10, 2021 • edited Loading

vemv commented Sep 13, 2021

vemv commented Sep 13, 2021

thumbnail left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vemv commented Sep 15, 2021

thumbnail commented Sep 16, 2021

vemv commented Sep 3, 2021 •

edited

Loading

vemv Sep 3, 2021 •

edited

Loading

thumbnail commented Sep 10, 2021 •

edited

Loading