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

make every version imply the one before it #117

Merged
merged 1 commit into from
Apr 5, 2023

Conversation

apoelstra
Copy link
Member

This allows the library to be compiled with any combination of versions of Bitcoin Core -- it will just use the highest one.

This won't guarantee compatibility because Bitcoin Core sometimes makes backward-compatibility breaking changes ... but it is much better than the current situation where if you specify multiple feature flags, the library won't compile at all.

@apoelstra
Copy link
Member Author

I'm confused by these errors -- it seems that I've broken the version detection somehow, but I don't understand how.

@RCasatta
Copy link
Collaborator

RCasatta commented Apr 4, 2023

After staring at versions the last 10 minutes I think I get it, I think the logic in versions.rs should be version_x, not version_x+1
but we have version_x, not version_x-1

@apoelstra
Copy link
Member Author

Ah! Yep, good catch!

Also I made some typos in versions.rs.

@apoelstra apoelstra force-pushed the 2023-04--versions branch 2 times, most recently from 71e8bb8 to c0050e0 Compare April 4, 2023 20:50
src/versions.rs Outdated
pub const VERSION: &str = "0.21.0";

#[cfg(feature = "0_20_1")]
#[cfg(all(feature = "0_20_1", not(feature = "0_20_1")))]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
#[cfg(all(feature = "0_20_1", not(feature = "0_20_1")))]
#[cfg(all(feature = "0_20_1", not(feature = "0_21_0")))]

src/versions.rs Outdated

#[cfg(not(feature = "0_17_1"))]
pub const VERSION: &str = "N/A";

Copy link
Collaborator

Choose a reason for hiding this comment

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

cargo fmt doesn't like this empty line and it's enforced in ci

@RCasatta
Copy link
Collaborator

RCasatta commented Apr 5, 2023

could you please squash this last fix for mac 66a5d09

@RCasatta RCasatta mentioned this pull request Apr 5, 2023
This allows the library to be compiled with any combination of versions
of Bitcoin Core -- it will just use the highest one.

This won't guarantee compatibility because Bitcoin Core sometimes makes
backward-compatibility breaking changes ... but it is much better than
the current situation where if you specify multiple feature flags, the
library won't compile at all.
@apoelstra
Copy link
Member Author

Done. Thank you!! Sorry for so many iterations.

@RCasatta
Copy link
Collaborator

RCasatta commented Apr 5, 2023

ACK c06ee2f

Thank you for this great idea to make features non-exclusive!

Going to release soon

@RCasatta RCasatta merged commit d6ff21f into rust-bitcoin:master Apr 5, 2023
@apoelstra apoelstra deleted the 2023-04--versions branch April 5, 2023 13:32
@apoelstra
Copy link
Member Author

Hooray! Thanks for accepting. Once we release I'll go try to update elementsd to use this new release, and a similar idea (which is actually what I'm trying to do here :P)

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