cockpit: set `RPM_BUILD_ROOT` w/ installing by ptoscano · Pull Request #2989 · candlepin/subscription-manager

ptoscano · 2022-03-01T12:51:51Z

Due to a recent change in Fedora 36 [1], the installation prefix is
mangled in certain scenarios [2].

Hence, set a $RPM_BUILD_ROOT environment variable when invoking
setup.py install, so it works again as it used in older versions of
Fedora. This script is used only for the cockpit CI testing, so there
is no change for usual builds.

[1] https://src.fedoraproject.org/rpms/python3.10/pull-request/63
[2] https://bugzilla.redhat.com/show_bug.cgi?id=2026979

ptoscano · 2022-03-01T13:06:04Z

Forgot to mention, thanks @jelly for reporting this problem while adding fedora-36 to the cockpit CI images for subscription-manager [1]!

[1] cockpit-project/bots#3016

martinpitt

This is an absolutely terrible hack and workaround. I asked in pypa/distutils#70 (comment) , but it is still plain wrong to install custom stuff into /usr.

Can't you install this into /usr/local/ (the correct location for non-distribution code) and run tests with that? E.g. cockpit is totally fine with reading packages from /usr/local/share/cockpit/subscription-manager, and it prefers that over an existing /usr/share/cockpit/subscription-manager/.

martinpitt · 2022-03-01T13:28:56Z

Or rather, as it fails here:

+ mv /usr/bin/rhsm-facts-service /usr/bin/rhsm-service /usr/bin/rhsmcertd-worker /usr/libexec
mv: cannot stat '/usr/bin/rhsm-facts-service': No such file or directory
mv: cannot stat '/usr/bin/rhsm-service': No such file or directory
mv: cannot stat '/usr/bin/rhsmcertd-worker': No such file or directory

explicitly install into /usr/local and adjust the mv to move from /usr/local/bin/ ?

martinpitt · 2022-03-01T13:29:44Z

Or even better, actually call rpmbuild -tb on your make dist tarball to build rpms, and install these? This will then also validate your spec file on the corresponding distributions, which is an additional win

hroncok · 2022-03-01T13:37:50Z

For the record, I doubt this is due to the recent change in https://github.com/pypa/distutils -- we have carried this patch in distutils in the standard library since Fedora 27.

hroncok · 2022-03-01T13:41:02Z

Forgot to mention, thanks @jelly for reporting this problem while adding fedora-36 to the cockpit CI images for subscription-manager [1]!

[1] cockpit-project/bots#3016

If this indeed started to happen in Fedora 36, this is much more likely caused by https://src.fedoraproject.org/rpms/python3.10/pull-request/63 -- that change landed in rawhide 5 months ago and I am quite sad that you have only discovered this now.

See also https://discuss.python.org/t/mechanism-for-distributors-to-add-site-install-schemes-to-python-installations/8467/1 and https://bugs.python.org/issue43976

ptoscano · 2022-03-01T14:02:38Z

but it is still plain wrong to install custom stuff into /usr.

The environment is a testing VM, it does not matter that /usr is polluted. Also, it installed on /usr to make sure to properly override the system installation.

If this indeed started to happen in Fedora 36, this is much more likely caused by https://src.fedoraproject.org/rpms/python3.10/pull-request/63 -- that change landed in rawhide 5 months ago and I am quite sad that you have only discovered this now.

We don't regularly test on Dedora rawhide, and AFAIK the cockpit CI doesn't test on it because of "not exactly stable" (AFAICR).

Or even better, actually call rpmbuild -tb on your make dist tarball to build rpms, and install these? This will then also validate your spec file on the corresponding distributions, which is an additional win

We have already other CI jobs that check that the spec file works and the package build is correct. Personally I'd like to limit the scope of the testing of this as much as possible.

@hroncok
Let me say explicitly: I appreciate your work in trying to make Python usable in Fedora (and RHEL too), as good platform for the Python-based ecosystem. So I don't want to imply that this is a bad/negative critique on your work, be the upstream changes or the Fedora-specific one.
That said: even in the context of a distribution, I don't understand why explicit install locations are not properly honoured; the invocation here is a simple

python3 ./setup.py install --prefix /usr --root /

I get the reasons behind not installing to /usr but rather to /usr/local for "system installations". However, if I explicitly want to install in a certain location, including /usr, I'd expect it to be respected and not silently mangled. This breaks the usability of existing scripts (like this) that, for certain reasons, may want to explicitly install to /usr.

