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

fix(forkdiff): update sub definitions to fix hydration #244

Open
wants to merge 2 commits into
base: optimism
Choose a base branch
from

Conversation

sambacha
Copy link
Contributor

Description

This updates your fork.yaml file so that forkdiff binary can successfully build the report page.

Under the existing fork.yaml file, it fails to build and gives the following error:

$ forkdiff
failed to hydrate patch stats 
error: sub definition 2 failed to hydrate: sub definition 6 failed to hydrate: file "eth/handler.go" was matched by glob 0 ("eth/handler.go") but is not remaining11:36:53 Tue Feb 13 2024 janitor macbook

Tests

I ran forkdiff with the updated changes and was able to successfully build the report page.

Additional context

There is no validation of the file, fork.yaml. Additionally, there exists no timestamp or versioning information in the generated report page.

I added two hard coded values:
1. the version of the corresponding git hash for go-ethereum, v1.13.8
2. a 'Last Updated' value, using TZ=UTC

```yaml
# fork.yaml
footer: |
!  a fork of [`go-ethereum, v1.13.8`](https://github.com/ethereum/go-ethereum).
     description: |

+     Last updated: Tue Feb 13 19:58:26 UTC 2024
  sub:
```

Considerations

fork.yaml should be validated at build, additionally I found no configuration specification for detailing diffing policy or setup.

For reference, this is one configuration

git config --global merge.renameLimit 999999
git config --global diff.renameLimit 33440
git config --global diff.algorithm patience

This is beyond the scope of this PR, I just thought to mention it, as it makes maintainer job easier.

@sambacha
Copy link
Contributor Author

Here is the generated forkdiff report page, I had to gzip it as I can not upload HTML files on comments.

index.html.gz

Comment on lines +28 to +29

Last updated: Tue Feb 13 19:58:26 UTC 2024
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
Last updated: Tue Feb 13 19:58:26 UTC 2024
Last updated: Tue Feb 13 19:58:26 UTC 2024

We're 100% going to forget updating this. The commit hash in the diff page is the best source of truth for last udpate.

@@ -1,6 +1,6 @@
title: "op-geth - go-ethereum fork diff overview"
footer: |
Fork-diff overview of [`op-geth`](https://github.com/ethereum-optimism/op-geth), a fork of [`go-ethereum`](https://github.com/ethereum/go-ethereum).
Fork-diff overview of [`op-geth`](https://github.com/ethereum-optimism/op-geth), a fork of [`go-ethereum, v1.13.8`](https://github.com/ethereum/go-ethereum).
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same here, we won't maintain manual versions in multiple places. Can you revert this part of the change?

@@ -166,7 +168,6 @@ def:
- "eth/ethconfig/config.go"
- title: Tx gossip disable option
globs:
- "eth/handler.go"
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it spit out the error because of the usage of this diff in two places. We should add a description of the eth/handler.go changes in one place, to keep some documentation of the total set of changes.

@protolambda protolambda requested a review from a team as a code owner September 17, 2024 16:34
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.

2 participants