ci: Run Windows native task on GitHub Actions

hebasto commented at 9:10 pm on July 27, 2023: member

Thus, someone would have to sponsor an amount of roughly 5kUSD/mo for those two tasks.

If the goal is to stay on a free plan, I think the only option is GitHub Actions CI.

Historical context:

Security concerns:

GITHUB_TOKEN permissions (from the build log in my personal repo):

02023-07-27T07:30:17.8313534Z ##[group]GITHUB_TOKEN Permissions
12023-07-27T07:30:17.8314113Z Contents: read
22023-07-27T07:30:17.8314608Z Metadata: read
32023-07-27T07:30:17.8314957Z Packages: read
42023-07-27T07:30:17.8315233Z ##[endgroup]

Comparison of resources:

Resource	Current, Cirrus CI	Suggested, GitHub Actions
CPU	6	2
RAM, GB	12	7

The TEST_RUNNER_TIMEOUT_FACTOR variable is set to the current default value for all CI tasks: https://github.com/bitcoin/bitcoin/blob/64440bb733896a7a2caf902825e0406cb993e666/ci/test/00_setup_env.sh#L48

DrahtBot commented at 9:10 pm on July 27, 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
Concept ACK	real-or-random
Stale ACK	dergoegge

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 Jul 27, 2023

maflcko commented at 5:46 am on July 28, 2023: member

Can you remove the Windows Cirrus task here, as it will need to be removed either way? Also, a link to a build would be nice, before this is enabled here.

maflcko commented at 5:47 am on July 28, 2023: member

Maybe also print the GITHUB_TOKEN permissions at every start of the task, just to double check they are read-only?

in .github/workflows/ci.yml:8 in a22565de13 outdated

0@@ -0,0 +1,190 @@
1+# Copyright (c) 2023 The Bitcoin Core developers
2+# Distributed under the MIT software license, see the accompanying
3+# file COPYING or http://www.opensource.org/licenses/mit-license.php.
4+
5+name: bitcoin-core-ci
6+run-name: Bitcoin Core CI
7+on: [pull_request, push]

maflcko commented at 5:55 am on July 28, 2023:

0on: [pull_request, push]  # https://docs.github.com/en/actions/using-workflows/workflow-syntax-for-github-actions#onpushpull_requestpull_request_targetpathspaths-ignore

(Could add a link to the docs)

hebasto commented at 10:48 am on July 28, 2023:

Done.

hebasto force-pushed on Jul 28, 2023

hebasto commented at 10:51 am on July 28, 2023: member

Can you remove the Windows Cirrus task here, as it will need to be removed either way?

Done.

Maybe also print the GITHUB_TOKEN permissions at every start of the task, just to double check they are read-only?

They are printed by default. To see them, in https://github.com/hebasto/bitcoin/actions/runs/5685393993/job/15410187833, one needs to expand “Set up job”, then expand “GIHUB_TOKEN Permissions”.

maflcko commented at 10:55 am on July 28, 2023: member

Ok, so I guess someone needs to enable Actions in both orgs for this to proceed.

maflcko commented at 10:56 am on July 28, 2023: member

In the meantime it may be good to do 10 runs and then check how many of them fail

hebasto commented at 9:10 am on July 29, 2023: member

In the meantime it may be good to do 10 runs and then check how many of them fail

Observing intermittent “Error: no RPC connection”. Converting this PR to a draft for now.

UPD. Timeout has been adjusted.

hebasto marked this as a draft on Jul 29, 2023

hebasto force-pushed on Jul 29, 2023

hebasto marked this as ready for review on Jul 30, 2023

hebasto commented at 2:35 pm on July 30, 2023: member

In the meantime it may be good to do 10 runs and then check how many of them fail

10 runs are green for the recent commit:

Ok, so I guess someone needs to enable Actions in both orgs for this to proceed. @sipa @fanquake

Mind enabling GitHub Actions in the https://github.com/bitcoin/bitcoin repository?

I’m suggesting the repository “Actions permissions” as follows:

DrahtBot added the label Needs rebase on Jul 31, 2023

hebasto force-pushed on Aug 1, 2023

hebasto commented at 9:00 am on August 1, 2023: member

Rebased on top of the merged #28188.

DrahtBot removed the label Needs rebase on Aug 1, 2023

hebasto commented at 7:52 am on August 5, 2023: member

Converting this PR to a draft as it conflicts with #28187.

Please review #28187 first.

hebasto marked this as a draft on Aug 5, 2023

DrahtBot added the label Needs rebase on Aug 15, 2023

hebasto commented at 5:19 pm on August 19, 2023: member

