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

[Merged by Bors] - Split common crates out into their own repos #3890

Closed
wants to merge 14 commits into from

Conversation

michaelsproul
Copy link
Member

@michaelsproul michaelsproul commented Jan 16, 2023

Proposed Changes

Split out several crates which now exist in separate repos under sigp.

For the published crates see: https://crates.io/teams/github:sigp:crates-io?sort=recent-updates.

Additional Info

  • Need to work out how to handle versioning. I was hoping to do 1.0 versions of several crates, but if they depend on ethereum-types 0.x that is not going to work. EDIT: decided to go with 0.5.x versions.
  • Need to port several changes from tree-states, capella, eip4844 branches to the external repos.

@michaelsproul michaelsproul added work-in-progress PR is a work-in-progress v3.5.0 Minor release circa Q1 2023 labels Jan 16, 2023
@michaelsproul michaelsproul changed the base branch from stable to unstable January 17, 2023 01:15
michaelsproul added a commit that referenced this pull request Jan 17, 2023
Squashed commit of the following:

commit 1ba4f80
Author: Michael Sproul <michael@sigmaprime.io>
Date:   Tue Jan 17 11:43:18 2023 +1100

    Bye 1.0.0 beta, hello 0.5.x

commit a862b23
Author: Michael Sproul <michael@sigmaprime.io>
Date:   Tue Jan 17 10:54:46 2023 +1100

    Cargo fmt

commit e29f358
Author: Michael Sproul <michael@sigmaprime.io>
Date:   Mon Jan 16 18:21:42 2023 +1100

    It compiles :O

commit 1ee4514
Author: Michael Sproul <michael@sigmaprime.io>
Date:   Mon Jan 16 17:27:10 2023 +1100

    Ethereum hashing

commit 69bdd1d
Author: Michael Sproul <michael@sigmaprime.io>
Date:   Mon Jan 16 17:24:58 2023 +1100

    Tree hash et al

commit 7cae5d9
Author: Michael Sproul <michael@sigmaprime.io>
Date:   Mon Jan 16 17:21:03 2023 +1100

    Delete crates!

commit dd9ee38
Author: Michael Sproul <michael@sigmaprime.io>
Date:   Mon Jan 16 17:19:19 2023 +1100

    Delete overrides

commit 0d54534
Author: Michael Sproul <michael@sigmaprime.io>
Date:   Mon Jan 16 17:19:04 2023 +1100

    Crate renames
@michaelsproul michaelsproul removed the v3.5.0 Minor release circa Q1 2023 label Feb 10, 2023
@michaelsproul michaelsproul added the v4.1.0 Post-Capella minor release label Mar 17, 2023
@michaelsproul michaelsproul added major-task A significant amount of work or conceptual task. ready-for-review The code is ready for review and removed work-in-progress PR is a work-in-progress labels Mar 30, 2023
@michaelsproul
Copy link
Member Author

This is ready for review by a brave soul.

To verify the contents of the external repos, one could try copying the current crate contents from unstable to the right spot in each external repo, and then looking at the diff. The diff should show only renames, minor changes and no backdoors 😉

@michaelsproul
Copy link
Member Author

Will batch this actually

bors r-
bors r+

@michaelsproul
Copy link
Member Author

Shit! Wrong PR

bors r-

@bors
Copy link

bors bot commented Mar 30, 2023

Canceled.

@michaelsproul
Copy link
Member Author

Another one, in the tree_hash repo, it uses the old ethereum_ssz 1.0.0-beta versions in dependencies, but it looks like you decided to not do 1.0, should we "update" to 0.5.0 to match Lighthouse?

Good catch yeah! It's not too impactful because they're dev-dependencies, so we're not pulling 1.0.0-beta into Lightouse (it would show in the Cargo.lock). I agree we should yank them and remove the releases to avoid confusion

Copy link
Member

@jimmygchen jimmygchen left a comment

Choose a reason for hiding this comment

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

I've went through all the listed repos, looks good! Tests are passing too \o/
(one failing due to port conflicts - but that should be fixed with the PR that was merged last night)

@divagant-martian
Copy link
Collaborator

What would you think about importing the whole lighthouse history into the repos and removing the unrelated files? I'ts a bit sad to lose all the history of those crates. While this is merged it should also make way easier to migrate changes from lighthouse to the repos via a rebase

@divagant-martian
Copy link
Collaborator

Also, have you thought how to do some kind of continuous integrations testing? as in, making sure these crates remain compatible between individual releases?

@michaelsproul
Copy link
Member Author

michaelsproul commented Apr 4, 2023

What would you think about importing the whole lighthouse history into the repos and removing the unrelated files?

