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.

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

ci: misc improvements #6575

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from
Open

ci: misc improvements #6575

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
48ae36e
ci: only clone what is needed
PastaPastaPasta Feb 13, 2025
01cbd02
ci: run unit tests in dedicated step
PastaPastaPasta Feb 12, 2025
525baac
ci: dedicated step to see ccache stats and cleanup and compress
PastaPastaPasta Feb 12, 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
14 changes: 12 additions & 2 deletions .github/workflows/build-src.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,14 +28,15 @@ jobs:
uses: actions/checkout@v4
with:
ref: ${{ github.event.pull_request.head.sha }}
fetch-depth: 0
fetch-depth: 50
Copy link
Collaborator

Choose a reason for hiding this comment

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

why don't make it fetch-depth: 1 ?

Copy link
Member Author

Choose a reason for hiding this comment

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

because we have to fetch the entire PR history / up to develop, for stuff like clang-diff-format linter and scripted diffs and whatnot

Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we have some mechanism to warn pull requests with >50 commits that they should consolidate or split their PRs to be under this limit?

Copy link
Collaborator

@knst knst Feb 14, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

let's put 999 then? 999 is not really a lot (less than 1 major release), but it will cover for sure big PRs.
Performance to make clone with 999 commits should be still fast.

Examples of PR with big amount of commits:
109 commits - #4752
111 commits - #4512

Copy link
Collaborator

Choose a reason for hiding this comment

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

Works for me, I was going to suggest something lower like 128 since there really is little reason for a PR that large to exist but since we aren't implementing a warning mechanism, 999 is more than enough to cover any PR, sane or otherwise.

Copy link
Member Author

Choose a reason for hiding this comment

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

Honestly; no. I don't ever want to merge a PR more than 50 commits :D


- name: Initial setup
id: setup
run: |
git config --global --add advice.detachedHead false
git config --global --add safe.directory "$PWD"
GIT_HEAD="$(git rev-parse HEAD)"
git fetch origin develop:develop
git checkout develop
git checkout "${GIT_HEAD}"
BUILD_TARGET="${{ inputs.build-target }}"
Expand Down Expand Up @@ -64,7 +65,7 @@ jobs:
${{ runner.os }}-${{ inputs.build-target }}-${{ steps.setup.outputs.HOST }}
${{ runner.os }}-${{ inputs.build-target }}

- name: Build source and run unit tests
- name: Build source
run: |
CCACHE_SIZE="400M"
CACHE_DIR="/cache"
Expand All @@ -73,6 +74,15 @@ jobs:
BUILD_TARGET="${{ inputs.build-target }}"
source ./ci/dash/matrix.sh
./ci/dash/build_src.sh
ccache -X 9
ccache -c
shell: bash

- name: Run unit tests
run: |
BASE_OUTDIR="/output"
BUILD_TARGET="${{ inputs.build-target }}"
source ./ci/dash/matrix.sh
./ci/dash/test_unittests.sh
shell: bash

Expand Down
Loading