ci: Run “Windows (VS 2022)” job on GitHub Actions #1389

pull hebasto wants to merge 1 commits into bitcoin-core:master from hebasto:230728-actions changing 1 files +39 −0
  1. hebasto commented at 2:02 pm on July 28, 2023: member

    This PR solves one item in #1392.

    In response to upcoming limiting free usage of Cirrus CI, suggesting to move (partially?) CI tasks/jobs from Cirrus CI to GitHub Actions (GHA).

    Here is example from my personal repo: https://github.com/hebasto/secp256k1/actions/runs/5806269046.

    For security concerns, see:

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

    image

    image

    See build logs in my personal repo: https://github.com/hebasto/secp256k1/actions/runs/5692587475.

  2. MarcoFalke commented at 3:04 pm on July 28, 2023: none
    Same nit here: Could delete the Windows task on Cirrus in this pull, or is there a reason why it should be kept after this is merged?
  3. hebasto commented at 3:12 pm on July 28, 2023: member

    Could delete the Windows task on Cirrus in this pull…

    Deleted.

  4. hebasto force-pushed on Aug 3, 2023
  5. hebasto commented at 1:23 pm on August 3, 2023: member
    Rebased on top of the merged #1290.
  6. real-or-random cross-referenced this on Aug 4, 2023 from issue ci: Future of CI after Cirrus pricing change by real-or-random
  7. real-or-random added the label ci on Aug 8, 2023
  8. in .github/workflows/ci.yml:27 in 72b8d875b2 outdated 
    22+
23+      - name: Generate buildsystem
24+        run: cmake -E env CFLAGS="/WX" cmake -B build -A x64 -DSECP256K1_ENABLE_MODULE_RECOVERY=ON -DSECP256K1_BUILD_EXAMPLES=ON -DBUILD_SHARED_LIBS=${{ matrix.build_shared_libs }}
25+
26+      - name: Build
27+        run: cmake --build build --config RelWithDebInfo -- /p:UseMultiToolTask=true /p:CL_MPcount=3
    real-or-random commented at 5:11 pm on August 8, 2023:
    optional nit: The 3 could be computed as n + 1, where n is read from the system automatically, same below.
    hebasto commented at 7:59 am on August 9, 2023:

    Done.

    In the MSBuild option list, it has been replaced with the high level -maxCpuCount switch.

  9. real-or-random commented at 5:17 pm on August 8, 2023: contributor

    Approach ACK

    Is there a better way to test PRs that change workflows (other than deploying them in your fork?) Or is the problem simply that we haven’t enabled GitHub Actions at all? AFAIU, on [pull_request] should run the workflow from the merge commit, see for example https://github.blog/2020-08-03-github-actions-improvements-for-fork-and-pull-request-workflows/ . Is this correct?

  10. hebasto commented at 5:34 pm on August 8, 2023: member

    Is there a better way to test PRs that change workflows (other than deploying them in your fork?) Or is the problem simply that we haven’t enabled GitHub Actions at all?

    Enabled GitHub Actions in this repo should help.

    AFAIU, on [pull_request] should run the workflow from the merge commit, see for example github.blog/2020-08-03-github-actions-improvements-for-fork-and-pull-request-workflows . Is this correct?

    According to the docs, for on.pull_request:

    a workflow only runs when a pull_request event’s activity type is opened, synchronize, or reopened.

  11. hebasto force-pushed on Aug 9, 2023
  12. hebasto commented at 7:51 am on August 9, 2023: member

    Updated 72b8d875b21bfbd50dd95ec625d95dddcea37614 -> 5a3d45baa00057d6e27f2119ccdeea80108373f1 (pr1389.03 -> pr1389.04, diff):

  13. MarcoFalke approved
  14. MarcoFalke commented at 7:58 am on August 9, 2023: none
    lgtm ACK
  15. hebasto cross-referenced this on Aug 9, 2023 from issue ci: Run "x86_64: macOS Ventura" job on GitHub Actions by hebasto
  16. real-or-random closed this on Aug 9, 2023
  17. real-or-random reopened this on Aug 9, 2023
  18. in .github/workflows/ci.yml:12 in 5a3d45baa0 outdated 
     7+    tags-ignore:
 8+      - '**'
 9+
10+env:
11+  SECP256K1_BENCH_ITERS: 2
12+  SECP256K1_TEST_ITERS: 16
    real-or-random commented at 10:55 am on August 9, 2023:
    Any reason not to keep the default of 64 for TEST_ITERS? We typically set lower values only in special environments such as QEMU.
    hebasto commented at 11:20 am on August 9, 2023:
    I cannot recall what exactly caused my decision. Going to address your comment and make a new push after addressing #1389 (comment).
    hebasto commented at 12:13 pm on August 9, 2023:
    Fixed.
  19. real-or-random commented at 10:57 am on August 9, 2023: contributor
    GitHub Actions should be enabled now. I tried to close and re-open to trigger the workflow, but it didn’t work. But yeah, happy to merge this (with the comment addressed) even if it doesn’t run on this PR. We can always adjust.
  20. hebasto commented at 11:05 am on August 9, 2023: member

    @real-or-random

    GitHub Actions should be enabled now.

    Just to double check, are “Actions permissions” and “Workflow permissions” set to safe values?

  21. hebasto commented at 11:14 am on August 9, 2023: member

    I tried to close and re-open to trigger the workflow, but it didn’t work.

    It should though…

    Let me try once more (perhaps, it depends on whether an actor is an author).

  22. achow101 commented at 11:25 am on August 9, 2023: member
    The suggested permissions were applied.
  23. ci: Run "Windows (VS 2022)" job on GitHub Actions a2f7ccdecc
  24. hebasto force-pushed on Aug 9, 2023
  25. hebasto commented at 12:12 pm on August 9, 2023: member
    Hmm… I’m struggling to figure out the reason for no activity in https://github.com/bitcoin-core/secp256k1/actions.
  26. achow101 commented at 1:05 pm on August 9, 2023: member
    I think github actions only runs if the repo contains a workflow, so prs adding new workflows aren’t going to be run until the repo itself has something.
  27. real-or-random approved
  28. real-or-random commented at 1:33 pm on August 9, 2023: contributor

    utACK 7d05f78fab1c930abdc19da64cc5d576d3523cb7

    Let’s try merging it…

  29. real-or-random commented at 1:38 pm on August 9, 2023: contributor
    Or actually… @hebasto Can you remove the second commit for now, if we’re unsure whether this works at all.
  30. hebasto force-pushed on Aug 9, 2023
  31. hebasto commented at 1:39 pm on August 9, 2023: member

    Or actually… @hebasto Can you remove the second commit for now, if we’re unsure whether this works at all.

    Removed.

  32. real-or-random approved
  33. real-or-random commented at 1:43 pm on August 9, 2023: contributor
    utACK a2f7ccdecc4721d972f36d6aacc5f0c85ce0557d
  34. real-or-random merged this on Aug 9, 2023
  35. real-or-random closed this on Aug 9, 2023
  36. hebasto commented at 1:44 pm on August 9, 2023: member
    image
  37. hebasto deleted the branch on Aug 9, 2023
  38. hebasto cross-referenced this on Aug 9, 2023 from issue ci: Remove "Windows (VS 2022)" task from Cirrus CI by hebasto
  39. real-or-random referenced this in commit 8d2960c8e2 on Aug 9, 2023


hebasto MarcoFalke real-or-random achow101

Labels
ci

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin-core/secp256k1. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-11-24 00:15 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me