Skip to content

build: run basic feature matrix in CI#313

Closed
nyonson wants to merge 1 commit into2140-dev:masterfrom
nyonson:add-features-to-ci
Closed

build: run basic feature matrix in CI#313
nyonson wants to merge 1 commit into2140-dev:masterfrom
nyonson:add-features-to-ci

Conversation

@nyonson
Copy link
Copy Markdown
Collaborator

@nyonson nyonson commented Mar 18, 2025

First patch focuses on running some feature flag combinations, second patch fixes the tor dep so we can also run all feature flags.

Extending the "build" step to run all feature flags, as well as the existing default. Could possible add more flag combos in the future if things get complicated, but this should cover most cases for now. This helps ensure that the different flag combos exposed by the library actually build for consumers. If the feature flag matrix begins to get more complex, we could look into a more specific tool like cargo-hack, but for now simplicity is fine.

Switched to the optional dependency syntax which ensures that the (unused in our case) implicit features of the optional dependencies are not created. This cleans up the library interface for consumers, only our explicit flags are exposed.

Dropping the "check" CI step since build covers all of it. If build gets really slow in the future, we can add check back as a form of "fast failure", but as it is today this is just duplicating work and slowing CI down.

Requires bumping the tor dependencies due to a break in the pulled in time dependency. This is a conservative upgrade so we don't run into issues with the sqlite dependency, however the MSRV with the tor feature is still above the projects, 1.70.0.

@nyonson nyonson force-pushed the add-features-to-ci branch from 8ad29d4 to b056a64 Compare March 18, 2025 03:35
@nyonson
Copy link
Copy Markdown
Collaborator Author

nyonson commented Mar 18, 2025

Oh hm, looks like all features flags is broken right now. I'll update the patch to drop that till we can fix it.

@nyonson nyonson force-pushed the add-features-to-ci branch 6 times, most recently from 241e5ec to 6243472 Compare March 20, 2025 19:47
@nyonson
Copy link
Copy Markdown
Collaborator Author

nyonson commented Mar 21, 2025

Was able to fix up the tor feature deps just enough to get --all-features build working in f131669, which is nice to avoid any regressions there. The tor feature does break MSRV (it is 1.70.0 with it enabled), but fine for now.

Ideally the MSRV build check was a little more feature thorough, but I think it is good enough to prevent any big regressions.

@rustaceanrob
Copy link
Copy Markdown
Collaborator

rustaceanrob commented Mar 28, 2025

At first glance I think a couple things are going on here. Could you potentially make a new PR for the tokio, corepc and bip324 upgrades so I can get those in first? With a rebase after that I think we can discuss the CI changes in more detail without holding up the upgrades.

I would imagine you can just cherry-pick them onto a new branch pretty easily.

@nyonson
Copy link
Copy Markdown
Collaborator Author

nyonson commented Mar 28, 2025

At first glance I think a couple things are going on here. Could you potentially make a new PR for the tokio, corepc and bip324 upgrades so I can get those in first? With a rebase after that I think we can discuss the CI changes in more detail without holding up the upgrades.

I would imagine you can just cherry-pick them onto a new branch pretty easily.

Hm yea, I forget if the bumps are necessary to get CI to work, but if that is the case I'll rebase them down.

@nyonson nyonson force-pushed the add-features-to-ci branch from f131669 to eade7c8 Compare March 28, 2025 16:40
@nyonson
Copy link
Copy Markdown
Collaborator Author

nyonson commented Mar 28, 2025

Ok, was able to move out the dependency bumps except for the tor one which is essential for the second patch eade7c8

Extending the "build" step to run all feature flags, as well as
the existing default. Could possible add more flag combos in the future
if things get complicated, but this should cover most cases for now.
This helps ensure that the different flag combos exposed by the library
actually build for consumers. If the feature flag matrix begins to get
more complex, we could look into a more specific tool like cargo-hack,
but for now simplicity is fine.

Switched to the optional dependency syntax which ensures that the
(unused in our case) implicit features of the optional dependencies are
not created. This cleans up the library interface for consumers, only
our explicit flags are exposed.

Dropping the "check" CI step since build covers all of it. If build gets
really slow in the future, we can add check back as a form of "fast
failure", but as it is today this is just duplicating work and slowing
CI down.
@nyonson nyonson force-pushed the add-features-to-ci branch from eade7c8 to 5fed91d Compare April 1, 2025 00:43
@rustaceanrob
Copy link
Copy Markdown
Collaborator

Hate to hold this up forever, but since it touches the "database" feature, it seems like an opportunity to get rid of that horrible feature name. In the case we implement a new storage backend, like redb, then the feature name would be too opaque as to what backend is being used. Can we potentially change this feature name to sqlite?

@nyonson
Copy link
Copy Markdown
Collaborator Author

nyonson commented Apr 2, 2025

Hate to hold this up forever, but since it touches the "database" feature, it seems like an opportunity to get rid of that horrible feature name. In the case we implement a new storage backend, like redb, then the feature name would be too opaque as to what backend is being used. Can we potentially change this feature name to sqlite?

I think we should follow convention and name the feature after the dependency, so in this case rusqlite. Something Kix was talking about for the bip324 lib here: rust-bitcoin/bip324#103 (comment)

@rustaceanrob
Copy link
Copy Markdown
Collaborator

Works for me

@nyonson
Copy link
Copy Markdown
Collaborator Author

nyonson commented Apr 2, 2025

Going to just fold this into #322

@nyonson nyonson closed this Apr 2, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants