mranderson rewrites some cljc/clj files twice

[benedekfazekas]

from @r0man 's PR #71

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:

Mr Anderson starts rewriting the instaparse dependency

It has a list of files to rewrite, one after the other

It starts rewriting instaparse/line-col.clj

After rewriting instaparse/line-col.clj it rewrites all other
files to refer to this new namespace, including
instaparse/line-col.cljc

When instaparse/line-col.cljc is picked up for rewriting it's
namepace was alreday replaced, but mranderson rewites it again.

[benedekfazekas]

Looking at this I realised that the reason behind instaparse having cljc and clj files for the same namespaces is to provide backward compatibility for clojure 1.6 (before reader conditionals). Instaparse does this via https://github.com/aengelberg/cljsee In fact if you look at the source of instaparse before the cljsee magicked uberjar, there is no such duplication: https://github.com/Engelberg/instaparse/tree/master/src/instaparse

Based on that I am thinking now that mranderson could actually filter out those excessive clj files and just work with the cljc files for instaparse or more generically for all such projects. There is a risk here of course that I break other project's mrandersoning so I wonder if there is a valid reason to have a cljc file and a clj (or cljs for that matter) for the same namespace. I know it is kinda the policy to have a clj and a cljs file for the same namespace or a single cljc file. no idea if there is a valid reason to have cljc and clj/cljs (apart from cljsee's). maybe @danielcompton if you are available for commenting on this, you could share your opinion?

[benedekfazekas]

created a branch called fix-72 with a wip fix for this (see my comment above), if you could try that @r0man if this helps with inlining instaparse for clojure-emacs/orchard#164

[r0man]

Hi @benedekfazekas,

thanks for looking into this. I found these 2 resources on that topic:

https://clojure.org/reference/libs#order
https://groups.google.com/g/clojure-dev/c/d5Hb1E7zfHE/m/sPybIAxgCwAJ?pli=1

Given what I'm reading there, I think it's perfectly fine to have even all 3 types of Clojure files in a project. So, if possible I would prefer the approach of correctly rewriting all those files, instead of removing what we think is not needed.

WDYT?

[r0man]

@benedekfazekas I found another issue which might affect you when experimenting with Instaparse. I created an issue for this here just now: #73

[benedekfazekas]

hm... good finds. maybe your approach is better then. let me have a think about this. on a sidenote the watermark missing could be a separate issue not related to this or your PR.

[benedekfazekas]

hi @r0man updated the fix-72 branch with an approach pretty much based on yours, pls give it a go and let me know if it works and if any comments

[r0man]

Hi @benedekfazekas,

I just tried it with Instaparse inlined in cider-nrepl and it seems to work!
Let's go with your approach. I think having this in the move namespace is better.

Thanks for taking a look at this.

[benedekfazekas]

glad it worked, will add the fix and do a snapshot release to unblock your cider-nrepl work, still thinking about #73 but suppose that is not really a blocker for you, is it?

[r0man]

Ok, thanks for your help! Much appreciated. No, I don't think this isn't a blocker. I will also still need a bit more time with the stacktrace stuff anyway.

[benedekfazekas]

coolio i might try to fix #73 as well and snapshot release them together

[benedekfazekas]

fixed with 718ef4a