Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ci: fix build-test job with --no-default-features, add miniscript/no-std #1636

Conversation

notmandatory
Copy link
Member

@notmandatory notmandatory commented Oct 1, 2024

Description

Fixes the CI build-test job with --no-default-features by also adding --features miniscript/no-std.

Until rust-miniscript removes the no-std feature we need to enable it when --no-default-features is used to build bdk_wallet or the whole workspace. See also the check-no-std job which does the same plus enables the bdk_chain/hashbrown feature which is also needed to build bdk_wallet with --no-default-features but is already enabled when building the whole workspace.

Notes to the reviewers

I think we didn't catch this on #1625 because the CI job names changed and I didn't update the branch merge requirements. Another possibility is it was passing because of cached build artifacts which I removed last night when I was trying to troubleshoot something else. I've updated the required CI jobs that need to pass before allowing a PR to be merged to master to include the ones with --no-default-features --features bdk_chain/hashbrown in the name.

Checklists

All Submissions:

  • I've signed all my commits
  • I followed the contribution guidelines
  • I ran cargo fmt and cargo clippy before committing

Until rust-miniscript removes the no-std feature we need to enable it
when --no-default-features is used to build bdk_wallet or the
whole workspace. See also the check-no-std job which does the same plus
enables the bdk_chain/hashbrown feature which is also needed to build
bdk_wallet with --no-default-features but is already enabled when
building the whole workspace.
@notmandatory notmandatory self-assigned this Oct 1, 2024
@notmandatory notmandatory added the ci label Oct 1, 2024
@notmandatory notmandatory added this to the 1.0.0-beta milestone Oct 1, 2024
Copy link
Contributor

@ValuedMammal ValuedMammal left a comment

Choose a reason for hiding this comment

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

ACK 0e80824

@ValuedMammal ValuedMammal merged commit b8746c8 into bitcoindevkit:master Oct 1, 2024
21 checks passed
Copy link
Contributor

@oleonardolima oleonardolima left a comment

Choose a reason for hiding this comment

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

post-merge ACK 0e80824

I think this approach also partially unblocks #1018 and #1575, should we move forward with those?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants