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.

Sign up for GitHub

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

Jump to bottom

chore: fix RUSTSEC-2024-0370 proc-macro-error is unmaintained #1603

Merged
merged 6 commits into from
Sep 28, 2024
Merged

chore: fix RUSTSEC-2024-0370 proc-macro-error is unmaintained #1603

merged 6 commits into from
Sep 28, 2024

Conversation

oleonardolima
Copy link
Contributor

@oleonardolima oleonardolima commented Sep 12, 2024 •
edited
Loading

A candidate for fixing #1593. It builds on top of #1615

Description

In #1593 it's mentioned that proc-macro-error is unmaintained for the
past few years, with no fix other than using proc-macro-error2 instead.

As on our scenario it's merely a transitive dependency of clap,
through clap_derive feature, which in latest releases doesn't depend on
proc-macro-error we can just bump it to latest.

It's valid to note that by bumping it, both examples that relies on clap
are no longer MSRV (1.63) compliant.

That said, this PR does:

  • Standardize the example packages to have example_ prefix.
  • Exclude examples from running in main Build & Test CI job.
  • Add new testing step to Build & Test Example CI job.
  • Bumps the clap to 4.5.17.

Notes to the reviewers

Changelog notice

  • Standardize the example packages to have example_ prefix.
  • Exclude examples from running in main Build & Test CI job.
  • Add new testing step to Build & Test Example CI job.
  • Bumps the clap to 4.5.17.

Checklists

All Submissions:

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

Bugfixes:

  • This pull request breaks the existing API
  • I've added tests to reproduce the issue which are now passing
  • I'm linking the issue being fixed by this PR

@oleonardolima
Copy link
Contributor Author

I also wonder if this could become even simpler by adopting Examples Cargo Target.

@oleonardolima oleonardolima force-pushed the fix/proc-macro-error-is-unmaintained branch from 89bdffc to c32967b Compare September 13, 2024 14:27
@oleonardolima
Copy link
Contributor Author

I think I also found a bug in how the bdk_esplora features were structured, it's somewhat depending on blocking feature being set up through the wallet_esplora example, and that's why it's failing in the CI when excluding the examples from the workspace.

@oleonardolima
Copy link
Contributor Author

I think I also found a bug in how the bdk_esplora features were structured, it's somewhat depending on blocking feature being set up through the wallet_esplora example, and that's why it's failing in the CI when excluding the examples from the workspace.

Also, it's something that we'd find out through #1018 if miniscript std/no-std weren't a blocker.

@oleonardolima oleonardolima force-pushed the fix/proc-macro-error-is-unmaintained branch from 51d8d64 to 8752555 Compare September 13, 2024 16:49
@oleonardolima
Copy link
Contributor Author

oleonardolima commented Sep 16, 2024 •
edited
Loading

I think I also found a bug in how the bdk_esplora features were structured, it's somewhat depending on blocking feature being set up through the wallet_esplora example, and that's why it's failing in the CI when excluding the examples from the workspace.

I was testing the fixes and got it in 8752555, but I'll move it to a separate PR to make it easier to review (#1615).

@oleonardolima oleonardolima mentioned this pull request Sep 16, 2024
Merged
8 tasks
@oleonardolima oleonardolima force-pushed the fix/proc-macro-error-is-unmaintained branch from 8752555 to 157d738 Compare September 16, 2024 16:06
@oleonardolima oleonardolima marked this pull request as ready for review September 16, 2024 16:08
@notmandatory notmandatory added dependencies Pull requests that update a dependency file ci labels Sep 17, 2024
@notmandatory notmandatory added this to the 1.0.0-beta milestone Sep 17, 2024
@ValuedMammal
Copy link
Contributor

Rather than rename the example-crates can't we use the "exclude" key in the workspace Cargo.toml to exclude them from invocations of cargo test --workspace?

https://doc.rust-lang.org/cargo/reference/workspaces.html#the-members-and-exclude-fields

@oleonardolima
Copy link
Contributor Author

https://doc.rust-lang.org/cargo/reference/workspaces.html#the-members-and-exclude-fields

I thought about it, but AFAICT it'll exclude from the workspace for any cargo command, and not only for the cargo test one.

ValuedMammal
ValuedMammal approved these changes Sep 19, 2024
View reviewed changes
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.

After considering the options I don't see any downside to using this solution.

@oleonardolima
fix(bdk_esplora): build with --no-default-features
33942ec 
- add `blocking-https` as one of the default features, instead of
  `blocking-https-rustls`, they are basically the same in
  `esplora-client`.
- add `async` and `blocking as required features for each test, using
  the `[[test]]` cargo target.
@oleonardolima
fix(bdk_electrum): build with --no-default-features
6e8f196 
- add `use-rustls` as required features for `test_electrum.rs`, using
  the `[[test]]` cargo target approach.
@oleonardolima oleonardolima force-pushed the fix/proc-macro-error-is-unmaintained branch from 157d738 to 2d43499 Compare September 20, 2024 00:51
@oleonardolima
Copy link
Contributor Author

After considering the options I don't see any downside to using this solution.

I rebased this one, as I updated the approach in #1615.

@oleonardolima
chore: use path as name for cargo test targets
9b7b195
@oleonardolima
chore(examples)!: update all examples to have example_ prefix
519728c
@oleonardolima
refactor(ci)!: update CI to build and test example in specific job
45be317
@oleonardolima
fix(RUSTSEC-2024-0370)!: bump clap to latest, removing transitive
d802d00 
dependency on `proc-macro-error`.

In bitcoindevkit#1593 it's mentioned that `proc-macro-error` is unmaintained for the
past few years, with no fix other than using proc-macro-error2 instead.

As on our scenario it's merely a transitive dependency of `clap`,
through `clap_derive` feature, which in latest releases doesn't depend on
`proc-macro-error` we can just bump it to latest.

It's valid to note that by bumping it, both examples that relies on clap
are no longer MSRV (1.63) compliant.
@oleonardolima oleonardolima force-pushed the fix/proc-macro-error-is-unmaintained branch from 2d43499 to d802d00 Compare September 20, 2024 16:54
ValuedMammal
ValuedMammal approved these changes Sep 27, 2024
View reviewed changes
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 d802d00

@ValuedMammal ValuedMammal merged commit 9e47b64 into bitcoindevkit:master Sep 28, 2024
21 checks passed
@oleonardolima oleonardolima deleted the fix/proc-macro-error-is-unmaintained branch September 28, 2024 17:41
This was referenced Oct 1, 2024
Closed
Closed
@notmandatory notmandatory changed the title fix(RUSTSEC-2024-0370): proc-macro-error is unmaintained ci: fix RUSTSEC-2024-0370 proc-macro-error is unmaintained Oct 2, 2024
@notmandatory notmandatory changed the title ci: fix RUSTSEC-2024-0370 proc-macro-error is unmaintained examples: fix RUSTSEC-2024-0370 proc-macro-error is unmaintained Oct 2, 2024
@notmandatory notmandatory changed the title examples: fix RUSTSEC-2024-0370 proc-macro-error is unmaintained chore: fix RUSTSEC-2024-0370 proc-macro-error is unmaintained Oct 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@notmandatory notmandatory notmandatory left review comments

@ValuedMammal ValuedMammal ValuedMammal approved these changes

Assignees

@oleonardolima oleonardolima

Labels
ci dependencies Pull requests that update a dependency file
Projects
BDK
Archived in project
Milestone
1.0.0-beta
Development

Successfully merging this pull request may close these issues.
3 participants
@oleonardolima @ValuedMammal @notmandatory