ci: drop -priority-level from bench in win cross CI #32288

pull fanquake wants to merge 1 commits into bitcoin:master from fanquake:win_cross_ci_bench_no_prio changing 1 files +1 −1

fanquake commented at 3:32 PM on April 16, 2025: member

So there's at least one CI sanity checking all benchmarks.

Related to #32277.
DrahtBot commented at 3:32 PM on April 16, 2025: contributor


The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.



Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32288.



Reviews

See the guideline for information on the review process.

Type Reviewers

ACK l0rinc, hebasto, mabu44

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.



Conflicts

Reviewers, this pull request conflicts with the following ones:
- #32219 (DRAFT: test: enabling wallet migration functional test on windows by m3dwards)
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.
DrahtBot added the label Tests on Apr 16, 2025
l0rinc commented at 3:47 PM on April 16, 2025: contributor

utACK 3f602f056001dc36657f01a938f62ea722f78854 - was thinking of pushing something similar

Type	Reviewers
ACK	l0rinc, hebasto, mabu44

ci: drop -priority-level from bench in win cross CI

So there's at least one CI sanity checking all benchmarks.

Related to #32277.

27f11217ca

fanquake force-pushed on Apr 17, 2025
in .github/workflows/ci.yml:361 in 27f11217ca
```
 357 | @@ -358,7 +358,7 @@ jobs:
 358 |            ./src/univalue/unitester.exe
 359 |  
 360 |        - name: Run benchmarks
 361 | -        run: ./bin/bench_bitcoin.exe -sanity-check -priority-level=high
 362 | +        run: ./bin/bench_bitcoin.exe -sanity-check
```
hebasto commented at 9:25 AM on April 17, 2025:

Why not drop -priority-level=high here as well:https://github.com/bitcoin/bitcoin/blob/7a3afe6787bc36fd98684eac020f9abdbfae6f0a/src/bench/CMakeLists.txt#L82-L84 ?

Will it have a significant impact on execution time?

fanquake commented at 9:27 AM on April 17, 2025:

I'm just changing CI, so that we catch regressions. Someone could follow up with changing this for all developers & users.

hebasto commented at 9:44 AM on April 17, 2025:

But this introduces another inconsistency between Windows and other platforms. Initially, run: ./bin/bench_bitcoin.exe -sanity-check -priority-level=high was written to mimic the bench_sanity_check_high_priority test task behaviour.

fanquake commented at 9:45 AM on April 17, 2025:

Can you explain your issue with running more tests in this CI?

hebasto commented at 9:54 AM on April 17, 2025:

If it only takes a couple of extra seconds, why run a different set of tests for each platform?

Windows should, to the best of our ability, be treated the same as any other platform.

fanquake commented at 10:06 AM on April 17, 2025:

If it only takes a couple of extra seconds,

I haven't measured, for all users, developers and CIs. The point of this change is to immediately start catching regressions in at least one CI.

hebasto commented at 10:15 AM on April 17, 2025:

The point of this change is to immediately start catching regressions in at least one CI.

Ok. Resolved.

maflcko commented at 10:18 AM on April 17, 2025:

The suggestion sounds reasonable. I'd be happy to review a pull doing it.
l0rinc commented at 9:39 AM on April 17, 2025: contributor

utACK 27f11217ca63e0f8f78f14db139150052dcd9962
hebasto approved
hebasto commented at 10:13 AM on April 17, 2025: member

ACK 27f11217ca63e0f8f78f14db139150052dcd9962.
mabu44 commented at 10:33 AM on April 17, 2025: none

utACK 27f11217ca63e0f8f78f14db139150052dcd9962
fanquake merged this on Apr 17, 2025
fanquake closed this on Apr 17, 2025
fanquake deleted the branch on Apr 17, 2025
l0rinc commented at 4:36 PM on April 17, 2025: contributor

Seems related: https://github.com/l0rinc/bitcoin/actions/runs/14519683955/job/40737543737?pr=8

Running with -sanity-check option, output is being suppressed as benchmark results will be useless. terminate called after throwing an instance of 'std::filesystem::__cxx11::filesystem_error' what(): filesystem error: cannot remove all: The process cannot access the file because it is being used by another process [C:\Users\RUNNER~1\AppData\Local\Temp\test_common bitcoin\WalletMigration\af1d7c4664d64d17a328] [C:\Users\RUNNER~1\AppData\Local\Temp\test_common bitcoin\WalletMigration\af1d7c4664d64d17a328\regtest\wallet.dat]

Contributors

Labels

Linked (view graph)

#32277 bench: WalletMigration benchmark broken

ci: drop -priority-level from bench in win cross CI #32288

Code Coverage & Benchmarks

Reviews

Conflicts