This is also something @paulhauner raised and I regret not doing this from the start. I didn't know about git filter-repo when I did the initial split. However, now that it's done I don't want to re-do it. My reasons for wanting to leave it as-is:

  • History is still available, it's just less accessible. We could link to the history from each repo to make bridging the gap easier, e.g.
  • Contributor stats are hard to get right regardless of the approach we take. With the current approach the contributor stats for LH are still accurate, but the commits by me to create the external repos give me a bunch of extra green "additions". If we copied the whole lighthouse repo and then deleted stuff we'd be double-counting everyone's work on every external repo, and there'd be a huge red commit where someone (me?) deletes everything. The filter-repo approach would create external repos with the most accurate stats, but has other downsides (below).
  • The filter-repo approach rewrites the history so that the commits on the external repo are no longer the real commits from sigp/lighthouse with their larger context. This could make it moderately hard to track down why changes were made, because we'd need to refer back to the sigp/lighthouse repo anyway via pull-request numbers, etc.
  • Duplicating the Lighthouse repo and then deleting the irrelevant bits would bloat the size of every external repo to the same size as Lighthouse. By my estimate (du -sh .git) this is around 240MB. This wouldn't impact the size of the crates on crates.io, but it would amount to 5x 250MB = 1.25GB of extra space for devs.
  • We would need to rewrite the history for the external repos. We'd need to re-do the changes that have been made outside of sigp/lighthouse including renames, adding CI, and changes that never made it to unstable like Abstract quoted serde utils ssz_types#3 and Fix VariableList pre-allocation bug! ssz_types#6. Further, we'd need to decide whether to preserve the history of the current external repos, which were used to publish the current releases on crates.io. We could delete the current repo history entirely by force-pushing, but this would really be a permanent loss. We'd probably have to archive the current repos and start from scratch with new ones.
  • This PR has been a lot of work to get into a reviewable state, and I would like to be able to merge it soon without re-doing a bunch of work.

All in all, I'd prefer to just push ahead with the current approach and take these learnings on board for future crate extractions 🦷

While this is merged it should also make way easier to migrate changes from lighthouse to the repos via a rebase

AFAIK there aren't any outstanding changes on Lighthouse that aren't in the repos. If we merge soon, hopefully the propagation of this PR to 4844-land will be virtually conflict-free, and 4844 can just use the new versions of the crates as-is.

Also, have you thought how to do some kind of continuous integrations testing? as in, making sure these crates remain compatible between individual releases?

Do you mean compatible with Lighthouse, or compatible with new compiler releases? I think compatibility with Lighthouse is adequately covered by the tests in Lighthouse itself, like the ones in consensus/types. For compiler versions we could add some scheduled actions that run on the external repos every 1/2/4 weeks and report any breakages?

Copy link
Member

@paulhauner paulhauner left a comment

Choose a reason for hiding this comment

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

I agree with @divagant-martian that it's sad to see the history go. I also see @michaelsproul's points about the baggage it contains. I also appreciate that, contrary to popular belief, Michael is a mortal human being and there are Git operations outside his knowledge. (Either that or his developers forgot to include git --help in his training data 🤖).

In terms of volume, I think I have the most to lose in the history. Some of my favorite bits of work are in those crates, however Git history isn't actual history and I can live with that! Anyway, the true history lives on in sigp/lighthouse!

That being said, my cup runneth over when it comes to Lighthouse attribution so it's totally valid for others to feel differently.

I think we can separate the issues at hand into two:

  1. The need to split the crates into different repos.
  2. How to balance full attribution against it's drawbacks.

I think we all agree that (1) is something we should do and this PR achieves (1) without making any decisions about (2) that aren't already made. Given that, I think we should merge this PR but consider the attribution topic still open (although I'm personally OK with attribution where it is now).

@paulhauner
Copy link
Member

I'm happy to see a ready-for-bors flag on this once the conflict is resolved.

@paulhauner paulhauner added v4.2.0 Q2 2023 and removed v4.1.0 Post-Capella minor release labels Apr 13, 2023
@michaelsproul michaelsproul added ready-for-merge This PR is ready to merge. and removed ready-for-review The code is ready for review labels Apr 28, 2023
@michaelsproul
Copy link
Member Author

I've fixed the merge conflicts and marked it ready for bors!

@michaelsproul
Copy link
Member Author

bors r+

😮

bors bot pushed a commit that referenced this pull request Apr 28, 2023
## Proposed Changes

Split out several crates which now exist in separate repos under `sigp`.

