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

CI(CodeQL): Improve run times and add concurrency groups #3361

Merged
merged 4 commits into from
Jan 15, 2024

Conversation

echoix
Copy link
Member

@echoix echoix commented Jan 14, 2024

During the last week or so, I explored many options and configuration possibilities to refresh our CodeQL jobs, starting from a new fresh advanced config and porting the new suggested practices one by one. This is the result of the most appropriate possibility.

  1. First, the goal was to add a concurrency group for PRs, since even when exploring (for other things) in my fork, sometimes I was waiting on runners because a new push to a PR would not cancel CodeQL analysis like our other workflows.
  2. Then, to improve build times, I added the number of jobs to the makeflags like our Ubuntu workflows.
  3. CodeQL scanning for python doesn't need to have grass built to work. This wasn't the case before when CodeQL was introduced, lots have changed. It seems it is a static analysis, but still needs to see the code for the python dependencies. So, I didn't build grass for python language scans. Edit: I completly removed Python dependencies after the extra reference I found in 4, and retesting it out.
  4. There's a weird background-compatibilty workaround CodeQL uses to try to autoinstall python dependencies if you ever used CodeQL with this enabled when it was the default. Now they prefer to not add them, but they go extra steps so you don't notice that default change (for new users) https://github.blog/changelog/2023-07-12-code-scanning-with-codeql-no-longer-installs-python-dependencies-automatically-for-new-users/. I thus removed any pip dependency installation, and it doesn't impact the C build either.
  5. CodeQL prefers now to use c-cpp, others being an alias.
  6. We could optionally remove to paths/paths ignore from .github/codeql/codeql-config.yml, but I didn't include them in this PR. It doesn't really change, C/C++ doesn't support it anyways, and without build

I also explored the possibility of running a single job that scans both languages, but after figuring out that building grass or not didn't seem to impact python scanning and after more exploration, then computing the run times vs runner usage didn't make it advantageous, especially with the MAKEFLAGS set.

@github-actions github-actions bot added the CI Continuous integration label Jan 14, 2024
@github-advanced-security
Copy link

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.

@neteler neteler added this to the 8.4.0 milestone Jan 14, 2024
@echoix
Copy link
Member Author

echoix commented Jan 14, 2024

The syntax warnings that you might observe in the python logs, at the extraction step (parsing the code to a database that can be analysed by the CodeQL queries), like https://github.com/OSGeo/grass/actions/runs/7520627203/job/20470529478?pr=3361#step:10:126 or https://github.com/OSGeo/grass/actions/runs/7520627203/job/20470529478?pr=3361#step:10:228
should be already addressed in #3331, that yesterday I cleaned up the black error remaining from last week, once it gets merged.

@echoix echoix merged commit 4c6be3e into OSGeo:main Jan 15, 2024
24 checks passed
@echoix echoix deleted the codeql-config branch January 15, 2024 12:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI Continuous integration
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants