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

modules: python: better handling of the Python paths for Debian #9288

Merged
merged 2 commits into from
Sep 29, 2021

Conversation

FFY00
Copy link
Member

@FFY00 FFY00 commented Sep 21, 2021

Hardcoding the name is fragile, and enabling it based on the existence of
/etc/debian_version (as the is_debianlike helper does) will result in
incorrect paths if the Python binary is not provided by Debian.

Using the deb_system distutils scheme instead makes sure we use the
install path from the current interpreter, which Debian could change
between releases, and gives us the correct value on Python installations
that are not provided by Debian (eg. deadsnakes, Github Action Python,
etc.)

Do notice, though, that there is still no guarantee that these are the
correct paths, as they assume all schemes paths have the install prefix
as a base, see #9284.

Signed-off-by: Filipe Laíns lains@riseup.net

@FFY00 FFY00 requested a review from jpakkane as a code owner September 21, 2021 19:09
@codecov
Copy link

codecov bot commented Sep 21, 2021

Codecov Report

Merging #9288 (83be616) into master (68eca11) will increase coverage by 0.02%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #9288      +/-   ##
==========================================
+ Coverage   66.85%   66.87%   +0.02%     
==========================================
  Files         386      388       +2     
  Lines       85062    85247     +185     
  Branches    17551    17515      -36     
==========================================
+ Hits        56869    57011     +142     
- Misses      23401    23494      +93     
+ Partials     4792     4742      -50     
Impacted Files Coverage Δ
mesonbuild/modules/python.py 65.39% <ø> (-0.18%) ⬇️
mesonbuild/scripts/vcstagger.py 87.50% <0.00%> (-4.17%) ⬇️
compilers/cython.py 43.18% <0.00%> (-3.16%) ⬇️
interpreterbase/_unholder.py 68.42% <0.00%> (-3.01%) ⬇️
mesonbuild/interpreterbase/_unholder.py 68.42% <0.00%> (-3.01%) ⬇️
interpreterbase/interpreterbase.py 68.09% <0.00%> (-2.56%) ⬇️
mesonbuild/interpreterbase/interpreterbase.py 68.09% <0.00%> (-2.56%) ⬇️
compilers/rust.py 76.47% <0.00%> (-1.70%) ⬇️
mesonbuild/interpreterbase/decorators.py 89.10% <0.00%> (-1.56%) ⬇️
interpreterbase/decorators.py 89.10% <0.00%> (-1.01%) ⬇️
... and 64 more

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 68eca11...83be616. Read the comment docs.

@FFY00 FFY00 force-pushed the better-debian-handling branch 2 times, most recently from 8cee5a7 to 33755fd Compare September 22, 2021 13:06
@xclaesse
Copy link
Member

Gave this a try on my Windows machine too:

>>> import distutils.command.install
>>> distribution = distutils.dist.Distribution()
>>> install_cmd = distribution.get_command_obj('install')
>>> install_cmd.prefix = ''
>>> install_cmd.finalize_options()
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "C:\Python38\lib\distutils\command\install.py", line 344, in finalize_options
    self.convert_paths('lib', 'purelib', 'platlib',
  File "C:\Python38\lib\distutils\command\install.py", line 487, in convert_paths
    setattr(self, attr, convert_path(getattr(self, attr)))
  File "C:\Python38\lib\distutils\util.py", line 122, in convert_path
    raise ValueError("path '%s' cannot be absolute" % pathname)
ValueError: path '/Lib/site-packages' cannot be absolute

@xclaesse
Copy link
Member

OOC, that's the plan with distutils upstream? They could remove the module completely, so we should guard the import behind try/except and fallback to sysconfig? Also, other than deb_system, are there any other case where sysconfig is not enough?

@FFY00
Copy link
Member Author

FFY00 commented Sep 22, 2021

Yes, 1sec. I will push my code to handle all that.

@xclaesse
Copy link
Member

I'm also wondering if we should actually allow installing python modules outside of Meson's prefix. Thinking about Windows where they should go into C:\Python38\Lib\site-packages. But that's a bridge we could cross later.

@FFY00 FFY00 force-pushed the better-debian-handling branch from 33755fd to e204313 Compare September 22, 2021 16:37
@FFY00 FFY00 force-pushed the better-debian-handling branch 3 times, most recently from 563923d to faed5be Compare September 22, 2021 17:38
raise ModuleNotFoundError('Unable to import distutils, please install python3-distutils')
debian_distutils = 'deb_system' in distutils.command.install.INSTALL_SCHEMES
except ModuleNotFoundError:
debian_distutils = False
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we need a try inside a try, import distutils should always work on all platforms, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not after 3.12.

Copy link
Member Author

Choose a reason for hiding this comment

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

Unless someone is naughty enough to create a Python package that provides the same module.

Copy link
Member

Choose a reason for hiding this comment

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

In that case we'll need to fix links_against_libpython() too, let's cross that bridge when/if we get there? OTOH, this is getting petty nitpicking I admit ;P

OOC, how confident are we that it's going to be removed in 3.12? It has been formally announced already? When that happens, what's the likely outcome for Debian, they'll patch sysconfig instead?

Sorry again for all my questions, I wasn't familiar with this topic, so I like to learned it a bit :)

Copy link
Member Author

Choose a reason for hiding this comment

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

In that case we'll need to fix links_against_libpython() too, let's cross that bridge when/if we get there? OTOH, this is getting petty nitpicking I admit ;P

I'd prefer doing in a different PR as it is not actually an issue right now, while this is. We can fix the current handling of the Debian install paths and then explore how to fase out distutils entirely.

OOC, how confident are we that it's going to be removed in 3.12?

Very.

It has been formally announced already? When that happens, what's the likely outcome for Debian, they'll patch sysconfig instead?

Yes, my best guess is either PEP 668 or that, or both.
Also related: https://bugs.python.org/issue43976 (maybe skip down to the latest discussion)

Sorry again for all my questions, I wasn't familiar with this topic, so I like to learned it a bit :)

No worries, I have been dealing with these issues for a little while and still struggle to keep track of everything.

@FFY00
Copy link
Member Author

FFY00 commented Sep 23, 2021

@xclaesse can you have a look? I think this is essentially ready.

@FFY00 FFY00 force-pushed the better-debian-handling branch from df44630 to 18ddc57 Compare September 24, 2021 14:27
@FFY00
Copy link
Member Author

FFY00 commented Sep 24, 2021

I pushed a fix hopefully dealing with the failing tests due to old Python versions.

@eli-schwartz I will submit the improvements to the message in a different pull request, if that's alright with you, as we probably want to have a different discussion and review for that. Hopefully, we can merge this ASAP.

@FFY00 FFY00 force-pushed the better-debian-handling branch from 18ddc57 to bb27cbd Compare September 29, 2021 00:35
@FFY00 FFY00 requested a review from eli-schwartz September 29, 2021 00:35
@FFY00
Copy link
Member Author

FFY00 commented Sep 29, 2021

Sorry for the delay, I have been busy. I removed the try .. except block for the Debian stuff from this PR, I'll see if I have time to open a new PR next week.

Hardcoding the name is fragile, and enabling it based on the existence of
/etc/debian_version (as the is_debianlike helper does) will result in
incorrect paths if the Python binary is not provided by Debian.

Using the deb_system distuils scheme instead makes sure we use the
install path from the current interpreter, which Debian could change
between releases, and gives us the correct value on Python installations
that are not provided by Debian (eg. deadsnakes, Github Action Python,
etc.)

Do notice, though, that there is still no guarantee that these are the
correct paths, as they assume all schemes paths have the install prefix
as a base, see mesonbuild#9284.

Signed-off-by: Filipe Laíns <lains@riseup.net>
Signed-off-by: Filipe Laíns <lains@riseup.net>
@FFY00 FFY00 force-pushed the better-debian-handling branch from bb27cbd to 83be616 Compare September 29, 2021 00:48
@xclaesse
Copy link
Member

Looks good to me, thanks!

@xclaesse xclaesse merged commit 59b8e00 into mesonbuild:master Sep 29, 2021
@eli-schwartz
Copy link
Member

Thanks! Indeed this looks good to me too.

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.

4 participants