- [`ssz` and `ssz_derive`](https://github.com/sigp/ethereum_ssz)
- [`tree_hash` and `tree_hash_derive`](https://github.com/sigp/tree_hash)
- [`ethereum_hashing`](https://github.com/sigp/ethereum_hashing)
- [`ethereum_serde_utils`](https://github.com/sigp/ethereum_serde_utils)
- [`ssz_types`](https://github.com/sigp/ssz_types)

For the published crates see: https://crates.io/teams/github:sigp:crates-io?sort=recent-updates.

## Additional Info

- [x] Need to work out how to handle versioning. I was hoping to do 1.0 versions of several crates, but if they depend on `ethereum-types 0.x` that is not going to work. EDIT: decided to go with 0.5.x versions.
- [x] Need to port several changes from `tree-states`, `capella`, `eip4844` branches to the external repos.
@bors bors bot changed the title Split common crates out into their own repos [Merged by Bors] - Split common crates out into their own repos Apr 28, 2023
@bors bors bot closed this Apr 28, 2023
@michaelsproul michaelsproul deleted the split-crates-out branch April 28, 2023 03:48
bors bot pushed a commit that referenced this pull request May 30, 2023
## Issue Addressed

`publish-crate` action and its script no longer used after #3890.
divagant-martian pushed a commit to divagant-martian/lighthouse that referenced this pull request Jun 7, 2023
## Issue Addressed

`publish-crate` action and its script no longer used after sigp#3890.
ghost pushed a commit to oone-world/lighthouse that referenced this pull request Jul 13, 2023
## Proposed Changes

Split out several crates which now exist in separate repos under `sigp`.

- [`ssz` and `ssz_derive`](https://github.com/sigp/ethereum_ssz)
- [`tree_hash` and `tree_hash_derive`](https://github.com/sigp/tree_hash)
- [`ethereum_hashing`](https://github.com/sigp/ethereum_hashing)
- [`ethereum_serde_utils`](https://github.com/sigp/ethereum_serde_utils)
- [`ssz_types`](https://github.com/sigp/ssz_types)

For the published crates see: https://crates.io/teams/github:sigp:crates-io?sort=recent-updates.

## Additional Info

- [x] Need to work out how to handle versioning. I was hoping to do 1.0 versions of several crates, but if they depend on `ethereum-types 0.x` that is not going to work. EDIT: decided to go with 0.5.x versions.
- [x] Need to port several changes from `tree-states`, `capella`, `eip4844` branches to the external repos.
ghost pushed a commit to oone-world/lighthouse that referenced this pull request Jul 13, 2023
## Issue Addressed

`publish-crate` action and its script no longer used after sigp#3890.
Woodpile37 pushed a commit to Woodpile37/lighthouse that referenced this pull request Jan 6, 2024
Split out several crates which now exist in separate repos under `sigp`.

- [`ssz` and `ssz_derive`](https://github.com/sigp/ethereum_ssz)
- [`tree_hash` and `tree_hash_derive`](https://github.com/sigp/tree_hash)
- [`ethereum_hashing`](https://github.com/sigp/ethereum_hashing)
- [`ethereum_serde_utils`](https://github.com/sigp/ethereum_serde_utils)
- [`ssz_types`](https://github.com/sigp/ssz_types)

For the published crates see: https://crates.io/teams/github:sigp:crates-io?sort=recent-updates.

- [x] Need to work out how to handle versioning. I was hoping to do 1.0 versions of several crates, but if they depend on `ethereum-types 0.x` that is not going to work. EDIT: decided to go with 0.5.x versions.
- [x] Need to port several changes from `tree-states`, `capella`, `eip4844` branches to the external repos.
Woodpile37 pushed a commit to Woodpile37/lighthouse that referenced this pull request Jan 6, 2024
## Issue Addressed

`publish-crate` action and its script no longer used after sigp#3890.
Woodpile37 pushed a commit to Woodpile37/lighthouse that referenced this pull request Jan 6, 2024
Split out several crates which now exist in separate repos under `sigp`.

- [`ssz` and `ssz_derive`](https://github.com/sigp/ethereum_ssz)
- [`tree_hash` and `tree_hash_derive`](https://github.com/sigp/tree_hash)
- [`ethereum_hashing`](https://github.com/sigp/ethereum_hashing)
- [`ethereum_serde_utils`](https://github.com/sigp/ethereum_serde_utils)
- [`ssz_types`](https://github.com/sigp/ssz_types)

For the published crates see: https://crates.io/teams/github:sigp:crates-io?sort=recent-updates.

- [x] Need to work out how to handle versioning. I was hoping to do 1.0 versions of several crates, but if they depend on `ethereum-types 0.x` that is not going to work. EDIT: decided to go with 0.5.x versions.
- [x] Need to port several changes from `tree-states`, `capella`, `eip4844` branches to the external repos.
Woodpile37 pushed a commit to Woodpile37/lighthouse that referenced this pull request Jan 6, 2024
## Issue Addressed

`publish-crate` action and its script no longer used after sigp#3890.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
major-task A significant amount of work or conceptual task. ready-for-merge This PR is ready to merge. v4.2.0 Q2 2023
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants