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

fix caller() for PAR::Packer in modulinos packer #509

Merged
merged 15 commits into from
Apr 1, 2024

Conversation

kal247
Copy link
Contributor

@kal247 kal247 commented Mar 19, 2024

No description provided.

@github-actions github-actions bot added Type: enhancement improve a feature that already exists Priority: low get to this whenever Program: units The units program Program: echo The echo program Program: date The date program Program: glob The glob program Program: factor The factor program labels Mar 19, 2024
Copy link
Owner

@briandfoy briandfoy left a comment

Choose a reason for hiding this comment

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

The check for 'main' is a bit weird here and we can't disallow that. That would be surprising to people who write a simple program and want to load one of the modulinos.

Is this something that PAR does that caller(1) won't catch?

@briandfoy briandfoy added the Status: changes requested adjust the pull request as noted in comments label Mar 19, 2024
@briandfoy briandfoy self-assigned this Mar 19, 2024
@briandfoy
Copy link
Owner

Note that this change makes t/factor/factor.t hang.

@kal247 kal247 changed the title fix caller() for PAR::Packer in modulinos packer v1.32 Mar 19, 2024
@kal247 kal247 changed the title packer v1.32 fix caller() for PAR::Packer in modulinos packer Mar 19, 2024
@kal247
Copy link
Contributor Author

kal247 commented Mar 19, 2024

Long story short :
While I was preparing this PR yesterday, Murphy's law hit me.
Calling the modulinos indirectly, like bin/perlpowertools echo (for example) didn't work (anymore?).
So I added this 'main' test to fix the issue, then noticed that the tests for 'factor' and 'units' hang.
At that point I was very confused (doubting of my code, my memory and my two laptops) and decided to start the PR...

@kal247
Copy link
Contributor Author

kal247 commented Mar 19, 2024

I'll raise another PR with all the code, so you can have the full picture.

@kal247
Copy link
Contributor Author

kal247 commented Mar 19, 2024

Somehow a commit "packer v1.32" has been added to this PR.
The way of Git is really mysterious to me.

@kal247
Copy link
Contributor Author

kal247 commented Mar 19, 2024

The check for 'main' is a bit weird here and we can't disallow that. That would be surprising to people who write a simple program and want to load one of the modulinos.

Is this something that PAR does that caller(1) won't catch?

If bin/perlpowertools declared (or reused) a package, would it show as caller(0)? Maybe could we even test caller(0) eq 'PerlPowerTools'?

@briandfoy
Copy link
Owner

I don't know anything how packer works, so I don't have any advice to give there. Maybe people on /r/perl or Stackoverflow would know.

@kal247
Copy link
Contributor Author

kal247 commented Mar 20, 2024

To fix the issue with 'main' in modulinos : I've set package PerlPowerTools before calling do in helper script, then changed the test in modulinos.
For 'factor', I had to add 1; to avoid Failed test 'require 'bin/factor'.
'factor' and 'units' don't hang anymore and make test runs ok.

bin/date Outdated
@@ -22,7 +22,8 @@ my $VERSION = '1.0.4';
# prefer this if it is defined.
my $TZ;

run(\@ARGV) unless caller();
# run if called directly, indirectly, directly par-packed, undirectly par-packed
run(\@ARGV) if !caller() || caller(0) =~ /PerlPowerTools/ || caller(0) eq 'PAR' || caller(1) eq 'PAR';
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand why this is needed, but caller(0) is being evaluated twice, 1st by regex then by "eq". Would it be better to cover both PAR and PerlPowerTools in the regex?

Copy link
Owner

Choose a reason for hiding this comment

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

I like this plan (with some fixes), but let's choose a namespace that only the packer stuff will use just to avoid possible conflicts. Something like PerlPowerTools::Packed that's not already in use.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good ideas

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't understand why this is needed, but caller(0) is being evaluated twice, 1st by regex then by "eq". Would it be better to cover both PAR and PerlPowerTools in the regex?

There are some layers and iterations of development on this line of code...

@briandfoy
Copy link
Owner

The latest changes are good. It looks like there's an unrelated files outside of bin/ that snuck in. If we can fix that up I think this request is good to go.

@kal247
Copy link
Contributor Author

kal247 commented Mar 24, 2024

Sorry, but where are the unrelated files? I don't find them.

Another question : I've not yet committed the latest 'perlpowertools.exe' file itself. Should I do it, or will it be rebuilt automatically anyway?

Thanks

@briandfoy
Copy link
Owner

When I look at this PR in GitHub, there are 12 files changed. I figure that you merge something and some other files snuck in.

As for rebuilding, I haven't set up anything to do anything with packer. If something needs to be rebuilt we'll have to figure that out. It might happen as part of the release GitHub action.

@kal247
Copy link
Contributor Author

kal247 commented Mar 30, 2024

The 12 files are changed as intended :

2 changes to move and rename packer.pl -> packer
2 new tests
1 helper script
5 tools fixes for modulinos
new Readme file
new binary

@briandfoy
Copy link
Owner

For this PR, you should only be fixing the files under bin/. This is the same thing I said before. I'm not going to merge the files outside of bin/ for this change, so the pull request needs to be fixed.

@briandfoy briandfoy merged commit bc20fbb into briandfoy:master Apr 1, 2024
1 check failed
@kal247
Copy link
Contributor Author

kal247 commented Apr 3, 2024

Thanks!

@briandfoy briandfoy added Status: accepted The fix is accepted and removed Priority: low get to this whenever Status: changes requested adjust the pull request as noted in comments labels Apr 5, 2024
@briandfoy briandfoy added Status: released there is a new release with this fix and removed Status: accepted The fix is accepted labels Apr 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Program: date The date program Program: echo The echo program Program: factor The factor program Program: glob The glob program Program: units The units program Status: released there is a new release with this fix Type: enhancement improve a feature that already exists
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants