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

Use NQP prefix for finding profiler template #608

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

zopsicle
Copy link

@zopsicle zopsicle commented Mar 30, 2020

Fixes #567.

This fixes the problem but I am not sure what would be the best way/place to generate the file with the nqpconfig subroutine definition in it. Also missing .gitignore entry.

@lizmat lizmat requested a review from vrurg March 30, 2020 14:25
Copy link
Contributor

@vrurg vrurg left a comment

Choose a reason for hiding this comment

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

We already generate nqp-config.nqp where NQP prefix is specified. It should work, to my understanding. Besides, nqp_home from config is the key to be used, I think.

@@ -93,6 +93,14 @@ sub configure_misc {

$config->{moar_stage0} = $self->nfp( "src/vm/moar/stage0", no_quote => 1 );
$config->{jvm_stage0} = $self->nfp( "src/vm/jvm/stage0", no_quote => 1 );

# TODO: Use the template infrastructure from nqp-configure?
open my $configfile, '>', 'src/core/Config.nqp';
Copy link
Contributor

Choose a reason for hiding this comment

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

We must not autogenerate anything in src. It actually belongs to gen/.

@vrurg
Copy link
Contributor

vrurg commented Mar 30, 2020

It seems to me that the correct code in HLL/Backend.nqp should be:

my $temppath := nqp::getcomp("nqp").config()<libdir> ~ '/profiler/template.html';

@zopsicle
Copy link
Author

zopsicle commented Mar 30, 2020

@vrurg I gave that a try. It fails with: X::AdHoc: Cannot call method 'config' on a null object.

Not really sure where to continue from here. I tried a few things but it seems the nqp compiler is not always available.

@zopsicle zopsicle force-pushed the nqp-prefix-profiler-template branch from 49a878d to a7806d5 Compare March 30, 2020 16:19
@vrurg
Copy link
Contributor

vrurg commented Mar 30, 2020

Ok, I see. I didn't know that rakudo uses profiling code from nqp. nqp::getcomp("nqp") makes sense under nqp runtime only. For rakudo the line would look like:

nqp::getcomp("Raku").config<nqp-home> ~ 'lib/profiler/template.html'

So, the problem would be to know which compiler to use. @lizmat do you have an idea?

@lizmat
Copy link
Contributor

lizmat commented Mar 30, 2020

Try both, and see which one yields something to work with?

@vrurg
Copy link
Contributor

vrurg commented Mar 30, 2020

@lizmat the first thought. But what if somebody writes another nqp-based compiler?

Actually, as I look into it, the place where template.html gets installed is incorrect because the file is moar-backend specific and as such should be installed under some kind of backend subdir.

@vrurg
Copy link
Contributor

vrurg commented Mar 30, 2020

So far, it looks like the proper solution would be to add compiler parameter to run_profiled, dump_profile_data, and dump_instrumented_profile_data methods. Then instead of nqp::getcomp(...).config $compiler.config can be used.

Then, there is minor discrepancy to be taken care of somehow. NQP config has nqp_home key. But in Rakudo it is named nqp-home. I think this should be solved by renaming nqp_home into nqp-home or providing both variants for nqp compiler.

@lizmat
Copy link
Contributor

lizmat commented Mar 30, 2020

If nqp_home is something that can be set by environment variables, then I would choose to to rename the nqp-home to nqp_home instead of vice-versa: you cannot use hyphens in environment variables afaik.

@vrurg
Copy link
Contributor

vrurg commented Mar 30, 2020

No, it is purely our implementation detail thing. It is only affected by Configure.pl command line. If there is

Changing it to nqp-home would also go in line with source-digest key in the compiler config.

@zopsicle
Copy link
Author

So far, it looks like the proper solution would be to add compiler parameter to run_profiled, dump_profile_data, and dump_instrumented_profile_data methods. Then instead of nqp::getcomp(...).config $compiler.config can be used.

This seems overly complicated. When configuring NQP, we already know where it is going to be installed into, so we should be able to just get the path in there.

What about making hll-config, which is the function generated by gen-version.pl, part of the core setting? Then we can just call that and extract nqp-home or even libdir.

@zopsicle
Copy link
Author

Nevermind, this has to be done because nqp-home can be overridden with the NQP_HOME environment variable when invoking Rakudo. I’ll see if passing the compiler config around works.

@lizmat
Copy link
Contributor

lizmat commented Mar 31, 2020

And if the environment variable is called NQP_HOME, I think it should be called "nqp_home" internally as well.

@vrurg
Copy link
Contributor

vrurg commented Mar 31, 2020

@lizmat I disagree. By this logic we should use uppercase too. As well as rakudo-home key.

Snake case would make it inconsistent with other keys in the config. It's even more confusing, than the difference from the environment. Whereas if one already knows what's the rule of thomb he'd expect the key to be kebab-lower-cased.

@coke coke changed the base branch from master to main April 19, 2023 13:37
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.

--profile uses wrong prefix to find template.html
3 participants