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

jank repl clojure.core loads clojure.core twice #166

Open
frenchy64 opened this issue Jan 17, 2025 · 4 comments · May be fixed by #175
Open

jank repl clojure.core loads clojure.core twice #166

frenchy64 opened this issue Jan 17, 2025 · 4 comments · May be fixed by #175
Assignees
Labels

Comments

@frenchy64
Copy link

$ jank repl clojure.core
Bottom of clojure.core
Bottom of clojure.core
clojure.core=>
@Samy-33 Samy-33 self-assigned this Jan 18, 2025
@jeaye jeaye added the type:bug label Jan 18, 2025
@Samy-33 Samy-33 linked a pull request Jan 20, 2025 that will close this issue
@elken
Copy link

elken commented Jan 21, 2025

We can actually go up to 3 it seems

Image

After this, any subsequent calls don't show the log message.

This implies that clojure.core is not being cached properly, I assume what's happening here is:

  • jank start up and loads clojure.core, doesn't add to the cache
  • It passes over to the repl, which gets told to load clojure.core, doesn't add to the cache
  • When we get into the REPL, we finally require it and add it to the module cache

Is this right @jeaye ? If so it seems we should always cache namespaces when we require, or is there some AOT/JIT shenanigans going on?

@jeaye
Copy link
Member

jeaye commented Jan 21, 2025

Nope, it's an issue with the module loader. Should be marking this as loaded and then not loading it again.

I said the same thing over on the PR: #175

@Samy-33
Copy link
Contributor

Samy-33 commented Jan 21, 2025

the native loader::load() doesn't yet mark the ns loaded. It's the job of the clojure.core/load to include it in the clojure.core/*loaded-libs*.

Two ways we can go:

  1. Update *loaded-libs* from loader::load. And, skip modules already loaded. ; I don't like this because we are essentially doing the same thing at two places.
  2. Natively load clojure.core in the beginning and initialize the *loaded-libs* with clojure.core ; I dislike this idea more, because of the spatial distance between initialization and loading clojure.core. It's not at all obvious.

Perhaps, we can make do with the first one. But I am gonna check how clojure does it first.

@jeaye
Copy link
Member

jeaye commented Jan 21, 2025

Ok, that's a good breakdown. Choosing one of those is necessary, over the change we have in this PR, though. We'll need to ensure that modules don't get loaded again.

To me, I think that the first option is the only viable one, since this will apply to any module loaded natively. Given that jank is a C++ library as well as a language, people will be using jank to load modules natively. Those need to be properly handled.

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

Successfully merging a pull request may close this issue.

4 participants