Skip to content

ci: Simplify module configuration and extend test coverage#1840

Open
mllwchrry wants to merge 1 commit intobitcoin-core:masterfrom
mllwchrry:ci-simplify-modules
Open

ci: Simplify module configuration and extend test coverage#1840
mllwchrry wants to merge 1 commit intobitcoin-core:masterfrom
mllwchrry:ci-simplify-modules

Conversation

@mllwchrry
Copy link
Copy Markdown
Contributor

Enables all modules by default, and tests the disabling of each module separately (respecting the dependency chain). This simplifies the configuration of modules in CI and extends the test coverage.

The extended coverage is proven by exposing pre-existing issues fixed in #1837 and #1839.

@real-or-random
Copy link
Copy Markdown
Contributor

Concept ACK

This simplifies the CI file and it gives us more test coverage.

cc @hebasto

I kicked CI. The failed runs all had some network/GHA issues.

@real-or-random real-or-random requested a review from Copilot March 25, 2026 20:53

This comment was marked as resolved.

@real-or-random
Copy link
Copy Markdown
Contributor

I kicked CI. The failed runs all had some network/GHA issues.

Sorry, apparently I only claimed it but didn't actually press the button. Done now, let's see.

Copy link
Copy Markdown
Member

@hebasto hebasto left a comment

Choose a reason for hiding this comment

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

Concept ACK.

Would it be possible to keep the current approach where a single module is enabled while other modules are disabled, and vice versa? This helps to ensure that they are fully independent of each other.

@real-or-random
Copy link
Copy Markdown
Contributor

Concept ACK.

Would it be possible to keep the current approach where a single module is enabled while other modules are disabled, and vice versa? This helps to ensure that they are fully independent of each other.

Can you clarify? Are you suggesting doing both of the following?

  1. one run for each module, where just that one module is enabled
  2. one run for each module, where just that one module is disabled

If yes, then I don't think we currently do 1, or do we? (You said "keep the current approach...")

@hebasto
Copy link
Copy Markdown
Member

hebasto commented Apr 27, 2026

Concept ACK.
Would it be possible to keep the current approach where a single module is enabled while other modules are disabled, and vice versa? This helps to ensure that they are fully independent of each other.

Can you clarify? Are you suggesting doing both of the following?

  1. one run for each module, where just that one module is enabled

  2. one run for each module, where just that one module is disabled

If yes, then I don't think we currently do 1, or do we? (You said "keep the current approach...")

We do this for the Recovery module. Here:

- env_vars: { WIDEMUL: 'int64', RECOVERY: 'yes' }
- env_vars: { WIDEMUL: 'int64', ECDH: 'yes', EXTRAKEYS: 'yes', SCHNORRSIG: 'yes', MUSIG: 'yes', ELLSWIFT: 'yes' }

Here (almost):

- env_vars: { WIDEMUL: 'int128', RECOVERY: 'yes', EXTRAKEYS: 'yes', SCHNORRSIG: 'yes', MUSIG: 'yes', ELLSWIFT: 'yes' }
- env_vars: { WIDEMUL: 'int128', ECDH: 'yes', EXTRAKEYS: 'yes', SCHNORRSIG: 'yes', MUSIG: 'yes' }

And here:

- { WIDEMUL: 'int128', ECDH: 'yes', EXTRAKEYS: 'yes', SCHNORRSIG: 'yes', MUSIG: 'yes', ELLSWIFT: 'yes' }
- { WIDEMUL: 'int128', RECOVERY: 'yes' }

@mllwchrry
Copy link
Copy Markdown
Contributor Author

Would it be possible to keep the current approach where a single module is enabled while other modules are disabled, and vice versa? This helps to ensure that they are fully independent of each other.

Thanks for the feedback! I wonder what's the failure scenario that the "disable one" approach misses? If A has an unguarded dependency on B, { B: 'no' } catches it.
Solo enable entries would add 16 runs. I think that's ok, but first I want to understand explicitly how it expands the coverage.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants