Skip to content
New issue

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

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

Already on GitHub? # to your account

Dynamically add gen-path to classloader #31

Merged
merged 1 commit into from
Jun 15, 2019
Merged

Conversation

mfikes
Copy link
Contributor

@mfikes mfikes commented Jun 14, 2019

Fixes #30

@mfikes
Copy link
Contributor Author

mfikes commented Jun 14, 2019

@Olical The way this works is that it steals the approach used by add-lib to dynamically add code to the classpath.

add-loader-url comes from here originally https://github.com/clojure/tools.deps.alpha/blob/e160f184f051f120014244679831a9bccb37c9de/src/main/clojure/clojure/tools/deps/alpha/repl.clj#L22

And the bit of code that ensures that the correct class loader at play is from here mfikes/clojurescript@d68c939#diff-2c521d7ba435fbef7d128f4295f86785R611

The PR removes the gotcha warning, as well as the extra instructions regarding manually adding to :paths in deps.edn.

I successfully tested this locally, but I think I must have an older clone of cljs-test-runner (and thus the conflicts). Let me update and try again with a revision to this PR to consider.

@mfikes
Copy link
Contributor Author

mfikes commented Jun 14, 2019

OK. Rebaselined against master. Going to re-test to ensure all is OK.

@mfikes
Copy link
Contributor Author

mfikes commented Jun 14, 2019

OK @Olical, I've re-tested locally after having resolved merge conflicts and all appears to be OK.

This PR is ready for review.

@Olical
Copy link
Owner

Olical commented Jun 15, 2019

This is great, thank you very much! I've got a bus ride to my parents right now so I'll give it a whirl and merge if it looks good. Thank you so much for lending your expertise to fix this.

loader)))]
(if (instance? DynamicClassLoader loader)
(.addURL ^DynamicClassLoader loader u)
(throw (IllegalAccessError. "Context classloader is not a DynamicClassLoader")))))
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pretty cool stuff, will definitely be making use of this in other projects. Adding deps at runtime will be super handy with a Clojure plugin system I'll need soon. Thanks for a good example!

@Olical
Copy link
Owner

Olical commented Jun 15, 2019

It worked! 🎉

I altered the readme to put the advanced compilation section above the gotchas on it's own now but that's all. It's no longer a gotcha, just a tip. Thanks for removing the part about the weird bug though. I guess it didn't fix simple or whitespace... will try though.

@Olical
Copy link
Owner

Olical commented Jun 15, 2019

Oh, it looks like you fixed the whitespace and simple errors too... I don't fully understand why this fixes it but I'm very happy about it!

@Olical
Copy link
Owner

Olical commented Jun 15, 2019

Hmm, okay, :whitespace still just ends with a non-zero exit code, you accidentally fixed :simple though 😅

@Olical Olical merged commit 4bd00bf into Olical:master Jun 15, 2019
@mfikes mfikes deleted the issue-30 branch June 28, 2019 20:36
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

No test execution if :advanced and no runner out dir
2 participants