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

support ubuntu 24 in CI #1486

Draft
wants to merge 22 commits into
base: dev
Choose a base branch
from
Draft

support ubuntu 24 in CI #1486

wants to merge 22 commits into from

Conversation

tigercosmos
Copy link
Collaborator

@tigercosmos tigercosmos commented Jul 6, 2024

DON'T MERGE

Blocking by #1489 now

  • use venv for pip installation
  • add Ubuntu 24.04 in the CI

@seladb
Copy link
Owner

seladb commented Jul 6, 2024

@tigercosmos we need to install venv on all Linux containers to make it work. In Ubuntu the package is python3-venv I think, we need to figure out what the package name in the other distros

@tigercosmos
Copy link
Collaborator Author

@tigercosmos we need to install venv on all Linux containers to make it work. In Ubuntu the package is python3-venv I think, we need to figure out what the package name in the other distros

I see, let me figure out.

@tigercosmos tigercosmos added the CI label Jul 6, 2024
@tigercosmos tigercosmos added this to the Augest 2024 Release milestone Jul 7, 2024
@tigercosmos
Copy link
Collaborator Author

blocking by #1384 due to:

  DEPRECATION: netifaces is being installed using the legacy 'setup.py install' method, because it does not have a 'pyproject.toml' and the 'wheel' package is not installed. pip 23.1 will enforce this behaviour change. A possible replacement is to enable the '--use-pep517' option. Discussion can be found at https://github.com/pypa/pip/issues/8559

else
apt-get update
apt-get install -y python3 python3-venv python3-dev
fi
Copy link
Collaborator

Choose a reason for hiding this comment

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

this should be done in the container

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ya, we can do that as well, @seladb can you update the containers?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I can do it if you want, but you just need to open a MR here:
https://github.com/seladb/PcapPlusPlus-DockerImages

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

cool, I didn't know this.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@seladb shouldn't /PcapPlusPlus-DockerImages under the organization?

@@ -31,6 +31,7 @@ jobs:
apk update && apk add cppcheck python3-dev
python3 -m pip install cmake-format clang-format==18.1.6

# TODO: investigate how to run pre-commit with `venv`
- uses: pre-commit/action@2c7b3805fd2a0fd8c1884dcaebf91fc102a13ecd # v3.0.1
Copy link
Collaborator

Choose a reason for hiding this comment

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

Did you try

Suggested change
- uses: pre-commit/action@2c7b3805fd2a0fd8c1884dcaebf91fc102a13ecd # v3.0.1
- name: Activate virtualenv
run: |
${{ matrix.python }} -m venv ./venv
. .venv/bin/activate
echo PATH=$PATH >> $GITHUB_ENV
- uses: pre-commit/action@2c7b3805fd2a0fd8c1884dcaebf91fc102a13ecd # v3.0.1

@@ -52,6 +53,9 @@ jobs:
strategy:
matrix:
include:
- image: ubuntu2404
python: python3
Copy link
Collaborator

Choose a reason for hiding this comment

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

now that everybody use python: python3 maybe we could drop it no ?

${{ matrix.python }} -m pip install -U pip
${{ matrix.python }} -m venv ./venv
source ./venv/bin/activate
${{ matrix.python }} -m pip install pip
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
${{ matrix.python }} -m pip install pip
${{ matrix.python }} -m pip install -U pip

Else I think this command do nothing no ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

not sure, maybe yes, because we use venv now.

Copy link
Collaborator

Choose a reason for hiding this comment

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

as you're executing
${{ matrix.python }} -m pip -> I supposed it's already installed no?

If you really want to install it
${{ matrix.python }} -m ensurepip

${{ matrix.python }} -m pip install -r ci/run_tests/requirements.txt
${{ matrix.python }} ci/run_tests/run_tests.py --interface eth0 ${{ matrix.test-flags }}

- name: Test Examples
shell: bash
Copy link
Collaborator

@clementperon clementperon Jul 8, 2024

Choose a reason for hiding this comment

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

Why is it needed ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

the default shell is sh, and to source the venv activator, we need bash.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants