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

sys-apps/portage: Install dist-info #35647

Closed

Conversation

zmedico
Copy link
Member

@zmedico zmedico commented Mar 7, 2024

Use distutils-r1 for PEP517 mode to generate dist-info.

Use the meson build for system-wide install mode which overwrites files from the PEP517 build (we want at least installation.py to come from the system-wide mode since that controls evaluation of paths in the portage.const module).

For now, manually remove redundant files generated by the PEP517 build, and also omit these excluded files from dist-info RECORD files where a ../ prefix easily identifies them.

Bug: https://bugs.gentoo.org/920330

@gentoo-bot
Copy link

Pull Request assignment

Submitter: @zmedico
Areas affected: ebuilds
Packages affected: sys-apps/portage

sys-apps/portage: @gentoo/portage

Linked bugs

Bugs linked: 920330


In order to force reassignment and/or bug reference scan, please append [please reassign] to the pull request title.

Docs: Code of ConductCopyright policy (expl.) ● DevmanualGitHub PRsProxy-maint guide

@gentoo-bot gentoo-bot added assigned PR successfully assigned to the package maintainer(s). bug linked Bug/Closes found in footer, and cross-linked with the PR. labels Mar 7, 2024
@zmedico zmedico force-pushed the portage-install-dist-info-bug-920330 branch from 928a7fa to baed9cc Compare March 7, 2024 06:10
@gentoo-repo-qa-bot
Copy link
Collaborator

Pull request CI report

Report generated at: 2024-03-07 06:17 UTC
Newest commit scanned: 928a7fa
Status: ✅ good

There are existing issues already. Please look into the report to make sure none of them affect the packages in question:
https://qa-reports.gentoo.org/output/gentoo-ci/1f78bba998/output.html

Copy link
Contributor

@eli-schwartz eli-schwartz left a comment

Choose a reason for hiding this comment

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

As previously mentioned on the linked ticket:

Believe it or not, this was enough to satisfy pip check:

/usr/lib/python3.12/site-packages/portage-3.0.58.dist-info/METADATA

Metadata-Version: 2.1
Name: portage
Version: 3.0.58

So we could easily just write that out as part of the regular Portage build with some of the other fields for good measure.

Comment on lines 173 to 190
python_foreach_impl my_src_install
distutils-r1_src_install
# TODO: Add a setup arg to exclude this stuff.
rm -rf "${ED}/usr/"{bin,sbin,etc,lib/portage/bin} || die
# Omit excluded files that begin with ../ from RECORD files.
find "${ED}" -name RECORD | xargs sed -e '/^\.\.\//d' -i || die
BUILD_DIR="${S}-meson" python_foreach_impl my_src_install
Copy link
Contributor

Choose a reason for hiding this comment

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

It feels like this is trying very hard to hack how distutils works, simply to acquire the *.dist-info/ directory itself.

What is RECORD needed for here, anyways? Other than nothing. It's essentially pip trying to duplicate a vdb CONTENTS except not very well. It's even optional, per the spec -- in fact, all files in there are optional, other than METADATA.

The RECORD file is actually bad to have. It exists solely for pip to know how to uninstall the package using pip uninstall which obviously must never happen.

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's true. I've changed it to delete both the RECORD and entry_points.txt files (entry_points.txt refers to some entry points that do not all exist in the system-wide context).

Copy link
Member Author

Choose a reason for hiding this comment

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

This seems like extending a hacky reinvention of distutils-r1 logic in pure meson with hacky combination of distutils-r1 logic with previous hacky reinvention. Can't you just use DISTUTILS_ARGS (i.e. effectively meson args), to make meson do what you need it to do in PEP517 mode?

My understanding is that we can't install anything outside of sys.prefix in PEP517 mode (/usr on gentoo), so that would not account for installation of files into /etc. I suppose we could just use basic insinto and doins for /etc files though. How does that sound?

Copy link
Member

Choose a reason for hiding this comment

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

That, or just install them as usual and then mv in ebuild (that's how we deal with it in other packages).

Copy link
Contributor

Choose a reason for hiding this comment

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

We could just use basic insinto / doins for a METADATA file.

Actually possibly you could use install_data inside portage's own meson.build for this...

The only trick here is what information to include. There is very little metadata required to register a package with pip, most stuff -- like keywords and classifiers and license/author info -- is purely informative and intended for display on https://pypi.org

Copy link
Member

Choose a reason for hiding this comment

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

I prefer Eli's idea.

Copy link
Member Author

Choose a reason for hiding this comment

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

I also like Eli's idea to generate a METADATA file and install it with install_data, and we can use the path from get_install_dir to generate the install_data install_dir.

Copy link
Member Author

Choose a reason for hiding this comment

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

@zmedico zmedico force-pushed the portage-install-dist-info-bug-920330 branch from baed9cc to f9c8c8c Compare March 7, 2024 06:30
@gentoo-repo-qa-bot
Copy link
Collaborator

Pull request CI report

Report generated at: 2024-03-07 06:32 UTC
Newest commit scanned: baed9cc
Status: ✅ good

There are existing issues already. Please look into the report to make sure none of them affect the packages in question:
https://qa-reports.gentoo.org/output/gentoo-ci/6ff19f1407/output.html

Use distutils-r1 for PEP517 mode to generate dist-info.

Use the meson build for system-wide install mode which overwrites
files from the PEP517 build (we want at least installation.py to
come from the system-wide mode since that controls evaluation of
paths in the portage.const module).

For now, manually remove redundant files generated by the PEP517
build, and also omit undesirable RECORD and entry_points.txt
files from dist-info.

Bug: https://bugs.gentoo.org/920330
Signed-off-by: Zac Medico <zmedico@gentoo.org>
@zmedico zmedico force-pushed the portage-install-dist-info-bug-920330 branch from f9c8c8c to 06f8343 Compare March 7, 2024 06:36
@gentoo-repo-qa-bot
Copy link
Collaborator

Pull request CI report

Report generated at: 2024-03-07 07:02 UTC
Newest commit scanned: 06f8343
Status: ✅ good

There are existing issues already. Please look into the report to make sure none of them affect the packages in question:
https://qa-reports.gentoo.org/output/gentoo-ci/8bb43c38c4/output.html

Copy link
Member

@mgorny mgorny left a comment

Choose a reason for hiding this comment

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

This seems like extending a hacky reinvention of distutils-r1 logic in pure meson with hacky combination of distutils-r1 logic with previous hacky reinvention. Can't you just use DISTUTILS_ARGS (i.e. effectively meson args), to make meson do what you need it to do in PEP517 mode?

@zmedico zmedico marked this pull request as draft March 7, 2024 20:07
@zmedico
Copy link
Member Author

zmedico commented Mar 8, 2024

Closed in favor of gentoo/portage#1303.

@zmedico zmedico closed this Mar 8, 2024
@zmedico zmedico deleted the portage-install-dist-info-bug-920330 branch March 8, 2024 16:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
assigned PR successfully assigned to the package maintainer(s). bug linked Bug/Closes found in footer, and cross-linked with the PR.
Projects
None yet
6 participants