-
Notifications
You must be signed in to change notification settings - Fork 567
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
ci: whitelist paths, reorganize workflows & speed-up tests #5960
Conversation
This pull request sets up GitHub code scanning for this repository. Once the scans have completed and the checks have passed, the analysis results for this pull request branch will appear on this overview. Once you merge this pull request, the 'Security' tab will show more code scanning analysis results (for example, for the default branch). Depending on your configuration and choice of analysis tool, future pull requests will be annotated with code scanning analysis results. For more information about GitHub code scanning, check out the documentation. |
@github-code-scanning on Aug 16:
This was likely posted because this PR renames codeql-analysis.yml. That was mostly done to avoid running the CodeQL Python check when it is not But it seems that the file has to be named exactly codeql-analysis.yml, or else From security/code-scanning/tools/CodeQL/status:
I think I'll undo the renaming. Feel free to review this in any case. |
497a47a
to
12d65f3
Compare
Not that I understand much of the code in this PR, I do appreciate all the work being done to make CI-life more fun & less hasle! |
- config.mk.in | ||
- config.sh.in | ||
- configure | ||
- configure.ac |
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 ci, m4 and contrib folders? And *.sh
? Maybe tests folder?
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 ci, m4 and contrib folders? And
*.sh
? Maybe tests folder?
Good catch, fixed the missing m4/ and added comments to clarify what each
workflow is supposed to do.
ci/ is currently only used for the profile checks, so it's only in
check-profiles.yml.
contrib/, test/ and some shell scripts are used in make dist
, so they are in
build.yml (which in this PR checks both make dist
and the normal build).
test/ is otherwise only used when running tests, so it's only in test.yml.
The shell scripts in the root directory do not seem to be used for anything
during the normal build, so they wouldn't affect the checks.
Is there any script in particular that you think would affect 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.
ci/ is currently only used for the profile checks, so it's only in
check-profiles.yml.
./ci/printenv.sh
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.
ci/ is currently only used for the profile checks, so it's only in
check-profiles.yml.
./ci/printenv.sh
That makes sense, though since this is only used for diagnostics in CI, how
about adding it only to build.yml to avoid re-running everything on every
change?
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.
On second thought, this file is unlikely to be changed often anyway, so I added
it to every workflow.
I think it's OK to rename, but then the old configuration must be deleted from Security tab. I did a similar rename recently. |
- run: make lab-setup | ||
- run: make test-utils | ||
|
||
test-network: |
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 personally don't like that most of these sections is now duplicated multiple times. When adjusting something you now need to remember to check and update multiple places.
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 personally don't like that most of these sections is now duplicated
multiple times. When adjusting something you now need to remember to check
and update multiple places.
I don't like it either, but unfortunately GitHub doesn't support YAML anchors:
It looks like a core part of the language to me, but this problem was first
reported in 2019 and the above issue has been labeled as "enhancement" and
"papercut" rather than as a bug in their parser, so I wouldn't expect it to be
fixed any time soon.
Do you know of any other simple way to deduplicate them?
.github/workflows/check-c.yml
Outdated
permissions: | ||
actions: read # for github/codeql-action/init to get workflow details | ||
contents: read # for actions/checkout to fetch code | ||
security-events: write # for github/codeql-action/autobuild to send a status report |
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.
autobuild no longer used
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.
autobuild no longer used
While the comment mentions the autobuild step specifically, it looks like that
permission still needed by the job to create/modify alerts in the Security tab:
From https://docs.github.com/en/actions/using-jobs/assigning-permissions-to-jobs
security-events
Work with GitHub code scanning and Dependabot alerts. For example,
security-events: read
permits an action to list the Dependabot alerts for
the repository, andsecurity-events: write
allows an action to update the
status of a code scanning alert. For more information, see "Repository
permissions for 'Code scanning alerts'" and "Repository permissions for
'Dependabot alerts'" in "Permissions required for GitHub Apps."
Also, when creating codeql-analysis.yml on a new project, it generates the
permissions without any comments:
jobs:
analyze:
name: Analyze
# Runner size impacts CodeQL analysis time. To learn more, please see:
# - https://gh.io/recommended-hardware-resources-for-running-codeql
# - https://gh.io/supported-runners-and-hardware-resources
# - https://gh.io/using-larger-runners
# Consider using larger runners for possible analysis time improvements.
runs-on: ${{ (matrix.language == 'swift' && 'macos-latest') || 'ubuntu-latest' }}
timeout-minutes: ${{ (matrix.language == 'swift' && 120) || 360 }}
permissions:
actions: read
contents: read
security-events: write
I have removed the comments to match the current generated file.
I could also remove the write permission if you're sure that it's not needed.
.github/workflows/check-python.yml
Outdated
permissions: | ||
actions: read # for github/codeql-action/init to get workflow details | ||
contents: read # for actions/checkout to fetch code | ||
security-events: write # for github/codeql-action/autobuild to send a status report |
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.
autobuild no longer used
I kept the real checks on separate files and left codeql-analysis.yml as a From the testing on my GitHub fork, there do not appear to be any duplicated I also don't see any option to delete anything in the following page:
Could you clarify how you did the renaming? Are you sure that deleting the |
That is, replace `paths-ignore` with `paths`. This should reduce the number of unnecessary workflow executions and the frequency at which paths are changed. It also reduces the overall number of paths used. Also, add the missing ci/printenv.sh to the path whitelists.
I think that was the place, but the option will only show up after the file is deleted.
I just deleted the file (or rather, merged codeql.yml and codeql-analysis.yml): Some docs here. |
Note: When generating a new workflow, the permissions do not have comments anymore.
Only run the CodeQL Python analysis if a .py file is changed.
All of the current workflows are used for CI.
Move scan-build, cppcheck and CodeQL (cpp). This is similar to build-extra.yml, but for jobs that check for issues in the code rather than checking for build failures. Note: As this deletes codeql-analysis.yml, its configuration also has to be deleted in the GitHub web UI to prevent it from warning about the file being missing: * Security -> Code scanning -> Tool status -> (Setup Types) CodeQL -> (Configurations) language:python -> Delete configuration Misc: The above was clarified by @topimiettinen[1]. [1] netblue30#5960 (comment)
Thanks, I see what you mean now. I removed codeql-analysis.yml in the PR again and I managed to delete the
It was really confusing because the CodeQL item had "language:python" in the Maybe it makes sense when using many scanning tools, but these steps seemed |
Testing takes significantly longer than building, so this makes the default build check faster.
Do so when the output of the given job is not important. For example, when the output of another job can be used for debugging build-related issues.
Move scan-build, cppcheck and CodeQL (cpp). This is similar to build-extra.yml, but for jobs that check for issues in the code rather than checking for build failures. Note: As this deletes codeql-analysis.yml, its configuration also has to be deleted in the GitHub web UI to prevent it from warning about the file being missing: * Security -> Code scanning -> Tool status -> (Setup Types) CodeQL -> (Configurations) language:python -> Delete configuration Misc: The above was clarified by @topimiettinen[1]. [1] netblue30#5960 (comment)
Considering the most recent runs, this reduces the total amount of time it takes to run the tests from about 9-10 minutes to about 3 minutes. Note: Which jobs are split is mostly determined by how long each test takes. For example, this is the time each test step took in a run of `build_and_test` (10m17s total for the job) on commit bfcf8bc ("Merge pull request netblue30#5956 from kmk3/build-fix-dep-syntax", 2023-08-14)[1]: * 17s test-seccomp-extra * 1s test-firecfg * 16s test-capabilities * 6s test-apparmor * 10s test-appimage * 10s test-chroot * 41s test-sysutils * 24s test-private-etc * 40s test-profiles * 4s test-fcopy * 2s test-fnetfilter * 98s test-fs * 103s test-utils * 57s test-environment * 69s test-network [1]: https://github.com/netblue30/firejail/actions/runs/5860927500/job/15890009169
So it's been almost a week and from what I can tell all of the issues raised Is there anything else? If not I'll probably merge this later. Note that the tests as they are already fail randomly and if a single test While with this PR only the specific test job that fails needs to be |
Merged, thanks everyone for the feedback!
Done in this repository as well. |
Currently the number of make jobs used for the default build target are hard-coded and the value used varies across files. For consistency (and potentially better performance), use `make -j "$(nproc)"` everywhere that `make -j` is currently used. Kind of relates to commit 500d8f2 ("ci: run make in parallel where applicable", 2023-08-14) / PR netblue30#5960.
Currently the number of make jobs used for the default build target are hardcoded and the value used varies across files. For consistency (and potentially better performance), use `make -j "$(nproc)"` everywhere that `make -j` is currently used. Kind of relates to commit 500d8f2 ("ci: run make in parallel where applicable", 2023-08-14) / PR netblue30#5960.
Currently the number of make jobs used for the default build target are hardcoded and the value used varies across files. For consistency (and potentially better performance), use `make -j "$(nproc)"` everywhere that `make -j` is currently used. Kind of relates to commit 500d8f2 ("ci: run make in parallel where applicable", 2023-08-14) / PR netblue30#5960.
Currently the number of make jobs used for the default build target are hardcoded and the value used varies across files. For consistency (and potentially better performance), use `make -j "$(nproc)"` everywhere that `make -j` is currently used. Kind of relates to commit 500d8f2 ("ci: run make in parallel where applicable", 2023-08-14) / PR netblue30#5960.
Main changes:
Considering the most recent runs, this reduces the total amount of time
it takes to run the tests from about 9-10 minutes to about 3 minutes.