ci: Move tidy to persistent worker

maflcko commented at 9:54 am on August 4, 2023: member

Cirrus CI will be capping the free compute soon. For now, switch more tasks to persistent worker, as recommended by Cirrus CI.

(See slightly related discussion in #28098)

Also, add more docs.

DrahtBot commented at 9:54 am on August 4, 2023: contributor

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

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	hebasto
Stale ACK	pinheadmz

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

Conflicts

No conflicts as of last run.

DrahtBot added the label Tests on Aug 4, 2023

hebasto commented at 10:05 am on August 4, 2023: member

macOS task has not been started:

UPD: Scheduled in 14 minutes:

maflcko commented at 10:17 am on August 4, 2023: member

UPD: Scheduled in 14 minutes:

When there are pushes to many pull requests at the same time, scheduling may take a few minutes. This is similar to the Cirrus CI hosted open source compute cluster.

maflcko force-pushed on Aug 4, 2023

maflcko requested review from hebasto on Aug 8, 2023

maflcko requested review from pinheadmz on Aug 8, 2023

hebasto approved

hebasto commented at 10:20 am on August 9, 2023: member

ACK fa960fefb589ac97de206f46d7b5c565fc70e3db. I have reviewed the code and it looks OK.

However, I’m a bit reluctant about running macOS-cross job on a persistent worker. I’d prefer run it on GitHub Actions due to the ability to create downloadable artifacts that is used in the QML repo to make designers’ life a bit easier.

I understand, that there is no open PR with such a suggestion for now. But there is an ongoing Linux jobs related discussion in https://github.com/bitcoin-core/secp256k1/pull/1396.

It would be nice if we postpone moving macOS-cross job for a week or two.

ci: Move tidy to persistent worker fa1d8955f6

maflcko force-pushed on Aug 9, 2023

maflcko renamed this:
~~ci: Move tidy and macOS-cross to persistent workers~~
ci: Move tidy to persistent worker
on Aug 9, 2023

DrahtBot renamed this:
~~ci: Move tidy to persistent worker~~
ci: Move tidy to persistent worker
on Aug 9, 2023

maflcko commented at 10:52 am on August 9, 2023: member

It would be nice if we postpone moving macOS-cross job for a week or two.

Done for now. Should be trivial to re-ACK.

hebasto approved

hebasto commented at 10:55 am on August 9, 2023: member

re-ACK fa1d8955f69d3934f975f42eb04b5a3fc0d8aa35.

Thank you.

fanquake commented at 11:53 am on August 9, 2023: member

I’d prefer run it on GitHub Actions due to the ability to create downloadable artifacts that is used in the QML repo to make designers’ life a bit easier.

Why can’t this just be done in the QML repo if it’s required there?

maflcko commented at 11:55 am on August 9, 2023: member

I’ll create a separate pull/thread for macOS-cross next week, so that it can be discussed.

in .cirrus.yml:126 in fa1d8955f6 outdated

128+  persistent_worker:
129+    labels:
130+      type: medium
131   env:
132-    << : *CIRRUS_EPHEMERAL_WORKER_TEMPLATE_ENV
133+    FILE_ENV: "./ci/test/00_setup_env_native_tidy.sh"

pinheadmz commented at 6:38 pm on August 9, 2023:

doesn’t need PERSISTENT_WORKER_TEMPLATE_ENV ?

maflcko commented at 8:37 am on August 14, 2023:

Yes, I’ll remove it in a follow-up

maflcko commented at 8:45 am on August 14, 2023:

The reason is that the ENV only sets one env var, which can (and is) also be done directly in the screen command.

RESTART_CI_DOCKER_BEFORE_RUN=1 screen cirrus ...

maflcko commented at 10:00 am on August 15, 2023:

Removed in this pull. Should be trivial to re-ACK.

pinheadmz approved

pinheadmz commented at 7:00 pm on August 9, 2023: member

ACK fa1d8955f69d3934f975f42eb04b5a3fc0d8aa35

Lots of great comments, code change looks good to me and obviously works on ci. I also ran the task locally (although on macOS I had to enter the container and manually install clang with arm support). One question below and also I’m wondering if the output of “include-what-you-use” is used at all? It makes a lot of suggestions but none of that counts as errors?

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA256
 2
 3ACK fa1d8955f69d3934f975f42eb04b5a3fc0d8aa35
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQIzBAEBCAAdFiEE5hdzzW4BBA4vG9eM5+KYS2KJyToFAmTT4bgACgkQ5+KYS2KJ
 7yTqpug//S8FFIisKLeNiyUR8DjdLK+twfLOvkRu8iVwBsqgTXFQXxWY6xzbDkqTB
 8xGpyIGwpJWrREnu9ec2Ru4R8+PPcLwGmKiSET12XfUHWvinrZe0O0nUl1QlckV/h
 9shmsJkUzF9Zw839mZXL7UevQCg2QfMXYw+iMYgc0dGXL0qncsWAhphfr6LDCaf+h
10zNEwhx/KfiPszQPlpeb3lJvV4joE4vGJEvUZ2FAt5vRB89b+68tjL4D3l+yKFhiD
11bVT9k7D+nAAmm2suuXM6CSyQBZPEEknhrgK3OfLzDS/RZdrBoDSkrae4KsM9nENs
121TJ0K/x9tZCjQlmTKjEEdOgxv1kCmcQkSwqq57k5q5BjbyfVCAxxYhN7LB0kUdnr
13Ynjs/wzAVYb+kPsWvBi0gbEsl4LuFEGSMsSFTsowtoDlA49TNjTTQwzJMbky9nPh
146A5LBUAZRPlD6rDorEUoxL85Amj/DiL52xsHKF9qnc+yI0n26/xv8gm8WTD7tKc0
15jI2GYVSCD23VBO0wYvan8cxbBDLAjF13eEVtFT4r31RBTxoyWiG0Ty+qd3qMhvKE
16JspOA2sAscWFIjlFVH0AOyvMDU4o1rU7XLHCvFGc9Gj9imWxIpHEOTy0A8q78oSn
17tW4czoaWWXmC3tHphs2/qA90txKM8lUuqi8sl+aqhUqSo8lv1Eg=
18=If5B
19-----END PGP SIGNATURE-----

pinheadmz’s public key is on keybase

hebasto commented at 8:34 pm on August 13, 2023: member

I’ll create a separate pull/thread for macOS-cross next week, so that it can be discussed.

For a proposal to move macOS-cross to GitHub Actions, please see #28265.

maflcko commented at 8:42 am on August 14, 2023: member

I also ran the task locally (although on macOS I had to enter the container and manually install clang with arm support).

Yeah, that’s unrelated, because I only modify the cirrus yml, not the CI task config. Maybe create a separate issue, though I won’t be able to help, since I don’t have or use macOS.

One question below and also I’m wondering if the output of “include-what-you-use” is used at all? It makes a lot of suggestions but none of that counts as errors?

Yeah, that is on purpose and unchanged in this pull. Currently iwyu still has bugs (false negatives and false positives) and there are also trillion true positives and the true negatives sometimes lead to compile errors down the line in other modules.

I think for now it is best to check the iwyu output during review for newly added code or when a module is modified and touched for other reasons.

maflcko requested review from fanquake on Aug 14, 2023

hebasto commented at 4:48 pm on August 14, 2023: member

One question below and also I’m wondering if the output of “include-what-you-use” is used at all? It makes a lot of suggestions but none of that counts as errors?

Yeah, that is on purpose and unchanged in this pull. Currently iwyu still has bugs (false negatives and false positives) and there are also trillion true positives and the true negatives sometimes lead to compile errors down the line in other modules.

I think for now it is best to check the iwyu output during review for newly added code or when a module is modified and touched for other reasons.

* PERSISTENT_WORKER_TEMPLATE_ENV is not needed at all, because
  RESTART_CI_DOCKER_BEFORE_RUN is already set on the persistent worker.
* PERSISTENT_WORKER_TEMPLATE can be replaced by pinning the
  previous_releases task to a type of worker. This should make the CI
  performance more consistent.

faaa0794b2

maflcko requested review from hebasto on Aug 16, 2023

hebasto approved

hebasto commented at 8:31 am on August 16, 2023: member

re-ACK faaa0794b24f250f787bc9b9605270108ea101a0

DrahtBot requested review from pinheadmz on Aug 16, 2023

fanquake merged this on Aug 17, 2023

fanquake closed this on Aug 17, 2023

maflcko deleted the branch on Aug 17, 2023

sidhujag referenced this in commit 8288cb77d7 on Aug 17, 2023

ci: Move tidy to persistent worker #28214

Reviews

Conflicts