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

Binary flamegraph.pl may possible overwrite official flamegraph.pl #222

Open
vovkasm opened this issue Nov 4, 2024 · 5 comments · May be fixed by #223
Open

Binary flamegraph.pl may possible overwrite official flamegraph.pl #222

vovkasm opened this issue Nov 4, 2024 · 5 comments · May be fixed by #223

Comments

@vovkasm
Copy link

vovkasm commented Nov 4, 2024

Hi, we have an old bug in FreeBSD port of Devel::NYTProf module.

In short, both devel/p5-Devel-NYTProf and benchmarks/flamegraph ports try to install script flamegraph.pl into same place.

We can resolve this with renaming, but honestly I'd prefer to not patch original sources too extensively... Can we discuss a more general solution?
May be we can hide this script from users, or rename it to something like nytprof-flamegraph.pl, or maybe we can use upstream binary and depend on it?

Also let's mention author of flamegraph.pl, may be @brendangregg can help us with better solution?

@jkeenan
Copy link
Collaborator

jkeenan commented Nov 7, 2024

In the CPAN distribution Devel-NYTProf, flamegraph.pl is currently located in the bin/ subdirectory. It's there presumably because that's where CPAN distros place their installable programs. But Devel-NYTProf is currently in "steady-state" mode. We're not adding new features; we fix some bugs; mainly we (I) try to keep its test suite PASSing against updated versions of the Perl core distribution. I think if we moved flamegraph.pl to the demo/ directory within the distribution, it would not get installed and hence there would be no conflict with your port of @brendangregg's version, which is probably better maintained than ours. How does that sound?

@timbunce ^^

@shawnlaffan
Copy link
Contributor

flamegraph.pl is needed by nytprofhtml.pl.

my $flamegraph = File::Spec->catfile($Config{'bin'}, 'flamegraph') . $script_ext;
$flamegraph = which "flamegraph$script_ext" if not -e $flamegraph;

Perhaps it's better to rename the version in this repo to something like nyt_flamegraph.pl and update the calls as needed? Otherwise systems without their own flamegraph.pl will fail.

@timbunce
Copy link
Owner

timbunce commented Nov 7, 2024

Renaming seems fine to me.

@jkeenan
Copy link
Collaborator

jkeenan commented Nov 7, 2024

Renaming seems fine to me.

Okay. For medical reasons, I'm going to be out of commission for a month or so. So if @shawnlaffan can prepare a pull request and @timbunce can apply it and do a CPAN release, that would be great. Otherwise, I'll have to defer action until early in 2025.

@vovkasm
Copy link
Author

vovkasm commented Nov 7, 2024

I can help with MR too, but not quickly either, maybe within one or two weeks

@shawnlaffan shawnlaffan linked a pull request Nov 8, 2024 that will close this issue
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 a pull request may close this issue.

4 participants