Skip to content
This repository was archived by the owner on Jan 22, 2025. It is now read-only.

chore: workspace inheritance #29509

Merged
merged 8 commits into from
Jan 25, 2023
Merged

Conversation

yihau
Copy link
Contributor

@yihau yihau commented Jan 4, 2023

workspace.package

  • Cargo.toml
  • programs/sbf/Cargo.toml

workspace.dependencies

  • Cargo.toml
  • programs/sbf/Cargo.toml

btw, I merge some dependencies's version. if A use foo = "1" and B use foo = "1.10.1", I move foo = "1.10.1" to workspace and both A and B inherit it.
there are some dependencies's version are different. I will make other PRs to union them.

scripts

  • get version from root Cargo.toml
  • increasing cargo version for root Cargo.toml as well
  • ignore some checking when version = { workspace = true }

@github-actions github-actions bot added the web3.js Related to the JavaScript client label Jan 4, 2023
@yihau yihau force-pushed the cargo-inheritance branch from 9e85e0d to 296f20b Compare January 4, 2023 19:55
@github-actions github-actions bot removed the web3.js Related to the JavaScript client label Jan 4, 2023
@yihau yihau force-pushed the cargo-inheritance branch 4 times, most recently from 2871b0a to e8f914b Compare January 5, 2023 10:58
@yihau yihau marked this pull request as ready for review January 5, 2023 13:36
@yihau yihau force-pushed the cargo-inheritance branch 4 times, most recently from 07e1561 to a8e18ed Compare January 6, 2023 14:09
@steviez
Copy link
Contributor

steviez commented Jan 6, 2023

Thanks for doing this! I took a crack at this a few months ago, but ran into a CI issue that held things up before the PR fell off my radar. Nice to have a single point to change on all these deps, definitely simplifies operations that need to be done manually / shrinks diffs from running scripts.

A question from the old review that came up that I'll post here in case anyone else might have the same question:

On cargo publish, does cargo do the right thing and rewrite the Cargo.toml to not depend on the workspace file?

The answer is yes, and I confirmed it by running the following and inspecting the generated Cargo.toml file.

$ cargo publish --dry-run --package solana-logger

@willhickey
Copy link
Contributor

Do you plan on backporting this to v1.14? I'm running some tests (all good so far) but my attempt to cherry pick to v1.14 ran into a bunch of trivial conflicts. Just wondering if I should take the time to test on v1.14.

@mvines
Copy link
Contributor

mvines commented Jan 6, 2023

No 1.14 backport please, too much churn for a stable branch

@steviez steviez self-requested a review January 9, 2023 06:00
@yihau
Copy link
Contributor Author

yihau commented Jan 9, 2023

downstream project failed due to https://www.github.com/hyperium/hyper/issues/3045
I'm thinking upgrade hyper 0.14.20 => 0.14.23 and socket2 0.4.4 => 0.4.7 to solve the issue.

@steviez
Copy link
Contributor

steviez commented Jan 9, 2023

I'm thinking upgrade hyper 0.4.20 => 0.4.23 and socket2 0.4.4 => 0.4.7 to solve the issue.

Seems reasonable to me; think it makes sense to do those bumps in separate PR's like the other dependency bump PR's I see you've opened recently

@yihau yihau force-pushed the cargo-inheritance branch 3 times, most recently from 4f3fed1 to 5a20d5e Compare January 13, 2023 13:29
@yihau yihau force-pushed the cargo-inheritance branch 2 times, most recently from 9e3abe9 to 5397dd5 Compare January 23, 2023 03:30
@yihau yihau force-pushed the cargo-inheritance branch from 5397dd5 to 53827db Compare January 23, 2023 03:57
Copy link
Contributor

@steviez steviez left a comment

Choose a reason for hiding this comment

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

This is a large diff so not going to pretend I went through every line. Instead, the checks I performed:

  • Ran cargo tree on current tip of master and the PR branch and compared output; there was no difference
  • When I took a crack at this a while back, I tested a concern Michael had about the correct version getting populated in individual crates. I had tested that and tested again with no issue.
    • I tested by running cargo publish --dry-run --package solana-logger and inspecting /target/package/solana-logger-1.15.0/Cargo.toml

@t-nelson
Copy link
Contributor

t-nelson commented Jan 24, 2023

This is a large diff so not going to pretend I went through every line. Instead, the checks I performed:

  • Ran cargo tree on current tip of master and the PR branch and compared output; there was no difference
  • When I took a crack at this a while back, I tested a concern Michael had about the correct version getting populated in individual crates. I had tested that and tested again with no issue.
    • I tested by running cargo publish --dry-run --package solana-logger and inspecting /target/package/solana-logger-1.15.0/Cargo.toml

seems sufficiently rigorous to me! :shipit:

@yihau yihau merged commit a67d239 into solana-labs:master Jan 25, 2023
@yihau yihau deleted the cargo-inheritance branch January 25, 2023 06:00
@yihau yihau restored the cargo-inheritance branch January 25, 2023 07:46
yihau added a commit to yihau/solana that referenced this pull request Jan 25, 2023
yihau added a commit that referenced this pull request Jan 25, 2023
@mergify
Copy link
Contributor

mergify bot commented Jan 25, 2023

⚠️ The sha of the head commit of this PR conflicts with #29893. Mergify cannot evaluate rules on this PR. ⚠️

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants