Skip to content

make every version imply the one before it#117

Merged
RCasatta merged 1 commit intorust-bitcoin:masterfrom
apoelstra:2023-04--versions
Apr 5, 2023
Merged

make every version imply the one before it#117
RCasatta merged 1 commit intorust-bitcoin:masterfrom
apoelstra:2023-04--versions

Conversation

@apoelstra
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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 apoelstra force-pushed the 2023-04--versions branch from a5e6533 to 925d276 Compare April 4, 2023 20:34
@apoelstra
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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

@apoelstra apoelstra force-pushed the 2023-04--versions branch from c0050e0 to 5eeb733 Compare April 4, 2023 22:50
@RCasatta
Copy link
Copy Markdown
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 apoelstra force-pushed the 2023-04--versions branch from 5eeb733 to c06ee2f Compare April 5, 2023 13:14
@apoelstra
Copy link
Copy Markdown
Member Author

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

@RCasatta
Copy link
Copy Markdown
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
Copy Markdown
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