cljs.main: Failure to load user.cljs if temp out dir

Description

src/user.cljs

If, on the other hand you specify an output directory (via -d) things work.

Environment

{:deps {org.clojure/clojurescript {:mvn/version "1.10.339"}}}

Activity

Show:
Ruby
October 15, 2018, 7:42 PM

While working on the patch for CLJS-2753, I noticed that the fix for that bug also fixes this issue. If supporting absolute paths in compile-file isn't desired, this patch can be dropped and the CLJS-2753 patch used instead. My commit messages in CLJS-2917.patch regarding cljs.repl.rhino... are incorrect; these files dissoc :output-dir from opts, so they end up passing cljs.closure/compile-file a relative path. (which wasn't done for cljs.repl/load-file, resulting in these two issues)

Mike Fikes
October 5, 2018, 4:04 PM

Latest patch LGTM, behaves properly, and passes in CI.

Ruby
October 1, 2018, 6:34 PM

Thanks again, Mike. I managed to reproduce your error on Linux, made the changes you suggested to both the logic and assert, and confirmed that it was fixed. I also wrapped the output-file returned in the if truthy branch in an io/file call, as otherwise if an absolute path string was supplied, I'm concerned it would fail the (.exists out-file) expression further down.

Mike Fikes
October 1, 2018, 1:14 AM

The patch works for the ticket description if you go with the default browser REPL, but on macOS if you specify the Node REPL you will see a failure related to the canonicalization of the temp directory path:

I think that this can be fixed by asserting

instead of

Another issue that concerns me about the patch is that output-file could be a relative File, in which case

is a legitimate construct and the code should probably take that branch. In my opinion, it is only when out-file is absolute (either a string or File) that it should be take "as is" and not relative to out-dir. In other words, the condition is probably not that it satisfies util/file? but rather absolute-path?.

If we consider those two changes (using absolute-path? to trigger the logic and same-or-subdirectory-of? for the assert), this might cover all bases. (Both fns would need to be declare d as they occur after compile-file.)

Mike Fikes
October 1, 2018, 12:39 AM

Note, Herald indicated via private Slack message to me that the CA has been signed.

Assignee

David Nolen

Reporter

Mike Fikes

Labels

None

Approval

None

Patch

Code

Priority

Major