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

Replace ‘nightly_protocol_features’ with ‘nightly’ feature #6782

Merged
merged 13 commits into from
May 13, 2022

Conversation

mina86
Copy link
Contributor

@mina86 mina86 commented May 10, 2022

The ‘nightly_protocol’ and ‘nightly_protocol_features’ Cargo features
are a bit confusing to new and old developers alike. They two are
almost always paired together which raises the question why they are
separate features.

What’s worse, in test we often test for the ‘nightly_protocol’ feature
with the assumption that ‘nightly_protocol_features’ is enabled as
well. This may result in people trying to run tests with only the
former feature and getting bogus test failures.

But that’s not all. What makes situation even more confusing is that
‘nightly_protocol_features’ enables ‘nightly_protocol’ in most of the
Cargo configuration.

To simplify the most common case where both features are enabled,
rename ‘nightly_protocol_features’ to ‘nightly’ and change it so that
it always forces ‘nightly_protocol’ feature on.

The less common case where a single protocol feature is tested
(i.e. ’--features nightly_protocol,some_nightly_feature’) isn’t
affected at all.

The ‘nightly_protocol’ and ‘nightly_protocol_features’ Cargo features
are a bit confusing to new and old developers alike.  They two are
almost always paired together which raises the question why they are
separate features.

What’s worse, in test we often test for the ‘nightly_protocol’ feature
with the assumption that ‘nightly_protocol_features’ is enabled as
well.  This may result in people trying to run tests with only the
former feature and getting bogus test failures.

But that’s not all.  What makes situation even more confusing is that
‘nightly_protocol_features’ enables ‘nightly_protocol’ in most of the
Cargo configuration.

To simplify the most common case where both features are enabled,
rename ‘nightly_protocol_features’ to ‘nightly’ and change it so that
it always forces ‘nightly_protocol’ feature on.

The less common case where a single protocol feature is tested
(i.e. ’--features nightly_protocol,some_nightly_feature’) isn’t
affected at all.
@mina86 mina86 marked this pull request as ready for review May 10, 2022 23:56
@mina86 mina86 requested review from rtsai123, mhalambek and a team as code owners May 10, 2022 23:56
@mina86 mina86 requested a review from matklad May 10, 2022 23:56
Makefile Outdated Show resolved Hide resolved
nearcore/src/lib.rs Show resolved Hide resolved
neard/src/cli.rs Show resolved Hide resolved
@jakmeier
Copy link
Contributor

Sorry, I broke your code by adding more cfg in a recent PR. This one: https://github.com/near/nearcore/pull/6784/files#diff-94a2066d3f4b7aa8071b052bdbbdf0e8b90e77f79c46a3c1358d3c10e863511cR240

@near-bulldozer near-bulldozer bot merged commit 6195773 into master May 13, 2022
@near-bulldozer near-bulldozer bot deleted the mpn-night branch May 13, 2022 15:09
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.

4 participants