Add special feature for inter-crate safe dev-related code reuse #32169

ryoqun · 2023-06-16T05:20:44Z

Problem

I'm tired of waiting rust-lang/cargo#8379 being fixed. :)

joking aside, there's so many code in our monorepo, which shouldn't be used in production code, but made pub only for the sake of usage in dependent crate's test/bench purposes.

when reviewing prs which touch such a method, it's too laborious to verify there's no misuse.

... and our social contract of naming things as for_test(s) and be careful in general doesn't work always: #32424

also, #[deprecated] won't work: the code should still be usable for test code; and sparkling #[allow(deprecated)]s at each and every call site would be too laborious.

Summary of Changes

Use a well-known work around: rust-lang/cargo#8379 (comment)

The caveat is that some special treatment must be taken due to cargo's feature unification. so I added cargo hack tricks in test-checks.sh.

the downside is that ci takes longer.... (don't scared of this pr's build time; this is mainly due to unpopulated target dir...; usually should take +~5 mins)

lastly, this feature makes call-hierarchy-based auditing easier, binaries smaller, and last-not-least, humans lazier. lol

examples of `dev-context-only-utils` usage

dcou: BorrowedAccount::set_data() #32424

new pipeline example

naming is finalized; please open follow up pr if you insist

below is kept for historical reference

Naming

dev: too short?

dev-utils: uncommon phrase; inspired from dev-dependencies and they're closely related. not used in monorepo (i think introducing a new terminology is good)

