-
Notifications
You must be signed in to change notification settings - Fork 639
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
Bump Ubuntu to 24.04 and Fedora to 39 #1408
base: dev
Are you sure you want to change the base?
Conversation
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.
Seems some CIs need to be fixed.
For Ubuntu 24, it's about PEP668: https://peps.python.org/pep-0668/#marking-an-interpreter-as-using-an-external-package-manager.
We can simply apply --break-system-packages
for the pip.
"Package and release / mingw-w64" is a known issue: #1347, we can ignore it.
@@ -98,15 +98,15 @@ jobs: | |||
|
|||
- name: Test PcapPlusPlus | |||
run: | | |||
${{ matrix.python }} -m pip install -U pip | |||
${{ matrix.python }} -m pip install -r ci/run_tests/requirements.txt | |||
${{ matrix.python }} -m pip install --break-system-packages -U pip |
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.
What about using a new environment variable like additional-flags
to set empty or --break-system-packages
since Fedora's python failed to find this flag. Changing these lines should fix maybe, what do you think?
${{ matrix.python }} -m pip install ${{ matrix.additional-python-flags }} -U pip
${{ matrix.python }} -m pip install ${{ matrix.additional-python-flags }} -r ci/run_tests/requirements.txt
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.
can't we just have an if-else statement?
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.
It is another option but adding an if else statement for step could lead more complex and longer workflow file, no? And maybe a bit duplication since the only difference is a flag
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.
Both work, so I am fine with either one.
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.
I have also bump Fedora so no more issue
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.
That's the reason we didn't use venv
until now 🙂
But if there are conflicts with the system Python packages, using venv
should solve it.
Or we can use the --break-system-packages
flag in all envs, I wouldn't use it selectively on specific envs
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.
I think using --break-system-packages
is fine enough unless someone want to put effort to change to venv, which can do it in another PR.
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.
I just noticed pip
on RHEL doesn't have the --break-system-packages
flag, probably because the pip
version is too old. So I guess we can either add this flag selectively or move to venv
...
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.
@clementperon do you think switching to RHEL licensing manager should solve 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.
I have bump Pip in the same MR
bd4f74b
to
ad00b7e
Compare
@clementperon now that we updated RHEL, maybe we can fix this PR and merge it? |
Ubuntu 18.04 is EOL since April 2023.
Also it doesn't support recent NodeJS this imply we can't properly bump Github CI Actions.
Let's drop it and add support of Ubuntu 24.04 instead