#28292 is a blocker for this PR.

maflcko commented at 8:31 am on August 21, 2023: member

#28292 is a blocker for this PR.

Is it? It should only affect the cache hit percentage, no? And the cache hit percentage shouldn’t affect the behavior of this pull, except for the cache hit percentage, potentially.

hebasto commented at 10:42 am on August 21, 2023: member

#28292 is a blocker for this PR.

Is it? It should only affect the cache hit percentage, no?

Yes, that is what I mean. Without #28292, caches for the master branch are to be evicted very often. And this PR makes it roughly 2 times more often.

maflcko commented at 10:51 am on August 21, 2023: member

Ok, fair enough. But that shouldn’t be a blocker to rebase this, and for code review to happen on the rebase.

hebasto force-pushed on Aug 21, 2023

hebasto marked this as ready for review on Aug 21, 2023

hebasto marked this as a draft on Aug 21, 2023

hebasto force-pushed on Aug 21, 2023

hebasto commented at 11:12 am on August 21, 2023: member

Rebased.

hebasto marked this as ready for review on Aug 21, 2023

DrahtBot removed the label Needs rebase on Aug 21, 2023

in .github/workflows/ci.yml:256 in 699c941d33 outdated

255+
256+      - name: Run functional tests
257+        env:
258+          PYTHONUTF8: 1
259+        shell: cmd
260+        run: python test\functional\test_runner.py --ci --tmpdirprefix=%RUNNER_TEMP% --combinedlogslen=99999999 --timeout-factor=%TEST_RUNNER_TIMEOUT_FACTOR% --quiet

maflcko commented at 11:23 am on August 21, 2023:

Any reason to remove --extended --exclude feature_dbcrash? If yes, create a separate pull/commit. If no, maybe keep as-is?

hebasto commented at 4:17 pm on August 21, 2023:

Well, a couple of tests run too long:

0feature_index_prune.py                                 | ✓ Passed  | 3307 s
1feature_pruning.py                                     | ✓ Passed  | 2845 s

maflcko commented at 7:02 am on August 22, 2023:

Is that longer than before?

hebasto commented at 12:12 pm on August 23, 2023:

https://cirrus-ci.com/task/5095403331256320:

0feature_index_prune.py                                 | ✓ Passed  | 2695 s
1feature_pruning.py                                     | ✓ Passed  | 2409 s

maflcko commented at 12:17 pm on August 23, 2023:

An alternative would be to run them on non-pr pushes only. Failures should be rare enough to deal with them post-merge.

hebasto commented at 11:12 pm on August 28, 2023:

An alternative would be to run them on non-pr pushes only. Failures should be rare enough to deal with them post-merge.

Done in #28360.

in .github/workflows/ci.yml:264 in 699c941d33 outdated

251+        run: python test\util\test_runner.py
252+
253+      - name: Run rpcauth test
254+        run: python test\util\rpcauth-test.py
255+
256+      - name: Run functional tests

maflcko commented at 12:11 pm on August 21, 2023:

style-nit: Any reason to give each step a name? When someone is looking at the debug log, they want to debug, so just using the full command as the name shouldn’t have any downside (maybe even benefits) and reduce the bloat here, no?

hebasto commented at 12:28 pm on August 21, 2023:

Now it looks like that:

and

So the command itself is available in the logs.

Besides, some commands are pretty long, for instance:

0python test\functional\test_runner.py --ci --tmpdirprefix=%RUNNER_TEMP% --combinedlogslen=99999999 --timeout-factor=%TEST_RUNNER_TIMEOUT_FACTOR% --quiet

Any reason to give each step a name?

I think, it’s a matter of consistency and personal taste.

In any case, I’m happy to update this PR and remove the name property from specific steps upon your request.

hebasto force-pushed on Aug 21, 2023

in .github/workflows/ci.yml:253 in 753a4af8a2 outdated

252+        run: src\test_bitcoin.exe -l test_suite
253+
254+      - name: Run benchmarks
255+        run: src\bench_bitcoin.exe -sanity-check
256+
257+      - name: Run bitcoin utils tests

maflcko commented at 2:15 pm on August 21, 2023:

0      - name: Run util tests

style-nit: For consistency with the other names?

hebasto commented at 11:48 am on August 22, 2023:

Done.

maflcko commented at 2:17 pm on August 21, 2023: member

Looks like CI doesn’t run on this? I wonder if it makes sense to document the required permissions inside the file?

hebasto force-pushed on Aug 21, 2023

real-or-random commented at 11:27 am on August 22, 2023: contributor

I wonder if it makes sense to document the required permissions inside the file?

I voted against documenting them here.

hebasto commented at 2:21 pm on August 22, 2023: member

Looks like CI doesn’t run on this? @fanquake

I’m kindly asking you to add ilammy/msvc-dev-cmd@* to the repository’s Actions permissions.

I wonder if it makes sense to document the required permissions inside the file?

I voted against documenting them here.

Your opinion about this will be very appreciated as well.

hebasto force-pushed on Aug 23, 2023

hebasto commented at 10:15 am on August 23, 2023: member

Looks like CI doesn’t run on this?

It runs now: https://github.com/bitcoin/bitcoin/actions/runs/5949997773/job/16136915568 @fanquake Thank you so much!

maflcko commented at 12:07 pm on August 23, 2023: member

Please squash your commits according to https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#squashing-commits

Also, you’ll have to adjust the pull request description to remove no longer applicable --extended removal and the TIMEOUT_FACTOR?

hebasto force-pushed on Aug 23, 2023

hebasto commented at 12:54 pm on August 23, 2023: member

Please squash your commits according to master/CONTRIBUTING.md#squashing-commits

Squashed your commits according to master/CONTRIBUTING.md#squashing-commits :D

dergoegge approved

dergoegge commented at 2:14 pm on August 23, 2023: member

lgtm ACK b0e233ba3b61f2ea40b6a5a7534f49a0d4ad4568

DrahtBot added the label Needs rebase on Aug 23, 2023

real-or-random commented at 3:16 pm on August 23, 2023: contributor

lgtm ACK

By the way, what’s an “lgtm ACK”? I’ve seen this a few times in this repo, but its meaning is not explained in https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#conceptual-review

hebasto force-pushed on Aug 23, 2023

hebasto commented at 3:31 pm on August 23, 2023: member

Rebased on top of the merged #21652.

DrahtBot removed the label Needs rebase on Aug 23, 2023

DrahtBot added the label CI failed on Aug 24, 2023

DrahtBot removed the label CI failed on Aug 24, 2023

hebasto requested review from maflcko on Aug 24, 2023

hebasto requested review from dergoegge on Aug 24, 2023

dergoegge commented at 10:05 am on August 24, 2023: member

By the way, what’s an “lgtm ACK”?

(at least for me) it’s just a normal ack pre-fixed with “looks good to me”

dergoegge approved

dergoegge commented at 10:12 am on August 24, 2023: member

reACK a3ebaa20eaab37b9ec32d6569fbc63d6216fe1b5

hebasto commented at 8:25 am on August 25, 2023: member

Friendly ping @MarcoFalke :)

in .github/workflows/ci.yml:265 in a3ebaa20ea outdated

260+      - name: Run rpcauth test
261+        run: py -3 test\util\rpcauth-test.py
262+
263+      - name: Run functional tests
264+        env:
265+          PYTHONUTF8: 1

maflcko commented at 8:35 am on August 25, 2023:

why only set this for the fun tests? This should be global

hebasto commented at 9:01 am on August 25, 2023:

why only set this for the fun tests?

Because only functional tests require it.

This should be global

Which python scripts for?

hebasto commented at 9:21 am on August 25, 2023:

This should be global

Which level at? The workflow or the Windows job?

maflcko commented at 12:32 pm on August 25, 2023:

The same level that was used in Cirrus. No need to change anything here.

hebasto commented at 12:50 pm on August 25, 2023:

The same level that was used in Cirrus. No need to change anything here.

Done.

in .github/workflows/ci.yml:227 in a3ebaa20ea outdated

221+        with:
222+          path: C:/vcpkg/downloads/tools
223+          key: ${{ github.job }}-vcpkg-tools
224+
225+      - name: vcpkg binary cache
226+        uses: actions/cache@v3

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

This will cycle and evict the caches in pulls, no?

hebasto commented at 8:58 am on August 25, 2023:

While key remains the same, pull requests will restore cache from the master branch and make no new saves.

in .github/workflows/ci.yml:210 in a3ebaa20ea outdated

204+      - name: Restore Ccache cache
205+        uses: actions/cache/restore@v3
206+        with:
207+          path: ~/AppData/Local/ccache
208+          key: ${{ github.job }}-ccache-${{ github.run_id }}
209+          restore-keys: ${{ github.job }}-ccache-

maflcko commented at 10:39 am on August 25, 2023:

This will cycle and evict the caches in pulls, no?

hebasto commented at 12:27 pm on August 25, 2023:

No, it won’t.

It is the “Restore” action. The paired “Save” action is guarded with https://github.com/bitcoin/bitcoin/blob/a3ebaa20eaab37b9ec32d6569fbc63d6216fe1b5/.github/workflows/ci.yml#L245

