Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix for loading markdown.mjs on MS-Windows #32

Merged
merged 1 commit into from
Dec 8, 2021

Conversation

ikappaki
Copy link
Contributor

@ikappaki ikappaki commented Dec 8, 2021

Hi,

could you please consider work around for #29 about loading markdown.mjs on MS-Windows, which is still occurring even after the #31 cumulative patch.

The cause of the issue seems to be with oracle/graaljs this time though. It appears as if it tries to use the path component of the Source URL as a path to he resource, but this method does not seem to be working on MS-Windows because the path component begins with /, and not a drive letter as common windows paths. I've raised oracle/graaljs#534 so as to get some feedback.

The work around is to warp the resource in a File. I've tested this to work both on MS-Windows and Linux, but not sure how to test using the more involved jar example mentioned in #31.

Thanks

so as to work around a polyglot issue failing to load local URLs on
MS-Windows.
@mk
Copy link
Member

mk commented Dec 8, 2021

Thanks for this and sorry that we missed this on your first PR.

@mk mk merged commit e8c7520 into nextjournal:main Dec 8, 2021
mk added a commit to nextjournal/clerk that referenced this pull request Dec 8, 2021
@mk
Copy link
Member

mk commented Dec 8, 2021

I've also bumped this in Clerk, see nextjournal/clerk@214df4e. Can you confirm that this works for you now? Thanks again!

@mk
Copy link
Member

mk commented Dec 8, 2021

Actually this seems to break CI so I had to push it out of Clerk: https://github.com/nextjournal/clerk/runs/4461312453?check_suite_focus=true

@ikappaki
Copy link
Contributor Author

ikappaki commented Dec 8, 2021

Actually this seems to break CI so I had to push it out of Clerk: https://github.com/nextjournal/clerk/runs/4461312453?check_suite_focus=true

Indeed, it does not work with jar file paths, is best to revert. let's wait to see what the graaljs team has to say, there is no obvious work around for jar files.

Caused by: java.lang.IllegalArgumentException: Not a file: jar:file:/C:/Users/ikappaki/.m2/repository/io/github/nextjournal/markdown/0.1.37/markdown-0.1.37.jar!/js/markdown.mjs

(I've narrowed it btw, it is the call to java.nio.file.Paths/get that can't handle the leading slash in the path , as in https://github.com/oracle/graal/blob/6773ebc7dcd59ec7185ba5f5db70b714aa43e108/truffle/src/com.oracle.truffle.polyglot/src/com/oracle/truffle/polyglot/FileSystems.java#L759

(Paths/get "c:/any" (into-array String [])) ;; => #object[sun.nio.fs.WindowsPath 0x1295c3ee "c:\\any"]
(Paths/get "/c:/any" (into-array String [])) ;; => Illegal char <:> at index 2: /c:/any

)

Thanks for the quick response.

mk added a commit that referenced this pull request Dec 8, 2021
@mk
Copy link
Member

mk commented Dec 8, 2021

@ikappaki thanks, I've reverted it. Do I understand it correctly that when the file is loaded from a jar it like in Clerk at nextjournal/clerk@aa6ed09 it works for you on windows? In that case I think it's fine to wait for a response from the Graal team.

@ikappaki
Copy link
Contributor Author

ikappaki commented Dec 8, 2021

@ikappaki thanks, I've reverted it. Do I understand it correctly that when the file is loaded from a jar it like in Clerk at nextjournal/clerk@aa6ed09 it works for you on windows? In that case I think it's fine to wait for a response from the Graal team.

@mk, it is clerk's bb dev workflow that fails on all archs now with the jar issue and thus I believe we need a new version of io.github.nextjournal/markdown with the revert ? Can you give bb dev a try please on Linux?

/mnt/d/src/clerk $ clojure -M:dev
Downloading: io/github/nextjournal/markdown/0.1.37/markdown-0.1.37.pom from clojars
Downloading: thheller/shadow-cljs/2.16.7/shadow-cljs-2.16.7.pom from clojars
Downloading: io/github/nextjournal/markdown/0.1.37/markdown-0.1.37.jar from clojars
Downloading: thheller/shadow-cljs/2.16.7/shadow-cljs-2.16.7.jar from clojars
Exception in thread "main" Syntax error macroexpanding at (markdown.clj:28:45).
	...
Caused by: java.lang.IllegalArgumentException: Not a file: jar:file:/home/ikappaki/.m2/repository/io/github/nextjournal/markdown/0.1.37/markdown-0.1.37.jar!/js/markdown.mjs
	at clojure.java.io$fn__11416.invokeStatic(io.clj:61)
	at clojure.java.io$fn__11416.invoke(io.clj:44)
	at clojure.java.io$fn__11390$G__11372__11395.invoke(io.clj:35)
	at clojure.java.io$file.invokeStatic(io.clj:424)
	at clojure.java.io$file.invoke(io.clj:418)
	at clojure.lang.AFn.applyToHelper(AFn.java:154)
	at clojure.lang.RestFn.applyTo(RestFn.java:132)
	at clojure.lang.Compiler$InvokeExpr.eval(Compiler.java:3706)
	... 97 more

I'm currently hit by a bizzare git@dep error and can't test any further past that point, I can't figure out where this dependency is coming from:

> bb dev
...
Cloning: [email protected]:nextjournal/clerk.git
Error building classpath. Unable to clone C:\Users\ikappaki\.gitlibs\_repos\ssh\github.com\nextjournal\clerk
FATAL ERROR: No supported authentication methods available (server sent: publickey)
fatal: Could not read from remote repository.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants