Skip to content

Add install-tags filtering for wheel generation #267

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

Closed
wants to merge 6 commits into from

Conversation

Brishen
Copy link

@Brishen Brishen commented Jan 17, 2023

This is a PR for #68 comprising entirely of @peter-urban's work.

peter-urban added a commit to themachinethatgoesping/meson-python that referenced this pull request Jan 17, 2023
Comment on lines -879 to +884
for cmd in self.build_commands():
self._meson(*cmd[1:])
self._meson('compile', *self._meson_args['compile'],)
if self._meson_args['install-tags']:
install_tags = '--tags=' + ','.join(self._meson_args['install-tags'])
self._meson('install', '--destdir', os.fspath(self._install_dir), install_tags, *self._meson_args['install'],)
else:
self._meson('install', '--destdir', os.fspath(self._install_dir), *self._meson_args['install'],)
Copy link
Member

Choose a reason for hiding this comment

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

I think this may have happened because of the merge, but this code now needs to go into build_commands.

Copy link
Contributor

Choose a reason for hiding this comment

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

True, this is a merge error, my code was still based on a very old version of meson-python

Copy link
Contributor

@peter-urban peter-urban Jan 18, 2023

Choose a reason for hiding this comment

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

if install-tags is not separated from the "install" args, this whole block can be reverted to
for cmd in self.build_commands(): self._meson(*cmd[1:])

@@ -631,7 +631,7 @@ def build_editable(self, directory: Path, verbose: bool = False) -> pathlib.Path
return wheel_file


MesonArgsKeys = Literal['dist', 'setup', 'compile', 'install']
MesonArgsKeys = Literal['dist', 'setup', 'compile', 'install', 'install-tags']
Copy link
Member

Choose a reason for hiding this comment

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

I wonder it wouldn't just be better to parse this from the install args.

Copy link
Contributor

Choose a reason for hiding this comment

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

good point, I'll implement this

Suggested change
MesonArgsKeys = Literal['dist', 'setup', 'compile', 'install', 'install-tags']
MesonArgsKeys = Literal['dist', 'setup', 'compile', 'install']

Copy link
Member

Choose a reason for hiding this comment

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

Thanks! This can be done with argparse and parse_known_args.

Comment on lines 909 to +915
def _install_plan(self) -> Dict[str, Dict[str, Dict[str, str]]]:
install_plan = self._info('intro-install_plan').copy()

for files in install_plan.values():
for file, details in list(files.items()):
if details['tag'] not in self._meson_args['install-tags']:
del files[file]
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure if we should do it here.

Copy link
Contributor

Choose a reason for hiding this comment

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

i updated this block to parse the "install" args instead of using an extra 'install-tags' field

Suggested change
def _install_plan(self) -> Dict[str, Dict[str, Dict[str, str]]]:
install_plan = self._info('intro-install_plan').copy()
for files in install_plan.values():
for file, details in list(files.items()):
if details['tag'] not in self._meson_args['install-tags']:
del files[file]
def _install_plan(self) -> Dict[str, Dict[str, Dict[str, str]]]:
"""Meson install_plan metadata."""
# copy the install plan so we can modify it
install_plan = self._info('intro-install_plan').copy()
# parse install args for install tags (--tags)
install_tags = []
for arg in self._meson_args['install']:
if arg.strip().startswith('--tags='):
install_tags = arg.split('=', 1)[1].split(',')
break
else:
return install_plan
# filter out files that do not fit the install tags
for files in install_plan.values():
for file, details in list(files.items()):
if details['tag'].strip() not in install_tags:
del files[file]
return install_plan

Copy link
Contributor

Choose a reason for hiding this comment

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

@FFY00 This is just a suggestion (it was easy to implement it here).
If you give me some hints or thoughts on your doubts I could look for a better place to put this filter.

Copy link
Member

Choose a reason for hiding this comment

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

This is a bit fragile, and I'd prefer not having to maintain it. I think it should be pretty easy to use argparse, as I mentioned in #267 (comment). What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh ja, I didn't see the comment 🙂 I'll update the suggestion tomorrow

Copy link
Member

Choose a reason for hiding this comment

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

No worries. Thank you 😊

Copy link
Contributor

@peter-urban peter-urban Jan 18, 2023

Choose a reason for hiding this comment

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

Updated the suggestion to use argparse

Suggested change
def _install_plan(self) -> Dict[str, Dict[str, Dict[str, str]]]:
install_plan = self._info('intro-install_plan').copy()
for files in install_plan.values():
for file, details in list(files.items()):
if details['tag'] not in self._meson_args['install-tags']:
del files[file]
def _install_plan(self) -> Dict[str, Dict[str, Dict[str, str]]]:
"""Meson install_plan metadata."""
# copy the install plan so we can modify it
install_plan = self._info('intro-install_plan').copy()
# parse install args for install tags (--tags)
parser = argparse.ArgumentParser()
parser.add_argument('--tags')
args, _ = parser.parse_known_args(self._meson_args['install'])
# filter the install_plan for files that do not fit the install tags
if args.tags:
install_tags = args.tags.split(',')
for files in install_plan.values():
for file, details in list(files.items()):
if details['tag'].strip() not in install_tags:
del files[file]
return install_plan

naturally this needs import argparse at the beginning of the file

@peter-urban
Copy link
Contributor

@Brishen I implemented the above made code suggestions in my fork. Could you update your PR? :-)

@peter-urban
Copy link
Contributor

@Brishen is it possible for you to update this pull request to adapt it to FFY00s comments?
You could again merge the it again against the main branch of my fork. :-)
Cheers

@peter-urban
Copy link
Contributor

Hi @Brishen, it seems that you have no time to update this PR. I grew a bit impatiens and, since this PR is based on my changes anyways, took the liberty to create a new, updated PR (#286)

Hope that is no problem for you :-)

@FFY00
Copy link
Member

FFY00 commented Feb 1, 2023

Closed in favor of #288.

@FFY00 FFY00 closed this Feb 1, 2023
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.

3 participants