Try testing with Scapy version updated to 2.5.0 #212
Conversation
Signed-off-by: Andy Fingerhut <andy_fingerhut@alum.wustl.edu>
Signed-off-by: Andy Fingerhut <andy_fingerhut@alum.wustl.edu>
... where the version of pip does not like you to run as root and install in system-wide directories. Signed-off-by: Andy Fingerhut <andy_fingerhut@alum.wustl.edu>
Signed-off-by: Andy Fingerhut <andy_fingerhut@alum.wustl.edu>
Signed-off-by: Andy Fingerhut <andy_fingerhut@alum.wustl.edu>
Signed-off-by: Andy Fingerhut <andy_fingerhut@alum.wustl.edu>
Signed-off-by: Andy Fingerhut <andy_fingerhut@alum.wustl.edu>
Signed-off-by: Andy Fingerhut <andy_fingerhut@alum.wustl.edu>
Signed-off-by: Andy Fingerhut <andy_fingerhut@alum.wustl.edu>
Signed-off-by: Andy Fingerhut <andy_fingerhut@alum.wustl.edu>
Signed-off-by: Andy Fingerhut <andy_fingerhut@alum.wustl.edu>
Signed-off-by: Andy Fingerhut <andy_fingerhut@alum.wustl.edu>
Signed-off-by: Andy Fingerhut <andy_fingerhut@alum.wustl.edu>
Signed-off-by: Andy Fingerhut <andy_fingerhut@alum.wustl.edu>
Signed-off-by: Andy Fingerhut <andy_fingerhut@alum.wustl.edu>
Signed-off-by: Andy Fingerhut <andy_fingerhut@alum.wustl.edu>
Signed-off-by: Andy Fingerhut <andy_fingerhut@alum.wustl.edu>
Signed-off-by: Andy Fingerhut <andy_fingerhut@alum.wustl.edu>
Signed-off-by: Andy Fingerhut <andy_fingerhut@alum.wustl.edu>
Signed-off-by: Andy Fingerhut <andy_fingerhut@alum.wustl.edu>
Signed-off-by: Andy Fingerhut <andy_fingerhut@alum.wustl.edu>
Signed-off-by: Andy Fingerhut <andy_fingerhut@alum.wustl.edu>
|
Most of these changes are for installing packages using the newer version of pip that is the default one available via Ubuntu 24.04 apt package installation. That newer version of pip gives an error if you attempt to install Python packages in system-wide directories as root. With these changes, CI tests now install Python packages under the user's |
|
There is a CI test that I see in the Github web page for this PR that is simply called |
| python3 CI/check-nnpy.py | ||
| ./CI/run_tests.sh | ||
| sudo python3 /usr/local/bin/nose2 utests.tests.test |
There was a problem hiding this comment.
FYI for reviewers - these tests are still run, but I moved the command that runs them into the ./CI/run_tests.sh script, for easier control of additional command line options.
This happens when the repository requires a particular workflow to pass but this workflow has been renamed or removed. Not a big problem, we can just bypass the failure and then update the required checks in the repository configuration. |
Makes sense. By adding a matrix of 2 OS versions and 2 Scapy versions for that required test, that probably changed the names. |
.github/workflows/build.yml
Outdated
| @@ -11,41 +11,53 @@ on: | |||
|
|
|||
| jobs: | |||
| verify-python: | |||
| strategy: | |||
| matrix: | |||
| os: [ubuntu-22.04, ubuntu-latest] | |||
There was a problem hiding this comment.
Maybe use ubuntu-24.04 here instead to avoid failures once this version changes?
There was a problem hiding this comment.
At least one argument used for this in p4lang/p4runtime repo is: when ubuntu-latest changes in 2 years, if it fails, we'll get reminded and update this.
If that argument doesn't work for you, I am happy to change it, as when 26.04 is released, someone might remember to do a sweep of repos to update these kinds of things.
There was a problem hiding this comment.
changes ubuntu-latest to ubuntu-24.04 in commit 23
There was a problem hiding this comment.
Yeah fixing the version avoids surprises or gaps (for example, when they update the runner to ubuntu-26.04 and now we are missing 24.04 without even knowing).
Signed-off-by: Andy Fingerhut <andy_fingerhut@alum.wustl.edu>
CI/run_tests.sh
Outdated
| MAJOR_MINOR_VERSION=$(python3 -c 'import sys; print(f"{sys.version_info.major}.{sys.version_info.minor}")') | ||
| PPATH="$HOME/.local/lib/python${MAJOR_MINOR_VERSION}/site-packages" | ||
|
|
||
| sudo PATH=${PATH} PYTHONPATH=${PPATH} python3 ptf_nn/ptf_nn_agent.py \ |
There was a problem hiding this comment.
Why are these path hacks necessary btw?
Maybe we should consider setting up virtualenv or poetry if Ubuntu24.04 requires it.
There was a problem hiding this comment.
Ah, I guess because of the use of sudo.
Although sudo -E env PATH="$PATH" should be enough here. If the Python environment is set up correctly.
There was a problem hiding this comment.
If you set up virtualenv as the normal user, then you pick a directory where that is installed, and using it typically either requires, or recommends, first running source $MYVENV/bin/activate, which might do other things, but perhaps the most crucial is setting the environment variable VIRTUAL_ENV.
If you know a way to run commands as root that don't do one of (a) pass the environment variable values on to the root user, as shown in my current sudo commands, or (b) somehow do source $MYVENV/bin/activate ; <other command I really want to run as root>, I'm interested to learn what that is.
I don't know of any way in which virtual environments somehow avoid this.
There was a problem hiding this comment.
And if I recall correctly, doing what I am doing now, where the root user uses a normal user's lib/python* directory, isn't merely read-only, but the root user can also write .pyc files and __pycache__ directories that the normal user then does not have permission to modify later.
There was a problem hiding this comment.
I don't think there is a way around this unless you disable sudo's path protection: https://unix.stackexchange.com/a/151188
I have found sudo -E env PATH="$PATH" the least invasive approach
There was a problem hiding this comment.
commit 25 uses sudo -E env PATH="$PATH" ... with success. Thanks for the suggestion.
There was a problem hiding this comment.
https://unix.stackexchange.com/a/480764
Might even remove the need to use any dollar signs. But I have not tried that command.
There was a problem hiding this comment.
Trying that variation with commit 26 to see if it passes the tests.
At some point, we have to stop tweaking and use something that works :-)
There was a problem hiding this comment.
It failed. Reverted that change in commit 27, back to what it was in commit 25.
There was a problem hiding this comment.
I was mostly interested because of p4lang/p4c#5047
Some of the lessons here I would like to apply to that PR.
Signed-off-by: Andy Fingerhut <andy_fingerhut@alum.wustl.edu>
Signed-off-by: Andy Fingerhut <andy_fingerhut@alum.wustl.edu>
Signed-off-by: Andy Fingerhut <andy_fingerhut@alum.wustl.edu>
CI/run_tests.sh
Outdated
|
|
||
| sleep 5 | ||
|
|
||
| sudo ptf --test-dir ptf_nn/ptf_nn_test \ | ||
| env |
There was a problem hiding this comment.
Removed in commit 28. I don't know how I got that in there.
| @@ -3,6 +3,9 @@ | |||
| THIS_DIR=$( cd "$( dirname "${BASH_SOURCE[0]}" )" && pwd ) | |||
| source $THIS_DIR/common.sh | |||
|
|
|||
| set -x | |||
| cmake --version | |||
There was a problem hiding this comment.
What's the motivation for this output?
There was a problem hiding this comment.
debugging CI script behavior.
|
The "squash and merge" button is disabled for me, because of CI test named Until we change the branch protection requirements, this will be needed for every merge going forward. |
| scapy_version: [2.5.0, 2.6.1] | ||
| # Don't abort other runs when one of them fails, to ease debugging. | ||
| fail-fast: false | ||
|
|
||
| name: Python code verification (src) |
There was a problem hiding this comment.
I would call this linting or static analysis. Minor comment.
I updated the CI to require these tests now.
No description provided.