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

sci-physics/lhapdf: Use single python #29143

Closed
wants to merge 3 commits into from
Closed

Conversation

APN-Pucky
Copy link
Contributor

DISTUTILS_SINGLE_IMPL forces python to be the selected python version thereby fixing linked bug.

Bug: https://bugs.gentoo.org/870139
Signed-off-by: Alexander Puck Neuwirth alexander@neuwirth-informatik.de

@gentoo-bot
Copy link

Pull Request assignment

Submitter: @APN-Pucky
Areas affected: ebuilds
Packages affected: sci-physics/lhapdf

sci-physics/lhapdf: @gentoo/sci-physics

Linked bugs

Bugs linked: 870139


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 Jan 17, 2023

LICENSE="GPL-2"
SLOT="0"
KEYWORDS="~amd64"
Copy link
Member

Choose a reason for hiding this comment

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

We don't have to drop the ~x86 keyword here.


if use python; then
cd "${S}"/wrappers/python || die
distutils-r1_src_prepare
Copy link
Member

Choose a reason for hiding this comment

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

Because we are switching to single target we can probably simplify this ebuild by:

  • dropping all the if use python stuff and let $(use_enable python) take care of things.
  • switching from distutils-r1 eclass to python-single-r1 eclass. This would also get rid of the DistutilsNonPEP517Build warning.
    Similar to the 6.5.X ebuilds.

Or maybe even enable python unconditionally since this is what the 6.5.X ebuilds seem to do.

# Copyright 1999-2023 Gentoo Authors
# Distributed under the terms of the GNU General Public License v2

EAPI=7
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we want to bump it to EAPI=8 since we might have to hang on to this version for a while until python is fixed in the 6.5.X series.

Copy link
Contributor Author

@APN-Pucky APN-Pucky Jan 17, 2023

Choose a reason for hiding this comment

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

Yes, I initial tried sth. similar to the 6.5.X EAPI=8 ebuilds for 6.3.0, but since the python interface/wrapper changed it did not work out of the box.
Admittedly, this PR is just the fastest/easiest fix to the bug.

@AndrewAmmerlaan AndrewAmmerlaan self-assigned this Jan 17, 2023
@AndrewAmmerlaan
Copy link
Member

DISTUTILS_SINGLE_IMPL forces python to be the selected python version thereby fixing linked bug.

I suspect the underlying reason this fixes the problem is because switching to single targets makes the distutils eclass run the python_single-r1_pkg_setup function. Which sets the correct environment variables that are used by the econf.

@APN-Pucky
Copy link
Contributor Author

Adding python_for_impl before econf and emake instead of going to single python also worked, but the only current python is python3_9 so single python does the same in that regard.

@AndrewAmmerlaan
Copy link
Member

Adding python_for_impl before econf and emake instead of going to single python also worked, but the only current python is python3_9 so single python does the same in that regard.

Single is fine, I don't think there is anything depending on lhapdf[python] so there is not much benefit to installing for multiple targets here anyway.

@gentoo-repo-qa-bot
Copy link
Collaborator

Pull request CI report

Report generated at: 2023-01-17 13:53 UTC
Newest commit scanned: bd6b878
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/d5ce882f29/output.html


EAPI=7

PYTHON_COMPAT=( python3_{7..9} )
Copy link

Choose a reason for hiding this comment

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

I think you may want to update python version here, if possible. Both 7 and 8 are gone and 9 is near to its EOL

Copy link
Member

Choose a reason for hiding this comment

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

Unfortunately it does not compile with python3.10. For this package we have this version with working python bindings, but its stuck on 3.9. And we have the newer versions which compile with 3.10 and 3.11 but there's a serious bug in the python bindings: https://bugs.gentoo.org/875386.

Ideally we would patch this version to support 3.10 and 3.11, but I'm not sure if this is feasible.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I got python3.10 running with 6.3.0 now, will update this PR soon.
I got inspired by
https://gitlab.com/hepcedar/lhapdf/-/issues/13 to delete the wrappers/python/lhapdf.cpp
and let make reproduce it with a more recent cython (which becomes a new dependency for lhapdf then).

src_configure() {
CONFIG_SHELL="${EPREFIX}/bin/bash" \
econf \
--disable-static \
Copy link

Choose a reason for hiding this comment

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

If you'll move to EAPI=8 remember to drop this one as now it made by EAPI itself

Bug: https://bugs.gentoo.org/870139
Signed-off-by: Alexander Puck Neuwirth <alexander@neuwirth-informatik.de>
@APN-Pucky
Copy link
Contributor Author

APN-Pucky commented Jan 17, 2023

I could not get it working with the python-single-r1 eclass, so I sticked to the distutils-r1, but with PEP517 setuptools (still with easy_install command is deprecated. Use build and pip and other standards-based tools.).
Python is default enabled and might work with python3_11 as well, but I would have to unmask a lot on my system to test it.

@gentoo-repo-qa-bot
Copy link
Collaborator

Pull request CI report

Report generated at: 2023-01-17 16:53 UTC
Newest commit scanned: 10db2c3
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/004e1bd326/output.html

sci-physics/lhapdf/lhapdf-6.3.0-r1.ebuild Outdated Show resolved Hide resolved
REQUIRED_USE="${PYTHON_REQUIRED_USE}"

BDEPEND="
dev-libs/boost:=
Copy link
Member

Choose a reason for hiding this comment

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

Do we need boost? The newer versions have dropped this dependency. And if we need boost, should we also depend on boost[${PYTHON_USEDEP}]?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

At 6.3.0 they still have boost.m4 in the repo (https://gitlab.com/hepcedar/lhapdf/-/tree/lhapdf-6.3.0/m4), but the changelog suggests that since 2017-07-09 (6.2.0) no boost is needed. I tested it an no boost is needed.


cd "${S}"/wrappers/python || die
distutils-r1_src_install
python_optimize
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 we can drop this, distutils-r1_src_install should already take care of python_optimize.

@AndrewAmmerlaan
Copy link
Member

Python is default enabled and might work with python3_11 as well, but I would have to unmask a lot on my system to test it.

I'll test it with python3_11, I have this installed already anyway.

Co-authored-by: Andrew Ammerlaan <andrewammerlaan@gentoo.org>
Signed-off-by: Alexander Puck Neuwirth <APN-Pucky@users.noreply.github.com>
@gentoo-repo-qa-bot
Copy link
Collaborator

Pull request CI report

Report generated at: 2023-01-18 11:58 UTC
Newest commit scanned: 320086c
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/d8c912f4a9/output.html

Signed-off-by: Alexander Puck Neuwirth <alexander@neuwirth-informatik.de>
@gentoo-repo-qa-bot
Copy link
Collaborator

Pull request CI report

Report generated at: 2023-01-18 12:08 UTC
Newest commit scanned: 72b6414
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/f828b59abb/output.html

@AndrewAmmerlaan
Copy link
Member

Great work, Thanks 👍

I tested with python3_11 which worked just fine, so I added that target as well.

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
4 participants