-
Notifications
You must be signed in to change notification settings - Fork 720
Add dynamic matrix generation for winpcap / npcap backends. #1950
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
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## dev #1950 +/- ##
=======================================
Coverage 83.52% 83.52%
=======================================
Files 310 310
Lines 54905 54905
Branches 12010 12000 -10
=======================================
Hits 45857 45857
+ Misses 7819 7798 -21
- Partials 1229 1250 +21
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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.
This is a great solution! However, it will add additional checks for PRs opened on the main repo vs. PRs opened in forks.
Ideally, I'd like the same number of checks, just use WinPcap instead of Npcap if not available
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.
Why the same number of checks?
If npcap isn't available, doing a CI run with it marked as winpcap is redundant as you are running the winpcap job twice in that case?
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.
The way it works now is that we have a combination of x64 / x86 and debug / release configurations and we attach some of them to WinPcap and others to Npcap. The assumption is that for most PRs it doesn't matter, but it still good to run with both options. I don't think we should have more jobs running on the main repo, but rather replace Npap jobs with WinPcap jobs on forked repo
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 don't actually build release configs in the build_and_test
CI as far as I know. Only debug.
It can be done so it uses Npcap backend if available and Winpcap if not. But that would have to be full replacement as it would be done in the matrix axis generation job, which would affect all Windows jobs.
I am worried as that would mean fully dropping Winpcap CI coverage on the main repo completely. My assumption is that most PRs from forked repos that want to contribute will target the main repo directly, so only main repo CI will run.
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 don't actually build release configs in the
build_and_test
CI as far as I know. Only debug.
You're right, we don't have release builds, we should probably have at least one...
It can be done so it uses Npcap backend if available and Winpcap if not. But that would have to be full replacement as it would be done in the matrix axis generation job, which would affect all Windows jobs.
I don't think we should always use Npcap if available or WinPcap if not. Ideally we should have a mix of both, but in forked repos, fallback to WinPcap. For example:
visual-studio:
strategy:
matrix:
include:
- os: windows-2025
platform: "Visual Studio 17 2022"
arch: Win32
pcap_lib: "winpcap"
- os: windows-2025
platform: "Visual Studio 17 2022"
arch: "x64"
pcap_lib: "npcap" # run on WinPcap in forked repos
- os: windows-2022
platform: "Visual Studio 17 2022"
arch: "x64"
pcap_lib: "npcap" # run on WinPcap in forked repos
In both main and forked repos we should have these 3 jobs, but in forked repos they should all use WinPcap
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 can be done so it uses Npcap backend if available and Winpcap if not. But that would have to be full replacement as it would be done in the matrix axis generation job, which would affect all Windows jobs.
I don't think we should always use Npcap if available or WinPcap if not. Ideally, we should have a mix of both, but in forked repos, fallback to WinPcap. For example:
visual-studio: strategy: matrix: include: - os: windows-2025 platform: "Visual Studio 17 2022" arch: Win32 pcap_lib: "winpcap" - os: windows-2025 platform: "Visual Studio 17 2022" arch: "x64" pcap_lib: "npcap" # run on WinPcap in forked repos - os: windows-2022 platform: "Visual Studio 17 2022" arch: "x64" pcap_lib: "npcap" # run on WinPcap in forked repos
In both main and forked repos we should have these 3 jobs, but in forked repos they should all use WinPcap
Well, that can't be done with the dynamic generation since that generates an axis. It can't generate individual jobs. See edit.
IMO, having the extra Npcap jobs is preferable than having a CI job that says that it is an npcap job, but surprise, it actually isn't. That is from both coverage and maintenance perspectives. Debugging a failed CI job that silently replaces a dependency with something else would be much harder than just not having the run job in the first place because the dependency is missing.
The way the jobs get named, people will see "pcap_lib=npcap" in the name and, without looking further in the code, will assume it uses npcap.
The jobs already cancel when a new push is done on a PR, so even if the CI is somewhat longer, it will abort if it is unnecessary.
Edit:
Upon further thought, I suppose you can set the pcap_lib
to read from a variable for the flip flop jobs, but I kinda dislike that because of variable coverage. IMO, the main repo should not have less CI coverage than the forked repos. :/
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.
@seladb Managed to get it to work. Can you check the VS job?
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.
Maybe we can have something like this?
visual-studio:
strategy:
matrix:
include:
- os: windows-2025
platform: "Visual Studio 17 2022"
arch: Win32
pcap_lib: "winpcap"
run_on: all-repos
- os: windows-2025
platform: "Visual Studio 17 2022"
arch: "x64"
pcap_lib: "npcap"
run_on: main-repo
- os: windows-2025
platform: "Visual Studio 17 2022"
arch: "x64"
pcap_lib: "winpcap"
run_on: forked-repos
- os: windows-2022
platform: "Visual Studio 17 2022"
arch: "x64"
pcap_lib: "npcap"
run_on: main-repo
- os: windows-2022
platform: "Visual Studio 17 2022"
arch: "x64"
pcap_lib: "winpcap"
run_on: forked-repos
It's not ideal to have so many jobs, but maybe it's the most explicit option, and also not too hard to implement
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.
The most recent commit already does that.
The variable npcap-or-default resolves to 'npcap' if OEM credentials are available and 'winpcap' if not.
IMO, that is enough. Forked repos should be able to run the CI with Npcap with no changes, provided they supply their own OEM secret.
… Npcap is not available.
- Use Npcap backend for x64 jobs if available.
…as a json object.
.github/workflows/build_and_test.yml
Outdated
pcap_lib: ${{ fromJSON(needs.define-windows-pcap-backend-matrix.outputs.pcap-backends) }} | ||
sys: [mingw32, mingw64] | ||
include: # Extends the matrix to add the env variable for each matching sys | ||
- sys: mingw32 | ||
env: i686 | ||
- sys: mingw64 | ||
env: x86_64 |
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.
Maybe we should define something similar to what we have in VS?
matrix:
include:
- sys: mingw32
env: i686
pcap_lib: "winpcap"
- sys: mingw64
env: x86_64
pcap_lib: ${{ needs.define-windows-pcap-backend-matrix.outputs.npcap-or-default }}
If we do that the pcap-backends
output is not needed
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.
Thank you for working on this @Dimi1010 ! 🙏 |
This PR adds a new step to CI pipeline that dynamically generates the windows pcap backends matrix axis that are tested against.
NPCAP_USERNAME
andNPCAP_PASSWORD
. These secrets are required for the silent installer to install the backend.