maflcko commented at 12:32 pm on August 25, 2023:

Ah sorry, the GHA syntax is so verbose

in .github/workflows/ci.yml:196 in a3ebaa20ea outdated

190+        with:
191+          path: |
192+            C:\ProgramData\chocolatey\lib\ccache
193+            C:\ProgramData\chocolatey\bin\ccache.exe
194+            C:\ccache\cl.exe
195+          key: ${{ github.job }}-ccache-installation-${{ env.CI_CCACHE_VERSION }}

maflcko commented at 10:44 am on August 25, 2023:

Any reason to cache this?

hebasto commented at 12:35 pm on August 25, 2023:

The “Install Ccache” step takes 19 seconds only. This time is negligible in comparison to the whole workflow runtime.

However, caching is still useful to avoid networking issues that are possible during choco install ccache command execution.

ci: Run "Win64 native" job on GitHub Actions 6a43372980

hebasto force-pushed on Aug 25, 2023

hebasto commented at 12:54 pm on August 25, 2023: member

Updated a3ebaa20eaab37b9ec32d6569fbc63d6216fe1b5 -> 6a4337298035683a8748ae446eae90e5c5f41d1b (pr28173.11 -> pr28173.12):

rebased
addressed @MarcoFalke’s comment

maflcko commented at 1:03 pm on August 25, 2023: member

lgtm

hebasto requested review from dergoegge on Aug 25, 2023

maflcko requested review from fanquake on Aug 28, 2023

maflcko commented at 8:13 am on August 28, 2023: member

Would be good to merge it, otherwise the CI will break this week.

maflcko added this to the milestone 26.0 on Aug 28, 2023

fanquake merged this on Aug 28, 2023

fanquake closed this on Aug 28, 2023

in .github/workflows/ci.yml:196 in 6a43372980

191+        with:
192+          path: |
193+            C:\ProgramData\chocolatey\lib\ccache
194+            C:\ProgramData\chocolatey\bin\ccache.exe
195+            C:\ccache\cl.exe
196+          key: ${{ github.job }}-ccache-installation-${{ env.CI_CCACHE_VERSION }}

maflcko commented at 6:42 pm on August 28, 2023:

Looks like the cache key will additionally include the branch name / context, so this will cycle the cache. See https://github.com/bitcoin/bitcoin/actions/caches

0win64-native-ccache-installation-4.7.5 2.7 MB cached August 28, 2023 08:55
1refs/pull/27601/merge 
2
3 win64-native-ccache-installation-4.7.5 2.7 MB cached August 28, 2023 07:43
4refs/pull/26312/merge

hebasto commented at 8:14 pm on August 28, 2023:

#28173 (review):

While key remains the same, pull requests will restore cache from the master branch and make no new saves.

A few pull requests created their caches since they had not been created for the master branch yet.

All recent pull requests skipped saving their caches because they hit the caches from the master branch.

in .github/workflows/ci.yml:224 in 6a43372980

219+
220+      - name: vcpkg tools cache
221+        uses: actions/cache@v3
222+        with:
223+          path: C:/vcpkg/downloads/tools
224+          key: ${{ github.job }}-vcpkg-tools

maflcko commented at 6:43 pm on August 28, 2023:

same, etc …

hebasto commented at 8:14 pm on August 28, 2023:

#28173 (review)

hebasto deleted the branch on Aug 28, 2023

fanquake referenced this in commit 13e169a0c6 on Aug 30, 2023

in .cirrus.yml:196 in 6a43372980

191-    - python test\util\rpcauth-test.py
192-  functional_tests_script:
193-    # Increase the dynamic port range to the maximum allowed value to mitigate "OSError: [WinError 10048] Only one usage of each socket address (protocol/network address/port) is normally permitted".
194-    # See: https://learn.microsoft.com/en-us/biztalk/technical-guides/settings-that-can-be-modified-to-improve-network-performance
195-    - netsh int ipv4 set dynamicport tcp start=1025 num=64511
196-    - netsh int ipv6 set dynamicport tcp start=1025 num=64511

maflcko commented at 7:12 am on September 1, 2023:

Is this no longer needed? Looks like the functional tests fail on almost every push?

hebasto commented at 8:57 am on September 1, 2023:

No, logs say nothing about WinError 10048 error.

See #28384 for a proposed fix.

sidhujag referenced this in commit 4a7a2303e9 on Sep 26, 2023

sidhujag referenced this in commit b2a4085030 on Sep 26, 2023

bitcoin locked this on Aug 31, 2024

ci: Run Windows native task on GitHub Actions #28173

Reviews

Conflicts