hroncok · 2022-03-01T14:12:20Z

I get the reasons behind not installing to /usr but rather to /usr/local for "system installations". However, if I explicitly want to install in a certain location, including /usr, I'd expect it to be respected and not silently mangled. This breaks the usability of existing scripts (like this) that, for certain reasons, may want to explicitly install to /usr.

I don't disagree. The culprit is in the fact that /local/ is technically not part of the prefix. See https://bugzilla.redhat.com/show_bug.cgi?id=2026979 for details.

ptoscano · 2022-03-01T15:35:05Z

I get the reasons behind not installing to /usr but rather to /usr/local for "system installations". However, if I explicitly want to install in a certain location, including /usr, I'd expect it to be respected and not silently mangled. This breaks the usability of existing scripts (like this) that, for certain reasons, may want to explicitly install to /usr.

I don't disagree. The culprit is in the fact that /local/ is technically not part of the prefix. See https://bugzilla.redhat.com/show_bug.cgi?id=2026979 for details.

Well, technically yes, however adding unexpected parts like this is not exactly a fair chance to the build system, IMHO.

Reading the bug makes me think that there is no solution for this in sight soon. Would it be OK from your POV if I amend the message of this to reflect the actual case, and keep the workaround?

hroncok · 2022-03-01T15:52:16Z

Would it be OK from your POV if I amend the message of this to reflect the actual case, and keep the workaround?

I care for the amended message because I think it's not good to reference an unrelated change in pypa/distutils. Thanks for that.

Whether or not you keep the workaround, I don't have a preference, as I don't have enough knowledge about your use case. If you think you must install to /usr directly, keep the workaround.

martinpitt · 2022-03-01T15:52:23Z

@ptoscano : What's wrong with just installing to /usr/local? (not by accident, but deliberately)?

hroncok · 2022-03-01T15:58:37Z

integration-tests/vm.install

 tar xzf ../subscription-manager-build-extra.tar.gz
 python3 ./setup.py build
-python3 ./setup.py install --prefix /usr --root /
+RPM_BUILD_ROOT="" python3 ./setup.py install --prefix /usr --root /


As for the workaround itself, I suggest the following:

Suggested change

RPM_BUILD_ROOT="" python3 ./setup.py install --prefix /usr --root /

# Simulate installation via RPM package by setting $RPM_BUILD_ROOT

# That way, we install to /usr instead of /usr/local, which is needed to ???

RPM_BUILD_ROOT=/ python3 ./setup.py install

Note three things:

the intent of setting $RPM_BUILD_ROOT is visible

the value of $RPM_BUILD_ROOT actually make sense (while technically, it doesn't matter)

the --prefix and --root arguments are probably useless so I've removed them

I can make $RPM_BUILD_ROOT more visible and more correct, however --prefix and --root have to stay as they are, as they are needed in older Fedora and RHEL version (the current cockpit testing matrix for main is fedora >= 34 and rhel >= 8.4).

as they are needed in older Fedora and RHEL version

Are you sure?

Root always defaults to /. And prefix defaults to /usr when $RPM_BUILD_ROOT is set.

Could you please a least try this?

Suggested change

RPM_BUILD_ROOT="" python3 ./setup.py install --prefix /usr --root /

RPM_BUILD_ROOT="/" python3 ./setup.py install

I need --root, as at least on RHEL >= 8.5 it did change the behaviour; see commit c7d74b7 (from #2840).

Mea culpa. I've just tested this on CentOS Stream 8 and there is indeed a very weird error happening without --root:

running install_data error: [Errno 2] No such file or directory: 'build/locale'

With --root /, it is gone. The reason appears to be that without --root the build process goes through bdist_egg and the build fails as your custom build class does not support that. It appears the --root option serves as another undocumented workaround here. Oh my.

So... should we go ahead with the current changes? (note I updated this PR a couple of hours ago)

I consider my concerns resolved. Note however that IMHO you are piling up workarounds on top of workarounds and actually building the RPM might be safer.

Well, the bigger workaround here is the sucky buildsystem, so there isn't much "good" doable when the baseline is already poor... :-(

ptoscano · 2022-03-01T16:04:32Z

@ptoscano : What's wrong with just installing to /usr/local? (not by accident, but deliberately)?

Note that this was added originally with #2091.

Personally speaking, I had bad past experiences with locally installed stuff in /usr/local that was supposed to shadow things in /usr but it really didn't, resulting in broken setups. That's why I never use /usr/local myself on my systems.

Another point is that sub-man has bits that have to be in certain system locations, e.g. the package manager plugins. Right now (because of the awful build system we have) they are not installed in the testing system during the cockpit testing, and this is a minor issue that so far has not hit us yet.

So we really need to keep doing system installations to fully test sub-man.

Due to a recent change in Fedora 36 [1], the installation prefix is mangled in certain scenarios [2]. Hence, set a $RPM_BUILD_ROOT environment variable when invoking 'setup.py install', so it works again as it used in older versions of Fedora. This script is used only for the cockpit CI testing, so there is no change for usual builds. [1] https://src.fedoraproject.org/rpms/python3.10/pull-request/63 [2] https://bugzilla.redhat.com/show_bug.cgi?id=2026979

martinpitt

Thanks! I still can't say I like it, but at least the situation is properly analyzed now, and it's the best you can do if you don't want to build/install an rpm. As a non-project member I can't formally approve, but I can at least revoke my "suggesting changes".

Thanks @hroncok for your help here!

jirihnidek

Thanks for all your reviews 👍

cnsnyder requested review from a team and jirihnidek and removed request for a team March 1, 2022 12:52

ptoscano mentioned this pull request Mar 1, 2022

testmap: add Fedora-36 to subscription-manager cockpit-project/bots#3016

Merged

martinpitt suggested changes Mar 1, 2022

View reviewed changes

hroncok reviewed Mar 1, 2022

View reviewed changes

ptoscano force-pushed the ptoscano/newer-distutils branch from eb26c57 to 763e0ca Compare March 1, 2022 16:22

martinpitt approved these changes Mar 2, 2022

View reviewed changes

jirihnidek approved these changes Mar 4, 2022

View reviewed changes

jirihnidek merged commit 9eb80b3 into main Mar 4, 2022

jirihnidek deleted the ptoscano/newer-distutils branch March 4, 2022 10:16

-RPM_BUILD_ROOT="" python3 ./setup.py install --prefix /usr --root /
+# Simulate installation via RPM package by setting $RPM_BUILD_ROOT
+# That way, we install to /usr instead of /usr/local, which is needed to ???
+RPM_BUILD_ROOT=/ python3 ./setup.py install

	RPM_BUILD_ROOT="" python3 ./setup.py install --prefix /usr --root /
	RPM_BUILD_ROOT="/" python3 ./setup.py install

Conversation

ptoscano commented Mar 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ptoscano commented Mar 1, 2022

Uh oh!

martinpitt left a comment

Choose a reason for hiding this comment

Uh oh!

martinpitt commented Mar 1, 2022

Uh oh!

martinpitt commented Mar 1, 2022

Uh oh!

hroncok commented Mar 1, 2022

Uh oh!

hroncok commented Mar 1, 2022

Uh oh!

ptoscano commented Mar 1, 2022

Uh oh!

hroncok commented Mar 1, 2022

Uh oh!

ptoscano commented Mar 1, 2022

Uh oh!

hroncok commented Mar 1, 2022

Uh oh!

martinpitt commented Mar 1, 2022

Uh oh!

hroncok Mar 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ptoscano Mar 1, 2022

Choose a reason for hiding this comment

Uh oh!

hroncok Mar 1, 2022

Choose a reason for hiding this comment

Uh oh!

hroncok Mar 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hroncok Mar 1, 2022

Choose a reason for hiding this comment

Uh oh!

ptoscano Mar 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hroncok Mar 1, 2022

Choose a reason for hiding this comment

Uh oh!

ptoscano Mar 1, 2022

Choose a reason for hiding this comment

Uh oh!

hroncok Mar 1, 2022

Choose a reason for hiding this comment

Uh oh!

ptoscano Mar 2, 2022

Choose a reason for hiding this comment

Uh oh!

ptoscano commented Mar 1, 2022

Uh oh!

martinpitt left a comment

Choose a reason for hiding this comment

Uh oh!

jirihnidek left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ptoscano commented Mar 1, 2022 •

edited

Loading

hroncok Mar 1, 2022 •

edited

Loading

hroncok Mar 1, 2022 •

edited

Loading

ptoscano Mar 1, 2022 •

edited

Loading