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

Remove nqp-home/lib and rakudo-home/lib from vm search paths #5304

Merged
merged 2 commits into from Jul 5, 2023

Conversation

ugexe
Copy link
Member

@ugexe ugexe commented Jul 1, 2023

vm_search_path used to provide some library paths for MoarVM that are now provided by other methods. This removes these redundant search paths for MoarVM similar to what we do for the JavaScript back end.

The removed comment suggests that this code shouldn't be needed
anymore, since `:from<NQP>` was implemented at some point. The
passing `make test` and `make spectest` also suggest this code is
not needed anymore. However, to be clear, I'm not sure why the
`:from<NQP>` adverb might make this library inclusion required.
@patrickbkr
Copy link
Contributor

Did you try this outside of CI? I suspect this breaks the installed executables. (The ones generated in the build directory are probably fine.)
Why do I think so: Searching for nqp-home in rakudo and nqp sources yields no other place that uses that path. (Except for accessing the profiler/template.html).
It seems that the nqp-home path is missing in the NQP module loader: https://github.com/Raku/nqp/blob/492fe69d1aea8f8823fa2a5144185284167bce4f/src/vm/moar/ModuleLoader.nqp#L267 (same for the JS and JVM ones)

@ugexe
Copy link
Member Author

ugexe commented Jul 3, 2023

It is still early for me so maybe I'm overlooking it, but what actions can I take to demonstrate the executable being broken now?

@patrickbkr
Copy link
Contributor

patrickbkr commented Jul 3, 2023

It is still early for me so maybe I'm overlooking it, but what actions can I take to demonstrate the executable being broken now?

I would expect about nothing to work with the nqp libraries not being found. To be extra sure, try doing something that loads a module.

The only idea I have, why it might work is that the rakudo executable in the build directory is built differently and doesn't rely on rakudo-home or nqp-home. So try using the one generated in the installation directory after a make install.

Or I'm wrong and this somehow works in a way I don't understand...

@ugexe
Copy link
Member Author

ugexe commented Jul 3, 2023

I'll test that later today sometime if someone else doesn't beat me to it. I wonder if we should have a CI test that does something like run make test but with the installed executable

@patrickbkr
Copy link
Contributor

Actually there is: https://github.com/rakudo/rakudo/blob/main/azure-pipelines.yml#L208
And that test did succeed with this PR. But why?

@ugexe
Copy link
Member Author

ugexe commented Jul 3, 2023

try doing something that loads a module

$ raku -e 'use Test; say 42;'
42

$ raku -e 'use NQPHLL:from<NQP>; say 42;'
42

It seems to work 🤔

@niner
Copy link
Collaborator

niner commented Jul 3, 2023 via email

@ugexe
Copy link
Member Author

ugexe commented Jul 3, 2023

Everything works even if I make vm_search_paths return an empty array 🤔

@patrickbkr
Copy link
Contributor

Ugh, it's too long since I wrote this... :-(
Doing some research:

I think https://github.com/rakudo/rakudo/blob/main/src/vm/moar/runner/main.c#L428-L446 is the reason these paths are not needed in Moars ModuleLoaderVMConfig. The paths are needed very early on for Moar to find Rakudo itself and in turn the module loader. That's why the paths are added in the runner.

I notice that neither the JVM nor JS ModuleLoaderVMConfigs return the nqp/rakudo-home lib dirs. Removing those paths in Moars ModuleLoaderVMConfig would be consistent.

I notice that the rakudo-home directory not only contains the lib/ and runtime/ directories, but also the core/, site/ and vendor/ CURIs. Those are added here. So there still is use for the rakudo-home retrieval logic in Rakudo land.

@ugexe It seems you are right. nqp-home and rakudo-home are both not needed and can go.

rakudo-home/lib is provided elsewhere, and thus this search path
isn't needed. This makes the vm_search_path return an empty array,
similar to the JS backend. Note that we don't get rid of the
vm_search_path method altogether because the JVM can return a non
empty array of paths.
@ugexe ugexe changed the title Remove nqp/lib from vm search paths Remove nqp-home/lib and rakudo-home/lib from vm search paths Jul 4, 2023
@ugexe ugexe merged commit 04bf4f7 into main Jul 5, 2023
7 of 11 checks passed
@ugexe ugexe deleted the ugexe/remove-unneeded-nqp-lib branch July 5, 2023 21:38
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.

None yet

3 participants