test-utils: common phrase; sounds bench are excluded. already used in monorepo as mod name.. (i think this is bad because of some human's cognitive ambiguity.)

non-essential/not-production: a bit too abstract?

dev-only/for-dev: 2nd candidate?

dev-profile-only-utils: dev-profile implies -Copt-level=0 and etc for me (ref: https://doc.rust-lang.org/cargo/reference/profiles.html#dev). that's not true for benches and cargo test --release (dev-utils could be valid use cases for these)

test-in-workspace: very explicit name for the purpose of the feature. this is out of scope thinking but this new feature flag can theoretically be used by third-party downstream crates outside the monorepo. so better off avoiding workspace? btw, Add new execution code-path for unified scheduler #31239 is using this.

*-helper?: imo, helper sounds some wrapper and less generic utils. we're planning to feature gate any dev related code under the new feature.

sol{,ana}?: needs proper justification.

use-*/export-*: i think these are redundant.

{dev/test}-code?: this as well redundant.

dev-pub: too gimmick?

utils vs util?

seems utils is popular?

fixes: #32170
fixes: #32171
fixes: #32246
fixes: #32247
fixes: #32248
fixes: #32249
fixes: #32344
fixes: #32345

codecov · 2023-06-16T07:49:06Z

Codecov Report

Merging #32169 (be5a110) into master (689ca50) will decrease coverage by 0.1%.
The diff coverage is n/a.

@@            Coverage Diff            @@
##           master   #32169     +/-   ##
=========================================
- Coverage    82.0%    82.0%   -0.1%     
=========================================
  Files         770      770             
  Lines      209236   209236             
=========================================
- Hits       171679   171627     -52     
- Misses      37557    37609     +52

t-nelson · 2023-06-16T16:15:17Z

looks like cargo-hack is currently trailing latest cargo by ten releases. any concerns with adding a tooling dependency they may break on us?

ryoqun · 2023-06-17T04:29:08Z

looks like cargo-hack is currently trailing latest cargo by ten releases.

hmm, how do you think so? maybe, you saw MSRV at https://crates.io/crates/cargo-hack (currently 1.60)? it's just build dependency of cargo-hack itself for minimum rust version.

it's released periodically: https://github.com/taiki-e/cargo-hack/releases

any concerns with adding a tooling dependency they may break on us?

i think there's little concern. cargo-hack is a thin wrapper for cargo. it's just reading rather stable Cargo.toml and spawning actual cargo subcommands for ease.

t-nelson · 2023-06-21T16:28:36Z

maybe, you saw MSRV

ah yeah, didn't realize that msrv was so prominent on crates.io nowadays. i probably just saw the first version-looking string and went with it 🙃

t-nelson · 2023-06-21T16:49:10Z

lol... run cargo hack check --help in the root of the monorepo 🙃

t-nelson

i'm generally supportive of this change for the sake of eliminating test-only logic from crate public apis. in order to fully take advantage, we'll need to replace all instances of cargo {test,check} with the cargo-hack-wrapped versions, right?

ci/test-checks.sh

ci/buildkite-pipeline.sh

ryoqun · 2023-06-23T06:17:15Z

we'll need to replace all instances of cargo {test,check} with the cargo-hack-wrapped versions, right?

@t-nelson nope. as this pr introduced another set of cargo hack check invocations, there should be no additional benefit of adjusting existing cargo invocations. And cargo hack test will take too long...

ryoqun · 2023-06-23T06:22:54Z

oookay. I've finally finished up this pr. ready for review. also, I'll create bunch of test prs to test this pr.

ryoqun · 2023-06-23T06:24:02Z

scripts/check-dev-utils.sh

unexpected from me? lol I've done some dense commenting work. :)

ryoqun · 2023-06-23T06:26:34Z

scripts/check-dev-utils.sh

+# binaries.
+# Note also that dev-utils-ci-marker feature must be added and all of its
+# dependencies should be edited likewise if any.
+declare dev_util_tainted_packages=(


currently empty; i'll add some with a follow up pr.

ryoqun · 2023-06-23T06:28:45Z

scripts/check-dev-utils.sh

+
+mode=${1:-full}
+
+if [[ $mode = "tree" || $mode = "full" ]]; then


hehe, I added this set of new checks after i drafted this pr.

ryoqun · 2023-06-23T06:29:18Z

scripts/check-dev-utils.sh

+declare dev_util_tainted_packages=(
+)
+
+mode=${1:-full}


the default full is intended for local development use.

ryoqun · 2023-06-23T08:36:32Z

also, I'll create bunch of test prs to test this pr.

phew, ci abusing time (^o^)/

I've prepared 6 scenarios and all results are expected.

(EDIT: these are re-run; see #32169 (comment))

`1a`: #32170

simplest case: some code is newly dev-utils-gated; but some affected crates aren't updated to activate dev-utils.

note that only ./check-dev-utils.sh check-all-targets is failing as expected, justifying its existence.
that means existing cargo check is lying due to feature unification.

`1b`: #32171

1a is fixed with eb76acd with additional dev-utils activation for the rest of needed crates.

`2a`: #32246

enabling dev-utils as part of normal dependencies is forbidden

note that only ./check-dev-utils.sh tree's first cargo tree is failing as expected, justifying its existence.

`2b`: #32247

2a is fixed with fabfe49 with correct white listing

`2c`: #32248

2b was made to fail yet again with new bad dependent crate on the whitelisted crate.
to make it pass, it's needed to contaminate any subtrees for dev-utils-as-normal-deps to damage control accidental dev-utils activation via feature unification.

note that only ./check-dev-utils.sh tree's second cargo tree is failing as expected, justifying its existence.

`2d`: #32249

2c is fixed with 0cd0402 with correct tainting.

`2e`: #32344

2d was made to fail yet yet again with 2a variant: the tainted leaf crate is whitelisted in check-dev-utils.sh but not marked as such its Cargo.toml.

`2f`: #32345

2e is fixed with cc93fb8 with correct flagging with dev-utils.

conclution

all in all, i think that almost all of dev-utils careless misuses will be detected while its usability and utility are upheld

ryoqun · 2023-06-30T14:09:03Z

well, i spotted a bug and fixed 652df19

good new is that the marker feature is gone. also, i added ci scenarios at #32169 (comment)

t-nelson

hmm, dev-profile implies -Copt-level=0 and etc for me (ref: https://doc.rust-lang.org/cargo/reference/profiles.html#dev). that's not true for benches and cargo test --release (dev-utils could be valid use cases for these)

haha... dammit. i want a general word for this context

scripts/check-dev-utils.sh

t-nelson · 2023-07-01T01:03:06Z

haha... dammit. i want a general word for this context

oh durr... dev-context-only-utils ?

ryoqun · 2023-07-01T05:18:11Z

haha... dammit. i want a general word for this context

oh durr... dev-context-only-utils ?

thanks for trying hard to give this baby a proper name. :) but, allow me to defend dev-utils a bit...

I wonder the -context-only- could convey the subtle implication of what you wanted to express. and the name is longer (considerably, imo). both context and only are generic in their meaning. so, i couldn't derive some valuable information from those words. say, serde-only vs just serde and alloc-context-only-utils and alloc-utils.

also, as we're recognizing usage of dev-utils as normal dep for benches, i'm not sure about the merit of indicating that context so vocally in its name... for that matter, dev-utils is defined and used just like normal features like serdes. and all of features including dev-utils are govenred in the same rule like no-propagation under dev dep. usage...

lastly, we have (hopefully) proper ci guard against misuse.

t-nelson · 2023-07-03T18:07:34Z

the main reason i don't believe dev-utils to be sufficient is i suspect that most people will follow logic along the lines of "dev-utils. i'm a dev. this is a utility. this needs dev-utils", then get hammered by CI and now we have to support them on discord. given that rust folks have been zero help in giving us a nice concise name, the only sane alternative is to trigger a "wtf is this?" to (hopefully!) prompt a docs search upon first exposure

ryoqun · 2023-07-04T01:17:56Z

the main reason i don't believe dev-utils to be sufficient is i suspect that most people will follow logic along the lines of "dev-utils. i'm a dev. this is a utility. this needs dev-utils", then get hammered by CI and now we have to support them on discord. given that rust folks have been zero help in giving us a nice concise name, the only sane alternative is to trigger a "wtf is this?" to (hopefully!) prompt a docs search upon first exposure

@t-nelson thanks for clarification. I'm persuaded and giving up shorter name: 6e6d189

requesting hopefully final review. :) Also, I'll update the ci scenarios prs shortly after :) (hint: i wonder it can make it to ship this today before US holiday...)

t-nelson

woot! let's give it a shot

ryoqun changed the title ~~Dev utils~~ Add workspace feature (dev-utils) to avoid accidental test code usage Jun 16, 2023

ryoqun changed the title ~~Add workspace feature (dev-utils) to avoid accidental test code usage~~ Add common feature (dev-utils) to avoid accidental test code usage Jun 16, 2023

ryoqun changed the title ~~Add common feature (dev-utils) to avoid accidental test code usage~~ Add common feature dev-utils to avoid accidental test code usage Jun 16, 2023

ryoqun changed the title ~~Add common feature dev-utils to avoid accidental test code usage~~ Add workspace-wide feature dev-utils to avoid test code misuse Jun 16, 2023

ryoqun changed the title ~~Add workspace-wide feature dev-utils to avoid test code misuse~~ Add common feature dev-utils to avoid test code misuse Jun 16, 2023

ryoqun referenced this pull request Jun 16, 2023

revert

72919fc

ryoqun changed the title ~~Add common feature dev-utils to avoid test code misuse~~ [wip & rfc] Add common feature dev-utils to avoid test code misuse Jun 16, 2023

ryoqun changed the title ~~[wip & rfc] Add common feature dev-utils to avoid test code misuse~~ [rfc] Add common feature dev-utils to avoid test code misuse Jun 16, 2023

ryoqun mentioned this pull request Jun 16, 2023

Dev utils ci failure #32170

Closed

CriesofCarrots mentioned this pull request Jun 21, 2023

[zk-token-sdk] Make ElGamalKeypair fields private #32190

Merged

t-nelson reviewed Jun 21, 2023

View reviewed changes

ci/test-checks.sh Outdated Show resolved Hide resolved

ci/test-checks.sh Outdated Show resolved Hide resolved

ci/test-checks.sh Outdated Show resolved Hide resolved

ci/buildkite-pipeline.sh Outdated Show resolved Hide resolved

t-nelson requested a review from yihau June 21, 2023 22:29

ryoqun mentioned this pull request Jun 22, 2023

Install cargo-hack for upcoming dev-utils #32234

Merged

ryoqun changed the title ~~[rfc] Add common feature dev-utils to avoid test code misuse~~ Add dev-utils feature for inter-crate test code safe reuse Jun 23, 2023

Add dev-utils feature for inter-crate test code safe reuse

41cca66

ryoqun force-pushed the dev-utils branch from eff2c65 to 41cca66 Compare June 23, 2023 06:10

ryoqun marked this pull request as ready for review June 23, 2023 06:12

ryoqun requested a review from t-nelson June 23, 2023 06:21

ryoqun commented Jun 23, 2023

View reviewed changes

scripts/check-dev-utils.sh Outdated

Copy link

Contributor Author

ryoqun Jun 23, 2023

Choose a reason for hiding this comment

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

unexpected from me? lol I've done some dense commenting work. :)

ryoqun commented Jun 23, 2023

View reviewed changes

Improve jq query and remove uneeded marker feature

652df19

ryoqun changed the title ~~Add dev-utils feature for inter-crate test code safe reuse~~ Add dev-utils feature for inter-crate safe dev-related code reuse Jun 30, 2023

ryoqun changed the title ~~Add dev-utils feature for inter-crate safe dev-related code reuse~~ Add dev-utils feat. for inter-crate safe dev-related code reuse Jun 30, 2023

t-nelson reviewed Jul 1, 2023

View reviewed changes

Use folding heredoc giving up proper indenting..

58013c2

ryoqun requested a review from t-nelson July 1, 2023 05:26

Use jq's alternative operator (//)

fd2d6cf

ryoqun force-pushed the dev-utils branch from bc215ae to fd2d6cf Compare July 1, 2023 05:39

This was referenced Jul 1, 2023

Dev utils ci failure4 #32344

Closed

Dev utils ci success4 #32345

Closed

Rename to dev-context-only-utils

6e6d189

ryoqun force-pushed the dev-utils branch from bf92965 to 6e6d189 Compare July 4, 2023 01:06

ryoqun changed the title ~~Add dev-utils feat. for inter-crate safe dev-related code reuse~~ Add common feature for inter-crate safe dev-related code reuse Jul 4, 2023

ryoqun changed the title ~~Add common feature for inter-crate safe dev-related code reuse~~ Add special feature for inter-crate safe dev-related code reuse Jul 4, 2023

Fix typo...

be5a110

t-nelson approved these changes Jul 6, 2023

View reviewed changes

ryoqun merged commit 1824743 into solana-labs:master Jul 7, 2023

This was referenced Jul 8, 2023

dcou: BorrowedAccount::set_data() #32424

Merged

dcou: SlotMeta::unset_parent() #32427

Merged

dcou: mod banking_trace::for_test #32428

Merged

t-nelson mentioned this pull request Jul 13, 2023

deprecates Signature::new in favor of Signature::{try_,}from #32481

Merged

This was referenced Aug 4, 2023

Moves accounts-db test-only code into a dev-context-only-utils feature #32718

Closed

Moves some accounts-db test-only code into a dev-context-only-utils feature #32748

Merged

ryoqun mentioned this pull request Aug 12, 2023

dcou: make internal function pub conditionally #32822

Merged

ryoqun mentioned this pull request Jan 11, 2024

Make check{2,3} faster by enabling parallel rustc for dcou #34739

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add special feature for inter-crate safe dev-related code reuse #32169

Add special feature for inter-crate safe dev-related code reuse #32169

ryoqun commented Jun 16, 2023 •

edited

Loading

codecov bot commented Jun 16, 2023 •

edited

Loading

t-nelson commented Jun 16, 2023

ryoqun commented Jun 17, 2023

t-nelson commented Jun 21, 2023

t-nelson commented Jun 21, 2023

t-nelson left a comment

ryoqun commented Jun 23, 2023

ryoqun commented Jun 23, 2023

ryoqun Jun 23, 2023

ryoqun Jun 23, 2023

ryoqun Jun 23, 2023

ryoqun Jun 23, 2023

ryoqun commented Jun 23, 2023 •

edited

Loading

ryoqun commented Jun 30, 2023 •

edited

Loading

t-nelson left a comment

t-nelson commented Jul 1, 2023

ryoqun commented Jul 1, 2023 •

edited

Loading

t-nelson commented Jul 3, 2023

ryoqun commented Jul 4, 2023

t-nelson left a comment


		mode=${1:-full}

		if [[ $mode = "tree" \|\| $mode = "full" ]]; then

Add special feature for inter-crate safe dev-related code reuse #32169

Add special feature for inter-crate safe dev-related code reuse #32169

Conversation

ryoqun commented Jun 16, 2023 • edited Loading

Problem

Summary of Changes

examples of dev-context-only-utils usage

new pipeline example

Naming

utils vs util?

codecov bot commented Jun 16, 2023 • edited Loading

Codecov Report

t-nelson commented Jun 16, 2023

ryoqun commented Jun 17, 2023

t-nelson commented Jun 21, 2023

t-nelson commented Jun 21, 2023

t-nelson left a comment

Choose a reason for hiding this comment

ryoqun commented Jun 23, 2023

ryoqun commented Jun 23, 2023

ryoqun Jun 23, 2023

Choose a reason for hiding this comment

ryoqun Jun 23, 2023

Choose a reason for hiding this comment

ryoqun Jun 23, 2023

Choose a reason for hiding this comment

ryoqun Jun 23, 2023

Choose a reason for hiding this comment

ryoqun commented Jun 23, 2023 • edited Loading

1a: #32170

1b: #32171

2a: #32246

2b: #32247

2c: #32248

2d: #32249

2e: #32344

2f: #32345

conclution

ryoqun commented Jun 30, 2023 • edited Loading

t-nelson left a comment

Choose a reason for hiding this comment

t-nelson commented Jul 1, 2023

ryoqun commented Jul 1, 2023 • edited Loading

t-nelson commented Jul 3, 2023

ryoqun commented Jul 4, 2023

t-nelson left a comment

Choose a reason for hiding this comment

ryoqun commented Jun 16, 2023 •

edited

Loading

examples of `dev-context-only-utils` usage

codecov bot commented Jun 16, 2023 •

edited

Loading

ryoqun commented Jun 23, 2023 •

edited

Loading

`1a`: #32170

`1b`: #32171

`2a`: #32246

`2b`: #32247

`2c`: #32248

`2d`: #32249

`2e`: #32344

`2f`: #32345

ryoqun commented Jun 30, 2023 •

edited

Loading

ryoqun commented Jul 1, 2023 •

edited

Loading