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

Add RHEL support to the python feature #830

Conversation

jdputschadi
Copy link
Contributor

This PR adds RHEL support to the python feature.

CentOS 7, RHEL/Alma/Rocky Linux 8 & 9, Mariner, and Fedora should all work. Tests for all of these are included.

Note:

Currently the python feature only supports Debian/Ubuntu-based distributions with the apt based systems, after this change those systems will continue to work as will RedHat systems. There is user visible change for systems that are not explicity supported: they will get an error message stating "Linux distro XXXX not supported."

This is instead of "odd" messages about not being able to find apt-get on unsupported systems.

resolves #829

@jdputschadi jdputschadi requested a review from a team as a code owner February 1, 2024 20:48
@jdputschadi
Copy link
Contributor Author

I see the cause of the error in the "checks" and will update the submitted code to not fail.

@samruddhikhandale
Copy link
Member

samruddhikhandale commented Feb 1, 2024

FYI - #577 is a known issue and tests against bookworm are currently failing. No pressure in fixing that, but it would be amazing if you could. ✨

@jdputschadi
Copy link
Contributor Author

jdputschadi commented Feb 2, 2024

Commit 4007701 should fix the previously failing checks/tests. If so, the python feature portions of #577 would also be resolved by this PR.

@jdputschadi
Copy link
Contributor Author

Needed f20275f to finish addressing #577 for this the python feature.

@jdputschadi
Copy link
Contributor Author

Darn it I see new failures -- serves me right for only testing the previous failures. I'll get the issue resolved.

managed. If so, add "--break-system-packages" to the pip install
flags.

This does not really breack system packages due to the setting of
PYTHONUSERBASE during the install of pipx, but does get us past
checks for installing python packages into the system python install.
@samruddhikhandale
Copy link
Member

I don't see an option to run the tests, closing and opening this PR (attempt at fixing it)

@samruddhikhandale
Copy link
Member

I don't see an option to run the tests, closing and opening this PR (attempt at fixing it)

Didn't help 😓

image

Not sure if ^ needs to be fixed.

@jdputschadi
Copy link
Contributor Author

Samruddhi:

commit dad4f8d works for the base tests:

  • ubunto:focal
  • ubuntu:jammy
  • debian:11
  • debian:12
  • mcr.microsoft.com/devcontainers/base:debian
  • mcr.microsoft.com/devcontainers/base:ubunto

It should work on all the scenarios. I'm in the process of confirming that.

@jdputschadi
Copy link
Contributor Author

Looks like I need to merge in a recent commit.

That may take a while, especially if I re-run tests on my end before pushing the result of the mrege.

@jdputschadi
Copy link
Contributor Author

jdputschadi commented Feb 5, 2024

Commit 13a7f30 resolves test failures and passes all tests on my laptop. This should solve the known issues with the python feature outlined in #577.

@jdputschadi
Copy link
Contributor Author

jdputschadi commented Feb 5, 2024

@samruddhikhandale

I don't understand the failure. Commit f9f1e7f contains code, from commit 4007701 , that is required for Mariner and also prevents the Mariner test case from failing due to lack of a "tk-devel" RPM package in Mariner:

        # Mariner does not have tk-devel package available, RedHat ubi8 and ubi9 do not have tk-devel
        if [ ${ID} != "mariner" ] && [ ${ID} != "rhel" ]; then
            REQUIRED_PKGS="${REQUIRED_PKGS} \
                tk-devel"
        fi

However the test log shows the mariner test failing for that reason (from the 0_test-scenarios\ \(python\).txt file in the downloaded logs):

2024-02-01T21:35:10.6118401Z 17.57 Running transaction
2024-02-01T21:35:10.6119075Z 17.59 Installing/Updating: mpfr-4.1.0-2.cm2.x86_64
2024-02-01T21:35:10.6119870Z 17.61 Installing/Updating: gawk-5.1.1-1.cm2.x86_64
2024-02-01T21:35:10.6127810Z 17.76 Running yum check-update ...
2024-02-01T21:35:10.6128493Z 17.77 Loaded plugin: tdnfrepogpgcheck
2024-02-01T21:35:10.6129084Z 17.87 Package findutils is already installed.
2024-02-01T21:35:10.6129689Z 17.87 Package gnupg2 is already installed.
2024-02-01T21:35:10.6130403Z 17.87 tk-devel package not found or not installed
2024-02-01T21:35:10.6131028Z 17.87 Error(1011) : No matching packages
2024-02-01T21:35:10.6132684Z 17.88 ERROR: Feature "Python" (Unknown) failed to install! Look at the documentation at https://github.com/devcontainers/features/tree/main/src/python for help troubleshooting this error.

The most recent test run acts as if it was run based of a commit associated with this PR that was earlier than 4007701.

I don't know how to get the tests to rerun. I suppose I could make an in-consequential change (add white-space somewhere) and push that change, but that doesn't seem right.

I'll follow any advice/direction you provide on this one.

@samruddhikhandale
Copy link
Member

I don't know how to get the tests to rerun.

I triggered a rerun for failed action runs. A bit busy now, however, will take a peek at reviewing sometime later this afternoon.

@samruddhikhandale
Copy link
Member

The tests failed this time due to 👇 As this test adds quite a bit of new tests, we are out of disk space. I think we'd need to configure powerful ACTIONS, i'd look into it.

2024-02-05T22:55:40.5897864Z [2024-02-05T22:55:40.589Z] #14 126.8 ERROR: Could not install packages due to an OSError: [Errno 28] No space left on device

@samruddhikhandale
Copy link
Member

@jdputschadi Can you update https://github.com/devcontainers/features/blob/main/.github/workflows/test-pr.yaml#L7 to 👇. Let's see if it fixes it.

runs-on: devcontainer-image-builder-ubuntu

Copy link
Member

@samruddhikhandale samruddhikhandale left a comment

Choose a reason for hiding this comment

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

Super cool, left some thoughts. Thank you so much!!!

src/python/devcontainer-feature.json Show resolved Hide resolved
src/python/install.sh Show resolved Hide resolved
src/python/install.sh Show resolved Hide resolved
src/python/install.sh Outdated Show resolved Hide resolved
src/python/install.sh Outdated Show resolved Hide resolved
src/python/install.sh Outdated Show resolved Hide resolved
src/python/install.sh Outdated Show resolved Hide resolved
src/python/install.sh Outdated Show resolved Hide resolved
src/python/install.sh Outdated Show resolved Hide resolved
@jdputschadi
Copy link
Contributor Author

@jdputschadi Can you update https://github.com/devcontainers/features/blob/main/.github/workflows/test-pr.yaml#L7 to 👇. Let's see if it fixes it.

runs-on: devcontainer-image-builder-ubuntu

@samruddhikhandale : this line is in multiple places in the test-pr.yaml file. Do you want all of them changed?

@jdputschadi
Copy link
Contributor Author

@samruddhikhandale : Before I push a commit addressing the above notes and conversations, I have one overarching question:

Do you want the packages option from #768 included in the push (i.e. merge the change into one PR).

@samruddhikhandale
Copy link
Member

@samruddhikhandale : Before I push a commit addressing the above notes and conversations, I have one overarching question:

Do you want the packages option from #768 included in the push (i.e. merge the change into one PR).

I was mostly hinting at replacing toolsToInstall with packages, but I understand that it's not possible.

So feel free to include/exclude based on your convenience and confidence. This PR is already big, we can merge that into a separate one.

@jdputschadi
Copy link
Contributor Author

@samruddhikhandale : I think I've got the next push ready and it should address all your input. I'm running all tests locally to make sure they pass before executing the push.

The packages option will be left to the other PR.

@jdputschadi
Copy link
Contributor Author

Commit baf1568 addresses input from @samruddhikhandale and passes all tests locally.

Copy link
Member

@samruddhikhandale samruddhikhandale left a comment

Choose a reason for hiding this comment

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

We really appreciate all the efforts you take for the contribution! ✨

Left some minor comments.

.github/workflows/test-pr.yaml Show resolved Hide resolved
src/python/install.sh Outdated Show resolved Hide resolved
src/python/install.sh Outdated Show resolved Hide resolved
src/python/install.sh Outdated Show resolved Hide resolved
test/python/scenarios.json Outdated Show resolved Hide resolved
src/python/install.sh Outdated Show resolved Hide resolved
Copy link
Member

@samruddhikhandale samruddhikhandale left a comment

Choose a reason for hiding this comment

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

Looks wonderful, thank you so much! Appreciate it 👏

@samruddhikhandale
Copy link
Member

samruddhikhandale commented Feb 9, 2024

The packages option will be left to the other PR.

@jdputschadi (Not to pressure you) Were you planning on submitting a PR? because I'd like to soon merge this new option due to devcontainers/images#856.

If you are not planning, then let me know. Looking forward to avoid duplicate efforts, thanks!

@jdputschadi
Copy link
Contributor Author

The packages option will be left to the other PR.

@jeffwilcox (Not to pressure you) Were you planning on submitting a PR? because I'd like to soon merge this new option due to devcontainers/images#856.

If you are not planning, then let me know. Looking forward to avoid duplicate efforts, thanks!

Possibly the wrong Jeff? The "other PR" I was referring to was #768. I was indicating that I would not include the "packages" feature that Alexander was submitting in #768 and leaving that work to him.

@samruddhikhandale
Copy link
Member

Possibly the wrong Jeff?

Oops!

The "other PR" I was referring to was #768. I was indicating that I would not include the "packages" feature that Alexander was submitting in #768 and leaving that work to him.

Great, thanks for confirming!

Will merge this PR as soon as the tests are green.

@samruddhikhandale samruddhikhandale merged commit 30e03b6 into devcontainers:main Feb 9, 2024
11 checks passed
@jdputschadi jdputschadi deleted the python-feature-rhel-support branch February 10, 2024 23:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants