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

Allow removing dirs from classpath during runs #330

Closed
AlexChalk opened this issue Nov 9, 2022 · 12 comments
Closed

Allow removing dirs from classpath during runs #330

AlexChalk opened this issue Nov 9, 2022 · 12 comments
Labels
feature New functionality

Comments

@AlexChalk
Copy link
Contributor

Context

My application includes something like the following structure:

├── env
│   ├── development
│   │   ├── my_app
│   │   │   └── env_config.clj
│   │   ├── resources
│   │   │   └── logback.xml
│   │   └── user.clj
│   ├── production
│   │   ├── my_app
│   │   │   └── env_config.clj
│   │   └── resources
│   │       └── logback.xml
│   ├── test
│   │   ├── my_app
│   │   │   └── env_config.clj
│   │   └── resources
│   │       └── logback.xml

I have a place to put environment-specific code, and I can then modify the classpath to include the correct code depending on the context in which I'm running the app. (I lifted this pattern from the luminus micro-framework.)

For example, for development I include :extra-paths ["env/development"] in my deps.edn :development alias, and I call (b/copy-dir {:src-dirs [... "env/production" ...] in my production build.clj script.

Problem

I know kaocha includes :kaocha/test-paths in tests.edn, and this is a great place to add ["env/test"] to the classpath when running tests from the command line.

The problem is that this doesn't work when running tests in the repl. I need to start my repl in development with the development alias included, and this means env/dev has already been added to the classpath and env/test is ignored when I call k/run.

Feature Request

I'm guessing that k/run has some way of modifying the context in which it's called to include :kaocha/test-paths in the classpath with no trace of this in the repl afterwards.

What I'd like is a similar config option, maybe :kaocha/excluded-paths, that removes dirs from the classpath in the context of k/run, with no trace of this afterwards.

Would you be open to this feature? I can try adding this myself, with the disclaimer that I'm pretty new to clojure (and completely new to kaocha) so I'd probably need some help. Thanks!

@alysbrooks
Copy link
Member

It sounds like in the REPL situation, you already have the right paths in your classpath, so maybe setting :kaocha.testable/skip-add-classpath? to true would work for you?

excluded-paths might be a useful feature nonetheless, although it seems like it might be kind of a niche feature so I'm not sure whether it would be a priority. I'll think about it some more.

@alysbrooks alysbrooks changed the title (Feature Request) Allow removing dirs from classpath during runs Allow removing dirs from classpath during runs Nov 10, 2022
@alysbrooks alysbrooks added the feature New functionality label Nov 10, 2022
@AlexChalk
Copy link
Contributor Author

It sounds like in the REPL situation, you already have the right paths in your classpath

@alysbrooks the problem is env/dev wasn't added by kaocha, it was necessarily added when I span up my REPL, so skipping kaocha classpath modifications won't help me here. If I want kaocha to successfully add env/test for test runs I need to remove env/dev first.

@plexus
Copy link
Member

plexus commented Nov 10, 2022

I'm guessing that k/run has some way of modifying the context in which it's called to include :kaocha/test-paths in the classpath with no trace of this in the repl afterwards.

That's not correct. We add to the classpath, afterwards these directories stay on the classpath.

What I'd like is a similar config option, maybe :kaocha/excluded-paths, that removes dirs from the classpath in the context of k/run, with no trace of this afterwards.

This is not possible in the JVM, the classpath that the JVM started with is inaccessible/immutable, you can't remove entries from it. You can install a child classloader that searches additional paths, but the application class loader is fixed.

There are probably ways to make this work if you try hard enough, eg by injecting a java agent at startup, but that's beyond what kaocha is willing to do I think.

However you don't necessarily need to remove entries in your case, all you need is that env/test is searched before env/dev. The priority-classloader in lambdaisland/classpath can do this, I would suggest playing around with that and disabling kaocha 's classpath manipulation.

@alysbrooks
Copy link
Member

Perhaps I'm still misunderstanding, but if you need env/dev for the majority of the REPL work and env/test for running tests, you may just have to run separate REPLs or re-engineer your env system so it can be changed dynamically.

@AlexChalk
Copy link
Contributor Author

However you don't necessarily need to remove entries in your case, all you need is that env/test is searched before env/dev.

@plexus If I can change the order they are searched (and change it back afterwards) that would probably be enough. The lambdaisland/classpath docs mention you can "Add/override the classpath based on the current deps.edn" using update-classpath!, is this how you leverage the priority-classloader?

Thanks also @alysbrooks, "env/dev for the majority of the REPL work and env/test for running tests" is what I'm after, yes.

@plexus
Copy link
Member

plexus commented Nov 14, 2022

The lambdaisland/classpath docs mention you can "Add/override the classpath based on the current deps.edn" using update-classpath!, is this how you leverage the priority-classloader?

Yes, update-classpath! will calculate a new classpath based on the current deps.edn, filter out the entries that are missing from the current classpath, install a priority-classloader, and add those missing entries to that classloader.

So you can use update-classpath!, passing it specific aliases which it then uses to compute the new classpath, or you can install a priority-classloader yourself, and manually add the directories you want to prepend.

See https://lambdaisland.com/blog/2021-08-25-classpath-is-a-lie for a write-up of this stuff.

@AlexChalk
Copy link
Contributor Author

Hi @plexus, thanks for the detailed writeup!

I might be misunderstanding, but based on that blog post, I'd expect (licp/update-classpath! {:extra {:paths ["env/test"]}})) to add any namespaces in env/test to the classpath with a higher priority than other previously loaded namespaces from the project.

The actual behaviour I'm observing is that update-classpath! can add completely new namespaces to the classpath, but not with a higher priority than namespaces added at repl startup.

Based on

filter out the entries that are missing from the current classpath ... and add those missing entries to that classloader

from your previous response, I'm guessing that I misunderstood the blog post and update-classpath! will unfortunately not do what I'm looking for? I'm confused as 'priority classloader' implies overrides would be possible (just not deletions), but this doesn't actually seem to happen.

@plexus
Copy link
Member

plexus commented Nov 19, 2022

Can you please be concrete about how you come to this conclusion? What's the output of licp/classpath-chain before/after calling update-classpath? What does (io/resource "my_app/env_, config.edn") resolve to?

@AlexChalk
Copy link
Contributor Author

AlexChalk commented Nov 19, 2022

Can you please be concrete about how you come to this conclusion?

I tried calling update-classpath! twice, once with an extra path containing a completely new namespace, once with an extra path containing a namespace with the same name as a previously loaded one. The extra path was only loaded in the first scenario.

What's the output of licp/classpath-chain before/after calling update-classpath?

Before:

([clojure.lang.DynamicClassLoader@~~several-of-these~~()]
 [app
  ("test"
   "env/development"
   "src"
   ~~several-m2-repositories~~)]
 [platform ()])

After:

([clojure.lang.DynamicClassLoader@215e201d ()]
 [lambdaisland/priority-classloader45989
  ("file:env/test/"
   "file:/home/adc/.m2/repository/org/apache/httpcomponents/httpcore/4.4.14/httpcore-4.4.14.jar"
   "file:/home/adc/.m2/repository/org/clojure/data.json/0.2.6/data.json-0.2.6.jar"
   "file:/home/adc/.m2/repository/fipp/fipp/0.6.25/fipp-0.6.25.jar"
   "file:/home/adc/.m2/repository/mvxcvi/puget/1.1.2/puget-1.1.2.jar"
   "file:/home/adc/.m2/repository/com/google/code/findbugs/jsr305/1.3.9/jsr305-1.3.9.jar"
   "file:/home/adc/.m2/repository/com/google/guava/guava/19.0/guava-19.0.jar"
   "file:/home/adc/.m2/repository/org/clojure/tools.reader/1.3.4/tools.reader-1.3.4.jar")]
 [clojure.lang.DynamicClassLoader@4a962973 ()]
 [app
  ("test"
   "env/development"
   "src"
   "~~remaining-m2-repositories~~")]
 [platform ()])

It looks like the issue is the updater expects a .jar, not a dir of source code?

What does (io/resource "my_app/env_, config.edn") resolve to?

I'm not sure I understand this question, do you want me do call clojure.java.io/resource on my env directory (env/development/env/test)? If so, both evaluate to nil.

@plexus
Copy link
Member

plexus commented Nov 21, 2022

@AlexChalk I've created a demo repo, please check that out locally and go through the provided repl sessions. Hopefully that clears things up.

It looks like the issue is the updater expects a .jar, not a dir of source code?

classpath entries can be either jars or directories, so you're good. That output looks good, env/test is now "on the classpath".

I'm not sure I understand this question, do you want me do call clojure.java.io/resource on my env directory (env/development/env/test)? If so, both evaluate to nil.

io/resource looks for a resource on the classpath, it's the exact same mechanism that clojure (or java) uses to locate files to load. So you pass it a relative file name, e.g. (based on your example in the original post) my_app/env_config.clj. If it can't find that file on the classpath it will return nil, otherwise you get a URL back of the file it found, e.g.

#object[java.net.URL 0x5c3a1d17 "file:env/test/my_app/env_config.clj"]

See the examples in the demo repo.

@AlexChalk
Copy link
Contributor Author

@plexus Thanks for this!

I was using clojure.tools.namespace.repl/refresh to reload everything after evaluating the licp functions and I think that's why this wasn't working, it wasn't picking up the change. Once I used a standard reload of the namespace I'd replaced like in your demo, everything functioned exactly like you describe 💪 .

Thanks also for the install-priority-loader! advice. I'm going to close this issue as my use case is resolved, I appreciate the stellar support.

@plexus
Copy link
Member

plexus commented Nov 22, 2022

Happy to help!

I was using clojure.tools.namespace.repl/refresh to reload everything

c.t.n keeps a map of last-modified timestamps for each file, and refresh will then reload namespace files that have been modified. I don't think it assumes that these paths can change. You might have more luck with c.t.n.r/refresh-all.

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

No branches or pull requests

3 participants