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(port): various ccache and matrix improvements #3258

Merged
merged 37 commits into from
Oct 14, 2023

Conversation

scarf005
Copy link
Member

@scarf005 scarf005 commented Sep 25, 2023

Summary

SUMMARY: Build "Ported various ccache and build improvements from DDA"

Purpose of change

speed up test loop by porting various improvements from DDA

Describe the solution

blamed and copied relevant lines. individual referenced commits and PRs are written in commit message.

Commits are cherry-picked from the works of: @BrettDong @kevingranade @irwiss @jbytheway

Testing

I'd be surprised if this doesn't cause mayhem.

Additional context

tests marked as [slow] were disabled to prevent build failure

@olanti-p
Copy link
Contributor

2 failing tests on GCC Curses build, the 1st one seems legit, 2nd may be RNG-dependent

([slow] ~starting_items)=> Aiming at a target partially covered by a wall
([slow] ~starting_items)=> -------------------------------------------------------------------------------
([slow] ~starting_items)=> ../tests/ranged_aiming_test.cpp:226
([slow] ~starting_items)=> ...............................................................................
([slow] ~starting_items)=> 
([slow] ~starting_items)=> ../tests/ranged_aiming_test.cpp:266: FAILED:
([slow] ~starting_items)=>   CHECK( unseen == 0 )
([slow] ~starting_items)=> with expansion:
([slow] ~starting_items)=>   2500 (0x9c4) == 0
([slow] ~starting_items)=> with messages:
([slow] ~starting_items)=>   unseen := 2500 (0x9c4)
([slow] ~starting_items)=>   failed := {  }
([slow] ~starting_items)=> grenade_lethality
([slow] ~starting_items)=> -------------------------------------------------------------------------------
([slow] ~starting_items)=> ../tests/explosion_balance_test.cpp:151
([slow] ~starting_items)=> ...............................................................................
([slow] ~starting_items)=> 
([slow] ~starting_items)=> ../tests/explosion_balance_test.cpp:103: FAILED:
([slow] ~starting_items)=>   CHECK( victims.avg() == Approx( lethality ).margin( margin ) )
([slow] ~starting_items)=> with expansion:
([slow] ~starting_items)=>   0.0 == Approx( 0.400000006 )
([slow] ~starting_items)=> with messages:
([slow] ~starting_items)=>   margin := 0.06f
([slow] ~starting_items)=>   grenade_act
([slow] ~starting_items)=>   range 15
([slow] ~starting_items)=>   96 survivors out of 96 targets.
([slow] ~starting_items)=>   (41,41,0) 80, (19,41,0) 80, (19,19,0) 80, (41,19,0) 80, (42,20,0) 80, (42,21,
([slow] ~starting_items)=>   0) 80, (42,39,0) 80, (42,40,0) 80, (40,42,0) 80, (39,42,0) 80, (21,42,0) 80,
([slow] ~starting_items)=>   (20,42,0) 80, (18,40,0) 80, (18,39,0) 80, (18,21,0) 80, (18,20,0) 80, (20,18,
([slow] ~starting_items)=>   0) 80, (21,18,0) 80, (39,18,0) 80, (40,18,0) 80, (43,21,0) 80, (43,22,0) 80,
([slow] ~starting_items)=>   (43,38,0) 80, (43,39,0) 80, (39,43,0) 80, (38,43,0) 80, (22,43,0) 80, (21,43,
([slow] ~starting_items)=>   0) 80, (17,39,0) 80, (17,38,0) 80, (17,22,0) 80, (17,21,0) 80, (21,17,0) 80,
([slow] ~starting_items)=>   (22,17,0) 80, (38,17,0) 80, (39,17,0) 80, (44,23,0) 80, (44,24,0) 80, (44,36,
([slow] ~starting_items)=>   0) 80, (44,37,0) 80, (37,44,0) 80, (36,44,0) 80, (24,44,0) 80, (23,44,0) 80,
([slow] ~starting_items)=>   (16,37,0) 80, (16,36,0) 80, (16,24,0) 80, (16,23,0) 80, (23,16,0) 80, (24,16,
([slow] ~starting_items)=>   0) 80, (36,16,0) 80, (37,16,0) 80, (45,25,0) 80, (45,26,0) 80, (45,27,0) 80,
([slow] ~starting_items)=>   (45,28,0) 80, (45,29,0) 80, (45,30,0) 80, (45,31,0) 80, (45,32,0) 80, (45,33,
([slow] ~starting_items)=>   0) 80, (45,34,0) 80, (45,35,0) 80, (35,45,0) 80, (34,45,0) 80, (33,45,0) 80,
([slow] ~starting_items)=>   (32,45,0) 80, (31,45,0) 80, (30,45,0) 80, (29,45,0) 80, (28,45,0) 80, (27,45,
([slow] ~starting_items)=>   0) 80, (26,45,0) 80, (25,45,0) 80, (15,35,0) 80, (15,34,0) 80, (15,33,0) 80,
([slow] ~starting_items)=>   (15,32,0) 80, (15,31,0) 80, (15,30,0) 80, (15,29,0) 80, (15,28,0) 80, (15,27,
([slow] ~starting_items)=>   0) 80, (15,26,0) 80, (15,25,0) 80, (25,15,0) 80, (26,15,0) 80, (27,15,0) 80,
([slow] ~starting_items)=>   (28,15,0) 80, (29,15,0) 80, (30,15,0) 80, (31,15,0) 80, (32,15,0) 80, (33,15,
([slow] ~starting_items)=>   0) 80, (34,15,0) 80, (35,15,0) 80, 
([slow] ~starting_items)=>   Wounded survivors: 0
([slow] ~starting_items)=>   average hp of survivors: 80

