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

Feature-gate the executable auto-download #102

Merged
merged 2 commits into from
Jan 8, 2023
Merged

Conversation

RCasatta
Copy link
Collaborator

@RCasatta RCasatta commented Jan 5, 2023

This way all the build-deps are not used when auto-download is not required granting a MSRV of 1.41.1

While 1.57 is required if auto-download feature is used

closes #88

could help with #100 and #95

This way all the build-deps are not used when auto-download is not required
granting a MSRV of 1.41.1
src/lib.rs Outdated
@@ -294,6 +294,8 @@ impl BitcoinD {
p2p_args,
conf_args
);

#[allow(clippy::needless_borrow)] // would break 1.41.1 if fixed
Copy link

Choose a reason for hiding this comment

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

FYI you can configure clippy to know about your MSRV and not suggest wrong things.

Copy link

@MaxFangX MaxFangX left a comment

Choose a reason for hiding this comment

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

tACK - Didn't squint too hard at the changes in build.rs, but I ran this patch and can confirm this PR fixes the dependency resolution issue described in #100, and also that all of our integration tests continue to work as expected. Can mark this PR as closing #100 once merged.

@RCasatta RCasatta merged commit fed768c into master Jan 8, 2023
@DanGould
Copy link

DanGould commented Jan 8, 2023

Seems like the right kind of thing to me. Very glad to see this addressed!

@DanGould
Copy link

DanGould commented Jan 8, 2023

MSRV seems impossibly confusing to maintain.

when I cargo msrv on this branch I get 1.54, which isn't even what you mention as required for all the features. Any clue what i'm doing wrong or how to figure out what is making the MSRV tick higher in my environment?

@RCasatta
Copy link
Collaborator Author

did Kixunil/payjoin#40 (comment) fix your issue?

@DanGould
Copy link

Not yet because I'm not sure how to figure out which particular dependency is causing MSRV to be higher than I'd like.

@Kixunil
Copy link

Kixunil commented Jan 19, 2023

I do it by trial-error approach. Just try to compile it and whichever returns error I check the changelog and downgrade. It's a bit tedious but not too terrible.

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.

Make build-dep optional if not needed
4 participants