Skip to content

add fix_shebang to install_step of PythonPackage easyblock so that we can fix shebangs when installing extensions#2680

Merged
boegel merged 1 commit intoeasybuilders:developfrom
ComputeCanada:fix_shebang_pythonpackage
Nov 9, 2022
Merged

add fix_shebang to install_step of PythonPackage easyblock so that we can fix shebangs when installing extensions#2680
boegel merged 1 commit intoeasybuilders:developfrom
ComputeCanada:fix_shebang_pythonpackage

Conversation

@mboisson
Copy link
Copy Markdown
Contributor

@mboisson mboisson commented Feb 14, 2022

…nstalling extensions

@mboisson
Copy link
Copy Markdown
Contributor Author

mboisson commented Feb 14, 2022

The idea is being able to add something like this in an exts_list for a PythonBundle:

    ('CacheControl', '0.12.10', {
    	'fix_python_shebang_for': ['bin/doesitcache'],
    }),

or

    ('wheel', '0.37.1', {
    	'fix_python_shebang_for': ['bin/wheel'],
    }),
    ('pip', '22.0.3', {
    	'fix_python_shebang_for': ['bin/pip', 'bin/pip3'],
    }),

in order to be able to install those with multi_deps for python.

akesandgren
akesandgren reviewed Feb 15, 2022
View reviewed changes
Comment thread easybuild/easyblocks/generic/pythonpackage.py
self.install_cmd_output += out

# fix shebangs if specified
self.fix_shebang()
Copy link
Copy Markdown
Contributor

@akesandgren akesandgren Feb 15, 2022

Choose a reason for hiding this comment

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

Won't this pick up a shebang list from the non-extension part of the easyconfig?
That may be a bad thing.

Copy link
Copy Markdown
Contributor Author

@mboisson mboisson Feb 15, 2022

Choose a reason for hiding this comment

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

Possibly. Do you have a suggestion on how to avoid that ?

Copy link
Copy Markdown
Contributor Author

@mboisson mboisson Feb 15, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

In reality, I always found it weird that extensions pick up anything from the non-extension part... I've been bitten by it more than once using configopts and other parameters. In that sense, the shebang part is no different than other parameters. If there's a change to do in that regards, it should encompass much more than the shebang.

Copy link
Copy Markdown
Contributor

@akesandgren akesandgren Oct 24, 2022

Choose a reason for hiding this comment

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

@boegel you should know the answer, does fix_shebang here pick up things from the non-extension part?

Copy link
Copy Markdown
Member

@boegel boegel Oct 26, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, if fix_python_shebang_for (or another fix_*_shebang_for) is set for the "parent", then it will be picked up for all extensions as well.

All easyconfig parameters set for the parent are inherited by each extension, which is important in some cases (like toolchain for example). There are some exceptions though, see https://github.com/easybuilders/easybuild-framework/blob/95bae20337781a06defe467faf487a122aae8512/easybuild/framework/extension.py#L108

and https://github.com/easybuilders/easybuild-framework/blob/95bae20337781a06defe467faf487a122aae8512/easybuild/framework/extensioneasyblock.py#L81-L86

Changing this now (for EasyBuild v5.0) is worth considering.
There may be some fallout, but that can be handled via exts_default_options...

@mboisson If you would like to see this changed, please open a framework issue on this, so we can tag it with the 5.0 milestone.

Copy link
Copy Markdown
Contributor Author

@mboisson mboisson Oct 26, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I don't really care whether extensions pick up parameters from the parent recipe, but I do know we need to be able to fix shebangs of extensions. This change is in fact already merged in our fork.

My point is rather that picking up things from the parent recipe has nothing to do with this PR. This PR is entirely coherent with what is currently happening with other parameters.

Copy link
Copy Markdown
Member

@boegel boegel Nov 9, 2022

Choose a reason for hiding this comment

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

Yeah, I agree.

And even if fix_*_shebang is being picked up from the parent once this change is merged, it should still be OK. Worst case, the shebangs get fixed...

@boegel boegel changed the title adding fix_shebang to install_step so that we can fix shebangs when i… add fix_shebang to install_step of PythonPackage easyblock so that we can fix shebangs when installing extensions Feb 16, 2022
@boegel boegel added this to the 4.x milestone Feb 16, 2022
@boegel boegel modified the milestones: 4.x, release after 4.5.4 Mar 25, 2022
@boegel boegel modified the milestones: 4.5.5, release after 4.5.5 Jun 4, 2022
@boegel
Copy link
Copy Markdown
Member

boegel commented Nov 9, 2022

Test report by @boegel

Overview of tested easyconfigs (in order)

  • SUCCESS tqdm-4.64.0-GCCcore-11.3.0.eb
  • SUCCESS HTSeq-0.11.3-foss-2021b.eb
  • SUCCESS GTDB-Tk-2.0.0-intel-2021b.eb
  • SUCCESS matplotlib-3.4.2-foss-2021a.eb
  • SUCCESS numexpr-2.8.1-foss-2021a.eb
  • SUCCESS pybind11-2.9.2-GCCcore-11.3.0.eb

Build succeeded for 6 out of 6 (6 easyconfigs in total)
node2672.swalot.os - Linux RHEL 8.4, x86_64, Intel(R) Xeon(R) CPU E5-2660 v3 @ 2.60GHz (haswell), Python 3.6.8
See https://gist.github.com/fe59b6de0274bbaacf2e805d514925a6 for a full test report.

boegel
boegel approved these changes Nov 9, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@boegel boegel left a comment

Choose a reason for hiding this comment

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

lgtm

@boegel boegel merged commit c60fe17 into easybuilders:develop Nov 9, 2022
@mboisson mboisson deleted the fix_shebang_pythonpackage branch November 9, 2022 14:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@akesandgren akesandgren akesandgren left review comments

@boegel boegel boegel approved these changes

Assignees

No one assigned

Labels

enhancement

Projects

None yet

Milestone

4.7.0

Development

Successfully merging this pull request may close these issues.

3 participants

@mboisson @boegel @akesandgren