Skip to content

split bin and lib targets of solana-faucet out to separate crates #6326

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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

t-nelson
Copy link

Problem

dependency version pinning for solana-faucet bin target unnecessarily restricts dependency resolution for the lib target

Summary of Changes

  • rename solana-faucet to solana-faucet-interface (ala sdk naming convention)
  • factor out solana-faucet bin target to it's own crate (solana-faucet-bin)
  • relax solana-faucet-interface dependency specifications

TODO: one time publish solana-faucet as a stub crate with guidance for migrating to new crates

@t-nelson t-nelson requested review from joncinque and yihau May 27, 2025 18:48
Copy link

mergify bot commented May 27, 2025

If this PR represents a change to the public RPC API:

  1. Make sure it includes a complementary update to rpc-client/ (example)
  2. Open a follow-up PR to update the JavaScript client @solana/kit (example)

Thank you for keeping the RPC clients in sync with the server API @t-nelson.

@t-nelson
Copy link
Author

t-nelson commented May 27, 2025

TODO: one time publish solana-faucet as a stub crate with guidance for migrating to new crates

how do we feel about doing this? i don't think either target from this crate is technically public api

@t-nelson t-nelson force-pushed the extract-faucet-bin branch 3 times, most recently from 665487f to f39a16c Compare May 27, 2025 21:57
@t-nelson t-nelson requested a review from steviez May 27, 2025 22:38
@codecov-commenter
Copy link

codecov-commenter commented May 27, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 82.8%. Comparing base (d4b2b25) to head (64419b8).

Additional details and impacted files
@@            Coverage Diff            @@
##           master    #6326     +/-   ##
=========================================
- Coverage    82.8%    82.8%   -0.1%     
=========================================
  Files         848      848             
  Lines      379050   379050             
=========================================
- Hits       314049   314027     -22     
- Misses      65001    65023     +22     
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@t-nelson t-nelson marked this pull request as draft May 28, 2025 00:19
@t-nelson
Copy link
Author

t-nelson commented May 28, 2025

having cold feet about the solana-faucet-interface crate. think i'll just break the bin instead

@t-nelson
Copy link
Author

having cold feet about the solana-faucet-interface crate. think i'll just break the bin instead

n/m there doesn't seem to be a clean way to omit a single crate target from the workspace without also breaking cargo install

@t-nelson t-nelson marked this pull request as ready for review May 28, 2025 04:17
@t-nelson
Copy link
Author

confirmed that ./scripts/cargo-install-all.sh still includes the solana-faucet bin

Copy link

@joncinque joncinque left a comment

Choose a reason for hiding this comment

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

The changes look good to me!

Just on naming, I'd be inclined to keep solana-faucet as the library to avoid breaking users, and make solana-faucet-cli or solana-faucet-server as the binary.

On the other hand, there aren't too many direct users of the crate, and I think we know them all personally: https://crates.io/crates/solana-faucet/reverse_dependencies

The only info about best practices I could find for a bin+lib was a forum answer: https://users.rust-lang.org/t/publishing-crate-with-lib-and-binary/124519/2

@vadorovsky vadorovsky changed the title split bin and lib targets of solana-fauct out to separate crates split bin and lib targets of solana-faucet out to separate crates Jun 2, 2025
@t-nelson
Copy link
Author

t-nelson commented Jun 4, 2025

The changes look good to me!

Just on naming, I'd be inclined to keep solana-faucet as the library to avoid breaking users,

that's what i wanted to do, but then we can't stub the bin to offer migration guidance for anyone attempting to cargo install it. renaming both seemes like the only alternative

and make solana-faucet-cli or solana-faucet-server as the binary.

i saw both -bin and -cli on crates.io so just yolo'd. happy to switch to -cli. we should make this choice and stick with it 'cause i have more of these to break out and i'd rather be consistent

On the other hand, there aren't too many direct users of the crate, and I think we know them all personally: https://crates.io/crates/solana-faucet/reverse_dependencies

seems like only two are even using modern crates

The only info about best practices I could find for a bin+lib was a forum answer: https://users.rust-lang.org/t/publishing-crate-with-lib-and-binary/124519/2

@t-nelson
Copy link
Author

t-nelson commented Jun 4, 2025

happy to switch to -cli

i went ahead and reserved solana-faucet-cli

@t-nelson t-nelson force-pushed the extract-faucet-bin branch from f39a16c to 64419b8 Compare June 4, 2025 03:01
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.

3 participants