-
Notifications
You must be signed in to change notification settings - Fork 378
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
Add RHEL support to the python feature #830
Conversation
I see the cause of the error in the "checks" and will update the submitted code to not fail. |
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. ✨ |
… on Mariner systems
… assuming "python" will work
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.
I don't see an option to run the tests, closing and opening this PR (attempt at fixing it) |
Samruddhi: commit dad4f8d works for the base tests:
It should work on all the scenarios. I'm in the process of confirming that. |
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. |
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:
However the test log shows the mariner test failing for that reason (from the
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. |
I triggered a rerun for failed action runs. A bit busy now, however, will take a peek at reviewing sometime later this afternoon. |
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.
|
@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.
|
There was a problem hiding this 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!!!
@samruddhikhandale : this line is in multiple places in the |
@samruddhikhandale : Before I push a commit addressing the above notes and conversations, I have one overarching question: Do you want the |
I was mostly hinting at replacing 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. |
@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 |
Commit baf1568 addresses input from @samruddhikhandale and passes all tests locally. |
There was a problem hiding this 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.
There was a problem hiding this 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 👏
@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! |
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. |
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 theapt
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