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

Install RHEL-8 pip packages in Dockerfile-RHEL. #774

Merged
merged 1 commit into from May 14, 2024

Conversation

clalancette
Copy link
Contributor

This is so that on RHEL-8, where we actually do
install pip packages, we install to user locations and not the system.

@clalancette
Copy link
Contributor Author

Here's a Humble+RHEL8 packaging job to see how this goes: Build Status

@clalancette clalancette force-pushed the clalancette/add-user-to-pip-install branch from 81328f0 to 761adee Compare April 25, 2024 15:06
@clalancette
Copy link
Contributor Author

And success there, so switching this to a real PR.

@clalancette clalancette marked this pull request as ready for review April 25, 2024 15:43
@clalancette
Copy link
Contributor Author

clalancette commented Apr 25, 2024

Here's the rest of CI (only on RHEL, since that is the only one affected):

CI:
Rolling+RHEL-9 amd64 CI: Build Status
Jazzy+RHEL-9 amd64 CI: Build Status
Iron+RHEL-9 amd64 CI: Build Status
Humble+RHEL-8 amd64 CI: Build Status

And packaging:

Rolling+RHEL-9 amd64: Build Status
Jazzy+RHEL-9 amd64: Build Status
Iron+RHEL-9 amd64: Build Status
Humble+RHEL-8 amd64: Build Status

@cottsay
Copy link
Member

cottsay commented Apr 25, 2024

Won't this cause the pip install operations that are part of this Dockerfile to happen under a different user account from the one we're actually running the job with?

Actually, the Humble+RHEL-8 amd64 job failed because of exactly this issue. The newer version of pytest isn't getting used.

@clalancette
Copy link
Contributor Author

Won't this cause the pip install operations that are part of this Dockerfile to happen under a different user account from the one we're actually running the job with?

Actually, the Humble+RHEL-8 amd64 job failed because of exactly this issue. The newer version of pytest isn't getting used.

Oh, hm. Possibly. Do you have suggestions on how to fix it? Ideas that I have include:

  1. Migrate all of the needed pip installs into Dockerfile-RHEL (though I'm not sure that would fix the issue)
  2. Add --user to the pip install commands in main.py (though I'm not sure what effect that will have on other platforms, including Windows).
  3. ???

@cottsay
Copy link
Member

cottsay commented Apr 25, 2024

If (2) is where the problem is, I feel like (2) should have the solution too. How is this not a problem on Ubuntu? Are they patching the default behavior of pip or something?

@clalancette
Copy link
Contributor Author

If (2) is where the problem is, I feel like (2) should have the solution too. How is this not a problem on Ubuntu? Are they patching the default behavior of pip or something?

We're getting away with it on Ubuntu Jammy and Ubuntu Noble because we aren't installing any pip packages at present.

I'm not sure why we aren't seeing a problem on RHEL-9, where we are installing some packages via pip (flake8-blind-except, flake8-class-newline, flake8-deprecated, nose, pep8, and colcon-ros-domain-id-coordinator).

@cottsay
Copy link
Member

cottsay commented Apr 25, 2024

We're getting away with it on Ubuntu Jammy and Ubuntu Noble because we aren't installing any pip packages at present.

Oof, seems sketchy, but I won't rock the boat.

Let's just move the new ENV line closer to the end of Dockerfile-RHEL and call it a day. The Dockerfile installs can continue to dump stuff in the system's pythonpath and the scripted installs can use PIP_USER.

@clalancette
Copy link
Contributor Author

clalancette commented Apr 25, 2024

Let's just move the new ENV line closer to the end of Dockerfile-RHEL and call it a day. The Dockerfile installs can continue to dump stuff in the system's pythonpath and the scripted installs can use PIP_USER.

Yep, got it. Done in a0fa99a, let's see if the failing CI is happy first, then I'll go run everything else:

Humble+RHEL-8 amd64 CI: Build Status

@clalancette
Copy link
Contributor Author

Here's a try with a totally different tactic; installing the pip packages inside the Dockerfile-RHEL for RHEL-8. This has some other disadvantages (namely duplication with what is in __main__.py), but this is kind of the direction I want to go in anyway.

Let's see how this works, and if this is better, we can discuss details:

Humble+RHEL-8 amd64 CI: Build Status

@clalancette
Copy link
Contributor Author

Another fix, another try:

Humble+RHEL-8 amd64 CI: Build Status

@clalancette
Copy link
Contributor Author

OK, so what is in 314e224 right now actually works. And while going that direction is my long-term goal, I think that for the purposes of fixing Humble here, it will be easier to just add a constraint in __main__.py for pycodestyle. I'm going to try that, which is going to mean we also need to run CI on Windows.

@clalancette clalancette force-pushed the clalancette/add-user-to-pip-install branch from 314e224 to 4347853 Compare April 26, 2024 21:08
@clalancette
Copy link
Contributor Author

Another fix, another try:

Humble+RHEL-8 amd64 CI: Build Status

@clalancette
Copy link
Contributor Author

clalancette commented Apr 29, 2024

CI again:

Humble+RHEL-8 amd64 CI: Build Status

@clalancette clalancette force-pushed the clalancette/add-user-to-pip-install branch from 2e9fb5b to bd953ea Compare April 29, 2024 13:48
@clalancette
Copy link
Contributor Author

CI again:

Humble+RHEL-8 amd64 CI: Build Status

It turns out that on RHEL-8, we need to install
a handful of packages from pip.  Doing this means
that the pip installation within __main__.py will
do exactly nothing, since all dependencies are
already installed.

This actually fixes two bugs:
1.  There is currently a bug where trying to run
'pip install foo' on RHEL-8 hits a permission denied
error.  This change avoids it by running that pip
install when we are effectively root in the container.
2.  Installing a too new version of pycodestyle doesn't
work with the pinned pytest that we are using.

Signed-off-by: Chris Lalancette <clalancette@gmail.com>
@clalancette clalancette force-pushed the clalancette/add-user-to-pip-install branch from 2b3f0df to a2e9ea8 Compare April 30, 2024 13:07
@clalancette clalancette changed the title Set the PIP_USER environment variable on RHEL. Install RHEL-8 pip packages in Dockerfile-RHEL. Apr 30, 2024
@clalancette
Copy link
Contributor Author

clalancette commented Apr 30, 2024

All right. After discussion and a lot of small PRs here, I think the solution that is implemented in this PR is right way to go. In particular, we pip install what we need during the Dockerfile-RHEL; see the commit message for more information on why we do that. @cottsay I could use review on this from you.

Here is CI on all flavors of RHEL:
Rolling+RHEL-9 amd64 CI: Build Status
Jazzy+RHEL-9 amd64 CI: Build Status
Iron+RHEL-9 amd64 CI: Build Status
Humble+RHEL-8 amd64 CI: Build Status

@sloretz
Copy link
Contributor

sloretz commented May 10, 2024

@cottsay friendly ping 🧇

@clalancette clalancette merged commit c1fbffe into master May 14, 2024
1 check passed
@clalancette clalancette deleted the clalancette/add-user-to-pip-install branch May 14, 2024 19:32
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.

None yet

3 participants