Skip to content

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.

Sign up for GitHub

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

Merged
merged 27 commits into from
Sep 12, 2025
Merged

Add dynamic matrix generation for winpcap / npcap backends. #1950

Changes from 24 commits
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
46f00b5
Add debug jobs for selecting winpcap or npcap matrix.
Dimi1010 Sep 7, 2025
6208d06
Update step so it does not use secrets directly in bash.
Dimi1010 Sep 7, 2025
46fae38
Fix syntax.
Dimi1010 Sep 7, 2025
e69287d
Fix condition.
Dimi1010 Sep 7, 2025
226081c
Attempt to fix again.
Dimi1010 Sep 7, 2025
d2dbca5
Fix quotes.
Dimi1010 Sep 7, 2025
3a84024
Fix expression again...
Dimi1010 Sep 7, 2025
e513745
temp remove the secret check
Dimi1010 Sep 7, 2025
59ca967
Fix case?
Dimi1010 Sep 7, 2025
fe11a71
Fix step outputs.
Dimi1010 Sep 7, 2025
a7c2574
Test dynamic matrix on mingw job.
Dimi1010 Sep 7, 2025
8120d04
Expand matrices on windows and mingw.
Dimi1010 Sep 7, 2025
404c334
Rename matrix generation job.
Dimi1010 Sep 7, 2025
bf056a5
Add step to enable Npcap backend if OEM credentials are available.
Dimi1010 Sep 7, 2025
6526248
Fix conditional.
Dimi1010 Sep 7, 2025
4455c5a
Updated step desc.
Dimi1010 Sep 7, 2025
a725c85
Improve comments.
Dimi1010 Sep 7, 2025
922f531
Lint
Dimi1010 Sep 7, 2025
99551bd
Fix case.
Dimi1010 Sep 7, 2025
993ee6c
Add step explaining why NPcap is unavailable.
Dimi1010 Sep 7, 2025
666f198
Fix quotes.
Dimi1010 Sep 7, 2025
6144ddc
Add another output that prefers Npcap backend but will use Winpcap if…
Dimi1010 Sep 9, 2025
0de92ff
Rollback windows jobs to a partial matrix.
Dimi1010 Sep 9, 2025
01f76c1
Removed quotation marks from npcap-or-default as it will not be read …
Dimi1010 Sep 9, 2025
28a25f4
Rollback mingw jobs to single jobs.
Dimi1010 Sep 11, 2025
16aa358
Remove pcap-backends output.
Dimi1010 Sep 11, 2025
f94090d
Merge branch 'dev' into ci/dynamic-pcap-backend
Dimi1010 Sep 11, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
55 changes: 44 additions & 11 deletions .github/workflows/build_and_test.yml
View file
Open in desktop
Copy link
Owner

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

Copy link
Collaborator Author

@Dimi1010 Dimi1010 Sep 8, 2025 •
edited
Loading

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?

Copy link
Owner

@seladb seladb Sep 8, 2025 •
edited
Loading

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

Copy link
Collaborator Author

@Dimi1010 Dimi1010 Sep 8, 2025 •
edited
Loading

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.

Copy link
Owner

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

Copy link
Collaborator Author

@Dimi1010 Dimi1010 Sep 8, 2025 •
edited
Loading

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. :/

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 Managed to get it to work. Can you check the VS job?

Copy link
Owner

@seladb seladb Sep 11, 2025 •
edited
Loading

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

Copy link
Collaborator Author

@Dimi1010 Dimi1010 Sep 11, 2025 •
edited
Loading

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.

Original file line number Diff line number Diff line change
Expand Up @@ -504,18 +504,50 @@ jobs:
path: ${{ env.CCACHE_DIR }}
key: ${{ steps.ccache-restore.outputs.cache-primary-key }}

define-windows-pcap-backend-matrix:
name: Determine Windows Pcap Backend Matrix
runs-on: ubuntu-latest
outputs:
pcap-backends: ${{ steps.generate-backends.outputs.pcap_lib }}
npcap-or-default: ${{ steps.generate-backends.outputs.npcap_or_default}}
steps:
- name: Initialize NPcap backend variable
run: echo "npcap_available=false" >> $GITHUB_ENV

- name: Enable NPcap backend if OEM credentials are available
env: # Secrets can't be accessed directly in 'if' conditionals.
npcap_username: ${{ secrets.NPCAP_USERNAME }}
npcap_password: ${{ secrets.NPCAP_PASSWORD }}
if: ${{ env.npcap_username != '' && env.npcap_password != '' }}
run: echo "npcap_available=true" >> $GITHUB_ENV

- name: NPcap backend is unavailable
if: ${{ env.npcap_available != 'true' }}
run: echo "NPcap backend will be skipped since OEM credentials are missing. NPCAP_USERNAME and NPCAP_PASSWORD are required."

- name: Generate pcap backends matrix
id: generate-backends
run: |
if [[ "${{ env.npcap_available }}" == "true" ]]; then
echo 'pcap_lib=["winpcap", "npcap"]' >> "$GITHUB_OUTPUT"
echo 'npcap_or_default=npcap' >> "$GITHUB_OUTPUT"
else
echo 'pcap_lib=["winpcap"]' >> $GITHUB_OUTPUT
echo 'npcap_or_default=winpcap' >> "$GITHUB_OUTPUT"
fi

mingw-w64:
runs-on: windows-latest
needs: define-windows-pcap-backend-matrix
strategy:
matrix:
include:
- env: i686
sys: mingw32
pcap_lib: "winpcap"
- env: x86_64
sys: mingw64
pcap_lib: "winpcap"

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
Copy link
Owner

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Updated 28a25f4 & 16aa358

The name of the job is a bit inaccurate now, but it can probably do.

runs-on: windows-latest
steps:
- name: Checkout code
uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2
Expand Down Expand Up @@ -596,6 +628,7 @@ jobs:
CODECOV_TOKEN: ${{ secrets.CODECOV_TOKEN }}

visual-studio:
needs: define-windows-pcap-backend-matrix
strategy:
matrix:
include:
Expand All @@ -606,11 +639,11 @@ jobs:
- os: windows-2025
platform: "Visual Studio 17 2022"
arch: "x64"
pcap_lib: "winpcap"
pcap_lib: ${{ needs.define-windows-pcap-backend-matrix.outputs.npcap-or-default }}
- os: windows-2022
platform: "Visual Studio 17 2022"
arch: "x64"
pcap_lib: "winpcap"
pcap_lib: ${{ needs.define-windows-pcap-backend-matrix.outputs.npcap-or-default }}

runs-on: ${{ matrix.os }}
steps:
Expand Down