benedekfazekas / mranderson

Dependency inlining and shadowing
Eclipse Public License 1.0
151 stars 14 forks source link

Don't rewrite CLJ/CLJC files twice #71

Closed r0man closed 2 years ago

r0man commented 2 years ago

Hello,

I would like to use Instaparse in Orchard and Cider as a dependency to parse stacktraces formatted by Aviso, Clojure and Java, and show them in the Cider stacktrace inspector.

In order to include Instaparse into the Cider middleware I think it must be shipped as a inlined dependency produced via mranderson.

Unfortunatly, mranderson wasn't able to inline Instaparse. It replaced namespace declarations in some of the files twice. The Instaparse JAR looks like this:

drwxrwxrwx      0  21-Sep-2022 11:03:02  instaparse/
-rw-rw-rw-    461  21-Sep-2022 10:43:54  instaparse/viz.cljs
-rw-rw-rw-    579  21-Sep-2022 10:43:54  instaparse/util.cljc
-rw-rw-rw-    647  21-Sep-2022 11:03:02  instaparse/util.clj
-rw-rw-rw-    679  21-Sep-2022 10:43:54  instaparse/macros.clj
-rw-rw-rw-    954  21-Sep-2022 10:43:54  instaparse/combinators.cljc
-rw-rw-rw-   1029  21-Sep-2022 11:03:02  instaparse/combinators.clj
-rw-rw-rw-   1973  21-Sep-2022 10:43:54  instaparse/reduction.cljc
-rw-rw-rw-   2046  21-Sep-2022 11:03:02  instaparse/reduction.clj
-rw-rw-rw-   2795  21-Sep-2022 10:43:54  instaparse/transform.cljc
-rw-rw-rw-   2868  21-Sep-2022 11:03:02  instaparse/transform.clj
-rw-rw-rw-   2893  21-Sep-2022 10:43:54  instaparse/failure.cljc
-rw-rw-rw-   2964  21-Sep-2022 11:03:02  instaparse/failure.clj
-rw-rw-rw-   3560  21-Sep-2022 10:43:54  instaparse/print.cljc
-rw-rw-rw-   3629  21-Sep-2022 11:03:02  instaparse/print.clj
-rw-rw-rw-   4444  21-Sep-2022 10:43:54  instaparse/line_col.cljc
-rw-rw-rw-   4516  21-Sep-2022 11:03:02  instaparse/line_col.clj
-rw-rw-rw-   4563  21-Sep-2022 10:43:54  instaparse/viz.clj
-rw-rw-rw-   6631  21-Sep-2022 10:43:54  instaparse/combinators_source.cljc
-rw-rw-rw-   6713  21-Sep-2022 11:03:02  instaparse/combinators_source.clj
-rw-rw-rw-   9734  21-Sep-2022 10:43:54  instaparse/repeat.cljc
-rw-rw-rw-   9804  21-Sep-2022 11:03:02  instaparse/repeat.clj
-rw-rw-rw-  10112  21-Sep-2022 10:43:54  instaparse/abnf.cljc
-rw-rw-rw-  10180  21-Sep-2022 11:03:02  instaparse/abnf.clj
-rw-rw-rw-  12874  21-Sep-2022 11:02:54  instaparse/cfg.cljc
-rw-rw-rw-  12941  21-Sep-2022 11:03:02  instaparse/cfg.clj
-rw-rw-rw-  15492  21-Sep-2022 10:43:54  instaparse/core.cljc
-rw-rw-rw-  15560  21-Sep-2022 11:03:02  instaparse/core.clj
-rw-rw-rw-  15773  21-Sep-2022 10:43:54  instaparse/auto_flatten_seq.cljc
-rw-rw-rw-  15853  21-Sep-2022 11:03:02  instaparse/auto_flatten_seq.clj
-rw-rw-rw-  41596  21-Sep-2022 10:43:54  instaparse/gll.cljc
-rw-rw-rw-  41663  21-Sep-2022 11:03:02  instaparse/gll.clj

For some namepaces Instaparse has a CLJ and a CLJC file, the instaparse/line-col namespace for example.

During the inlining done by mranderson, I observed the following behaviour:

This PR avoids the double rewrite by checking if the file to be processed has already the new namespace. If it does it is just moved to the new location without rewriting it's content again.

What do you think of this fix?

[1] https://github.com/clojure-emacs/orchard/pull/164

r0man commented 2 years ago

Hmm, for some reason the CICD is failing. It passes locally, though. Investigating ...

r0man commented 2 years ago

For some reason the rewritten Instaparse files are missing the ^{:mranderson/inlined true} annotation on Circle CI while they are present locally 🤔

benedekfazekas commented 2 years ago

will have a proper look tomorrow

benedekfazekas commented 2 years ago

hi there, no objections to the PR. but the test failure seems to be genuine to me, fails locally the same way as on CI after make clean && make test integration tests (make integration-test) pass so that is a good sign. Let me know if you fix the test failure and I will be happy to merge this.

r0man commented 2 years ago

Hi @benedekfazekas , hmm, I still can't reproduce this. Let me take a closer look in the next days again.

benedekfazekas commented 2 years ago

thanks, will try to find time to do the same

benedekfazekas commented 2 years ago

was fixed along the lines you proposed @r0man with 718ef4a8591772fb12f665ea46b09b9689f6aac0, thanks for your contribution