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

arc_summary: prefer python3 version and install when there is no python #8851

Merged
merged 1 commit into from
Jun 10, 2019

Conversation

eli-schwartz
Copy link
Contributor

Motivation and Context

Fixes installation in a minimal build chroot without python available.

Description

This matches the behavior of other python scripts, such as arcstat and dbufstat, which are always installed but whose install-exec-hook actions will simply touch up the shebang if a python interpreter was configured and that interpreter is a python2 interpreter.

How Has This Been Tested?

When configuring zfs with --enable-pyzfs=no, and with --with-python="$PWD/python3-fake" in order to fool the configure script into thinking python is available in order to work around #8731, the arc_summary script was not installed, while arcstat was installed (and used a python3 shebang). With this change, they are both installed, with a python3 shebang.

Tested in a minimal Arch Linux chroot without python available.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Performance enhancement (non-breaking change which improves efficiency)
  • Code cleanup (non-breaking change which makes code smaller or more readable)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation (a change to man pages or other documentation)

Checklist:

This matches the behavior of other python scripts, such as arcstat and
dbufstat, which are always installed but whose install-exec-hook actions
will simply touch up the shebang if a python interpreter was configured
*and* that interpreter is a python2 interpreter.

Fixes installation in a minimal build chroot without python available.

Signed-off-by: Eli Schwartz <eschwartz@archlinux.org>
@codecov
Copy link

codecov bot commented Jun 4, 2019

Codecov Report

Merging #8851 into master will increase coverage by 0.14%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #8851      +/-   ##
==========================================
+ Coverage   78.81%   78.96%   +0.14%     
==========================================
  Files         382      382              
  Lines      117771   117771              
==========================================
+ Hits        92821    92997     +176     
+ Misses      24950    24774     -176
Flag Coverage Δ
#kernel 79.45% <ø> (+0.07%) ⬆️
#user 67.95% <ø> (+0.37%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a3c98d5...ddd0bc7. Read the comment docs.

@behlendorf behlendorf added the Type: Building Indicates an issue related to building binaries label Jun 4, 2019
@behlendorf
Copy link
Contributor

@freqlabs would you mind reviewing this change related to #8731.

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

With #8731 merged, none of the Python scripts will be installed when ZFS is configured without Python, and you won't be able to fool the configure script anymore. I think the most straightforward solution is to have the desired version of Python installed when building ZFS if you intend to install the Python scripts.

@eli-schwartz
Copy link
Contributor Author

Hmm, I missed that that makes SUBDIRS conditional.

@eli-schwartz
Copy link
Contributor Author

Actually, this works fine:

./configure --with-python=$PWD/python3-fake PYTHON=$PWD/python3-fake --enable-pyzfs=no

@eli-schwartz eli-schwartz reopened this Jun 5, 2019
@behlendorf
Copy link
Contributor

As I understand it the intent here is to default to installing the python3 version when a non-standard python version has be specified. That seems entirely reasonable to me since it's most likely to be correct version and won't cause any harm.

@ghost
Copy link

ghost commented Jun 5, 2019

@eli-schwartz I get this error with that command:
configure: error: Cannot find /home/ryan/ZoF/python3-fake in your system path

@eli-schwartz
Copy link
Contributor Author

eli-schwartz commented Jun 5, 2019

That comes from the pyzfs code, though. (At least the error message is from AX_PYTHON_DEVEL which is part of config/always-pyzfs.m4)

@ghost
Copy link

ghost commented Jun 5, 2019

I ran exactly the command you gave. This comes from on line 72 in config/always-python.m4. Make sure you are testing with the commit that was merged for #8731

@behlendorf
Copy link
Contributor

Without this change applied, and assuming the $PWD/python3-fake exists and is executable; then I can confirm that arc_summary does not get installed (but arcstat and dbufstat do).

So while I think it would be best to build with Python installed in the environment, if a fake version is specified we do still want what's installed to be consistent. So this change makes sense to me. For example, with this change applied:

$ touch $PWD/python3-fake
$ chmod 775 $PWD/python3-fake
$ ./configure --with-python=$PWD/python3-fake PYTHON=$PWD/python3-fake --enable-pyzfs=no
$ make
$ DESTDIR=/var/tmp/zfs-install make install
$ ls /var/tmp/zfs-install/usr/local/bin/
arcstat  arc_summary  dbufstat  raidz_test  zgenhostid

@ghost
Copy link

ghost commented Jun 7, 2019

I am working on a solution that removes the need to have Python at configure time when using --disable-pyzfs. The goal is to allow for example --with-python=python2 or --with-python=python3.4 and only check the version number in the string, instead of trying to find that actual executable to get a version number. That removes the need to create a dummy python executable just to trick the configure script. In doing so, I make sure that PYTHON_VERSION is set in such a way that the existing tests against that variable will work. I am planning to do the PR for this in the next few days.

I have no objection to this PR, if the idea was to have created a dummy python executable beforehand, which I had not done when I tested it.

@behlendorf behlendorf added Status: Code Review Needed Ready for review and testing Status: Accepted Ready to integrate (reviewed, tested) and removed Status: Code Review Needed Ready for review and testing labels Jun 7, 2019
@behlendorf behlendorf merged commit 215e4fe into openzfs:master Jun 10, 2019
behlendorf pushed a commit that referenced this pull request Jun 11, 2019
This matches the behavior of other python scripts, such as arcstat and
dbufstat, which are always installed but whose install-exec-hook actions
will simply touch up the shebang if a python interpreter was configured
*and* that interpreter is a python2 interpreter.

Fixes installation in a minimal build chroot without python available.

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Reviewed-by: Ryan Moeller <ryan@freqlabs.com>
Signed-off-by: Eli Schwartz <eschwartz@archlinux.org>
Closes #8851
@ghost ghost mentioned this pull request Jun 12, 2019
12 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Accepted Ready to integrate (reviewed, tested) Type: Building Indicates an issue related to building binaries
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants