Skip to content

Conversation

eli-schwartz
Copy link
Member

@eli-schwartz eli-schwartz commented Sep 6, 2022

Fixes #10523

/cc @rgommers

@codecov
Copy link

codecov bot commented Sep 6, 2022

Codecov Report

Merging #10783 (eb69fed) into master (b8e53ed) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master   #10783   +/-   ##
=======================================
  Coverage   68.99%   68.99%           
=======================================
  Files         406      406           
  Lines       88630    88644   +14     
  Branches    19665    19665           
=======================================
+ Hits        61148    61164   +16     
+ Misses      22883    22881    -2     
  Partials     4599     4599           
Impacted Files Coverage Δ
mesonbuild/modules/python.py 74.25% <100.00%> (+0.60%) ⬆️
modules/python.py 69.57% <0.00%> (+0.67%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Copy link
Contributor

@rgommers rgommers left a comment

Choose a reason for hiding this comment

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

This is great, thanks @eli-schwartz. I tested it with SciPy, it allowed me to remove 102 lines of pure: false, and it caught a pretty bad bug (I had carefully checked all my install_sources() calls, but not get_install_dir()). Using find_installation(pure: false) removed a couple of stray {py_purelib} occurrences from intro-install_plan.json.


py_plat = import('python').find_installation(pure: false)
py_plat.install_sources('test.py', subdir: 'kw')
py_plat.install_sources('test.py', pure: true, subdir: 'kwrevert')
Copy link
Contributor

Choose a reason for hiding this comment

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

May be good to add a test file using get_install_dir as well?

Copy link
Contributor

Choose a reason for hiding this comment

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

@eli-schwartz I just discovered that this PR in its current state actually breaks get_install_dir(pure: false). To reproduce:

$ # use current SciPy `main`
$ meson setup build --prefix=$PWD/build-install
$ grep purelib build/meson-info/intro-install_plan.json | wc -l
# will give 0 with meson master, and 1 with this PR

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, that was dumb of me and happened because I didn't fully revert my first attempt.

@rgommers
Copy link
Contributor

rgommers commented Sep 6, 2022

This will close gh-10523.

@eli-schwartz
Copy link
Member Author

Thanks, I forgot there was an issue for it lol.

@eli-schwartz eli-schwartz force-pushed the python-installation-default-pure branch from 9ea99ce to dee113b Compare September 6, 2022 19:33


_PURE_KW = KwargInfo('pure', bool, default=True)
_PURE_KW = KwargInfo('pure', (bool, NoneType))
Copy link
Member

@dcbaker dcbaker Sep 9, 2022

Choose a reason for hiding this comment

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

You need to update the TypedDict annotations as well, and there are two, bot the PytInstallKw is now wrong because pure is Optional, and the FindInstallationKw needs the pure kw added

Copy link
Member

@dcbaker dcbaker left a comment

Choose a reason for hiding this comment

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

There's a couple of small typing issues, otherwise this LGTM

@eli-schwartz eli-schwartz force-pushed the python-installation-default-pure branch from dee113b to eb69fed Compare September 20, 2022 01:14
@eli-schwartz eli-schwartz merged commit eb69fed into mesonbuild:master Sep 20, 2022
@eli-schwartz eli-schwartz deleted the python-installation-default-pure branch September 20, 2022 10:57
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.

Add pure setting to python_installation object

3 participants