PR#41: Use __isa_bits instead of crafted value - rpms/python37

rpms / python37

#41 Use __isa_bits instead of crafted value

Merged 4 years ago by cstratak. Opened 4 years ago by kwizart.

rpms/ kwizart/python37 isa_bits_fix into master

Use __isa_bits instead of crafted value

Nicolas Chauvet • 4 years ago

709de90

python37.spec

file modified

+1 -11

		`@@ -132,16 +132,6 @@`
		`# Tracked at https://bugzilla.redhat.com/show_bug.cgi?id=1724753`
		`%global source_date_epoch_from_changelog 0`

		`- # For multilib support, files that are different between 32- and 64-bit arches`
		`- # need different filenames. Use "64" or "32" according to the word size.`
		`- # Currently, the best way to determine an architecture's word size happens to`
		`- # be checking %%{_lib}.`
		`- %if "%{_lib}" == "lib64"`
		`- %global wordsize 64`
		`- %else`
		`- %global wordsize 32`
		`- %endif`
		`-`

		`# =======================`
		`# Build-time requirements`
		`@@ -786,7 +776,7 @@`
		`# Filanames are defined here:`
		`%global _pyconfig32_h pyconfig-32.h`
		`%global _pyconfig64_h pyconfig-64.h`
		`- %global _pyconfig_h pyconfig-%{wordsize}.h`
		`+ %global _pyconfig_h pyconfig-%{__isa_bits}.h`

		`# Use a common function to do an install for all our configurations:`
		`InstallPython() {`

kwizart commented 4 years ago

The assumption to rely on %%{_lib} as lib64 where wordsize is 64bit is
wrong on aarch64 and ppc64le where wordsize is 64bit and lib is used.

Signed-off-by: Nicolas Chauvet kwizart@gmail.com

kwizart commented 4 years ago

Please note that this change wasn't tested and specially I haven't looked into assumption made in others python37 dependencies.

kwizart commented 4 years ago

And the same issue occurs for the python3 package...

churchyard commented 4 years ago

is wrong on aarch64 and ppc64le where wordsize is 64bit and lib is used.

What?

$ rpm -qlp glibc-2.31-1.fc32.aarch64.rpm | grep /usr/lib
...
/usr/lib64/gconv/UHC.so
/usr/lib64/gconv/UNICODE.so
...

kwizart commented 4 years ago

Well, Indeed I thought early aarch64 (and ppc64le) fedora versions were using lib instead or lib64. But looking down to fc27 I agree that lib64 is used there.

With that said, __isa_bits is really what you need to use here, would you pick this change if I'm updating the comment ?

kwizart commented 4 years ago

(and as a side note I wonder if the python SIG plans to maintain a python37 on epel7 ?)

churchyard commented 4 years ago

In aarch64 rawhide mock:

# aarch64-test01.fedorainfracloud.org
<mock-chroot> sh-5.0# rpm --eval '%{_lib} %{__isa_bits} %{_arch}'
lib64 64 aarch64
<mock-chroot> sh-5.0# rpm -q rpm
rpm-4.15.1-2.fc32.1.aarch64

In ppc64le rawhide mock:

# ppc64le-test.fedorainfracloud.org
<mock-chroot> sh-5.0# rpm --eval '%{_lib} %{__isa_bits} %{_arch}'
lib64 64 ppc64le
<mock-chroot> sh-5.0# rpm -q rpm
rpm-4.15.1-2.fc32.1.ppc64le

Your code is indeed easier than the current one, but how is the current one broken?

kwizart commented 4 years ago

Your code is indeed easier than the current one, but how is the current one broken?

All agreed, can you read again my previous comment.

Edited 4 years ago by kwizart

zuul commented 4 years ago

Build succeeded.

rpm-scratch-build : SUCCESS in 1h 17m 18s
rpm-linter : SUCCESS in 6m 18s
rpm-rpminspect : SUCCESS in 8m 34s (non-voting)
rpm-test : SUCCESS in 7m 03s

churchyard commented 4 years ago

All agreed, can you read again my previous comment.

I've send the second comment prior to seeing your reply to the first (Pagure did not update the comments).

With that said, __isa_bits is really what you need to use here, would you pick this change if I'm updating the comment ?

Yes, we'd take this if the comment is updated. I suggest starting with python39 and we can take it downwards, but starting with python37 is also fine if easier for you.

(and as a side note I wonder if the python SIG plans to maintain a python37 on epel7 ?)

We are ready to help if EPEL is interested: https://lists.fedoraproject.org/archives/list/epel-devel@lists.fedoraproject.org/thread/V336LVAXNJKVRECQ7XRLKICQIMVBDUAR/#V336LVAXNJKVRECQ7XRLKICQIMVBDUAR

cstratak commented 4 years ago

Nice one, I do like changes that make our SPEC's cleaner. As mentioned before, could you change the commit message to reflect the situation and maybe also send a PR for the rest of the Pythons?

rebased onto 709de90

4 years ago

kwizart commented 4 years ago

Okay, I've force-pushed the commit with a new comment.
I will prepare the same for python3.

cstratak commented 4 years ago

It looks good to me. Miro if you are ok with it, I'm gonna go ahead and merge it.

churchyard commented 4 years ago

Sure, go ahead. And @kwizart thanks for this!

cstratak commented 4 years ago

Thank you for your contribution!