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

Minor cleanups #9

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Minor cleanups #9

wants to merge 3 commits into from

Conversation

grg
Copy link
Collaborator

@grg grg commented Jan 20, 2025

Replaces #7 -- that PR got closed automatically after merging the license branch. I guess things work differently when merging from a fork vs merging from a different branch in the same repo.

Several small clean-ups:

  • Switch from #!/usr/bin/perl to #!/usr/bin/env perl to use the Perl version that appears first in the users path. This is useful when the user wants to use a different version of perl than that provided with the OS -- this may happen when using an older OS version for vendor tool compatibility, but a newer Perl has been installed.
  • Remove pre-compiled Genesis binaries with compiled-in modules. Without this, changes to the Genesis Perl modules don't impact Genesis execution. Perhaps an install step should be generating a compiled binary?
  • Add a space to the "Generating " message.

grg added 3 commits January 20, 2025 16:48
Use `#!/usr/bin/env perl` instead of `#!/usr/bin/perl` to allow
overriding the default version of perl installed on the system.
Remove the Genesis binaries with compiled-in modules. Want changes to
PerlLibs/Genesis2 to impact the program.
@grg grg requested a review from steveri January 20, 2025 22:50
@grg grg mentioned this pull request Jan 20, 2025
@steveri
Copy link
Contributor

steveri commented Jan 20, 2025

I did some poking around with these changes...it looks as though we will have to do some alterations before this works with 'pip install'...I will see what I can do...

@steveri
Copy link
Contributor

steveri commented Jan 21, 2025

Okay, here's my plan, @grg see if this works for you

  • I will upgrade the 'pip install' bpypi binary so that it works with this new set of changes
  • I'll spend a little time verifying
  • then I will give final approval for this merge

Hopefully that will happen by end of day today or tomorrow.

Offline, in the background, I will work to unify the 'pip install' and manual-install paths so that we don't run into this problem in the future...

steveri
steveri previously approved these changes Jan 21, 2025
@steveri
Copy link
Contributor

steveri commented Jan 21, 2025

Done and done.
Looks good. Approved!
Merge at will.

Comparing 80312 lines of tmp-garnet.v0 (generated using Genesis2 existing master)
versus    80312 lines of tmp-garnet.v1 (generated using Genesis2 'pull/9/head')

diff tmp-garnet.v0 tmp-garnet.v1
# [ no diffs ]
Test PASSED

@steveri
Copy link
Contributor

steveri commented Jan 22, 2025

Oops please do not merge this yet there appears to be a problem

@steveri steveri dismissed their stale review January 22, 2025 15:36

see comment

@steveri
Copy link
Contributor

steveri commented Jan 22, 2025

NVM the issue has been resolved, and it was of course not the fault of your code.

Approval is re-applied, merge at will.

Maybe do it fast before I get confused again :)

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.

2 participants