@scarf005
Copy link
Member Author

2 failing tests on GCC Curses build, the 1st one seems legit, 2nd may be RNG-dependent

are they caused by #3258 ?

@olanti-p
Copy link
Contributor

These tests are marked [slow], and this PR enables them. They've most likely been broken for some time.

@scarf005
Copy link
Member Author

should i disable slow tests and re-introduce them in later PR? i think fixing CCache should be done quickly

@olanti-p
Copy link
Contributor

That could work.

Copy link
Contributor

@olanti-p olanti-p left a comment

Choose a reason for hiding this comment

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

I can confidently say I don't understand much of it, but here's matrix.yml

.github/workflows/matrix.yml Outdated Show resolved Hide resolved
.github/workflows/matrix.yml Outdated Show resolved Hide resolved
.github/workflows/matrix.yml Outdated Show resolved Hide resolved
.github/workflows/matrix.yml Outdated Show resolved Hide resolved
.github/workflows/matrix.yml Outdated Show resolved Hide resolved
.github/workflows/matrix.yml Outdated Show resolved Hide resolved
.github/workflows/matrix.yml Outdated Show resolved Hide resolved
@scarf005
Copy link
Member Author

applied all of the suggestions, let's hope this works...

@scarf005 scarf005 requested a review from olanti-p September 29, 2023 06:57
Copy link
Contributor

@olanti-p olanti-p left a comment

Choose a reason for hiding this comment

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

I completely forgot, but Catch2 has a special tag [!mayfail] we could use to acknowledge that the test sometimes fails.
https://github.com/catchorg/Catch2/blob/v2.x/docs/test-cases-and-sections.md#special-tags
Adding it to failing slow tests may be a better solution that outright disabling all of them.

.github/workflows/matrix.yml Show resolved Hide resolved
build-scripts/gha_compile_only.sh Show resolved Hide resolved
@olanti-p
Copy link
Contributor

olanti-p commented Sep 29, 2023

I don't understand Mac parts, but I didn't see anything suspicious with them.

@scarf005
Copy link
Member Author

scarf005 commented Oct 1, 2023

Adding it to failing slow tests may be a better solution that outright disabling all of them.

makes sense.

@scarf005
Copy link
Member Author

scarf005 commented Oct 1, 2023

image

@github-actions github-actions bot added the tests changes related to tests label Oct 1, 2023
@scarf005
Copy link
Member Author

scarf005 commented Oct 3, 2023

@olanti-p do you think this is ready?

github cache limit is 10GB
@scarf005
Copy link
Member Author

scarf005 commented Oct 3, 2023

@olanti-p i think it's ready for reviewing.

tests/overmap_test.cpp Outdated Show resolved Hide resolved
@scarf005 scarf005 requested a review from olanti-p October 5, 2023 03:56
@scarf005
Copy link
Member Author

scarf005 commented Oct 5, 2023

image

wait, errrr

image

of course, yes, the mac builds.

@scarf005 scarf005 enabled auto-merge October 5, 2023 07:16
@olanti-p
Copy link
Contributor

olanti-p commented Oct 7, 2023

MacOS build failed on this PR, but it's legit related failure this time.

Makefile:536: *** "SDL2 framework not found".  Stop.

@scarf005
Copy link
Member Author

scarf005 commented Oct 7, 2023

i haven't changed anything on macos related code, not really sure what's happening.

@olanti-p
Copy link
Contributor

olanti-p commented Oct 7, 2023

It's complaining about missing dependency, so obviously something here changed how dependencies are detected for the build job.

@scarf005 scarf005 requested a review from olanti-p October 10, 2023 00:53
@scarf005
Copy link
Member Author

failing item display test will be fixed in #3407

@scarf005 scarf005 enabled auto-merge October 10, 2023 13:31
Copy link
Contributor

@olanti-p olanti-p left a comment

Choose a reason for hiding this comment

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

The caching didn't work for a couple builds when I tested it, and a couple other builds received only partial speedup - don't know what's up with that. Either way, some caching is better than no caching, and a couple builds went down to sub 3 minutes, which is definitely an improvement.

@scarf005 scarf005 added this pull request to the merge queue Oct 13, 2023
Merged via the queue into cataclysmbnteam:upload with commit 5f5bf06 Oct 14, 2023
8 of 16 checks passed
@olanti-p olanti-p mentioned this pull request Oct 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tests changes related to tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants