Skip to content
This repository has been archived by the owner on Jan 24, 2024. It is now read-only.

Adopt go.work, rename modules to prep for monorepo #414

Closed
wants to merge 1 commit into from

Conversation

protolambda
Copy link
Collaborator

@protolambda protolambda commented May 5, 2022

This PR:

  • renames the modules to their final destination name in the monorepo
  • splits the packages into modules: op-node, op-proposer, op-batcher, op-e2e
  • moves ops to ops-bedrock, to prepare for monorepo
  • Many Makefile and Dockerfile updates to make the above work
  • updates CI (including getting coverage for all the separate modules)

@protolambda protolambda force-pushed the go-work branch 4 times, most recently from b216411 to 7329233 Compare May 5, 2022 16:01
@protolambda
Copy link
Collaborator Author

Stuck on a go issue:

../../../go/pkg/mod/github.com/btcsuite/btcd@v0.22.0-beta/chaincfg/genesis.go:10:2: ambiguous import: found package github.com/btcsuite/btcd/chaincfg/chainhash in multiple modules:
        github.com/btcsuite/btcd v0.22.0-beta (/home/proto/go/pkg/mod/github.com/btcsuite/btcd@v0.22.0-beta/chaincfg/chainhash)
        github.com/btcsuite/btcd/chaincfg/chainhash v1.0.0 (/home/proto/go/pkg/mod/github.com/btcsuite/btcd/chaincfg/chainhash@v1.0.0)

Which I think causes a build issue which the linting tooling can't handle (but go itself can somehow...):

golangci-lint run -E asciicheck,goimports,misspell --fix
WARN [runner] Can't run linter goanalysis_metalinter: buildir: failed to load package : could not load export data: no export data for "github.com/btcsuite/btcd/chaincfg/chainhash" 
ERRO Running error: 1 error occurred:
        * can't run linter goanalysis_metalinter: buildir: failed to load package : could not load export data: no export data for "github.com/btcsuite/btcd/chaincfg/chainhash"

@protolambda
Copy link
Collaborator Author

See btcsuite/btcd#1839
Fixed by upgrading to github.com/btcsuite/btcd v0.22.1

@protolambda protolambda force-pushed the go-work branch 3 times, most recently from f762ce0 to 1cd9f16 Compare May 5, 2022 17:35
Copy link
Contributor

@trianglesphere trianglesphere left a comment

Choose a reason for hiding this comment

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

In general this looks good. The big thing I'd like is to set a cutoff date for which PRs go in first and which PRs should be built on top of this branch (to try to limit merge / rebase conflicts).

Makefile Outdated Show resolved Hide resolved
@protolambda
Copy link
Collaborator Author

protolambda commented May 5, 2022

Rebased to resolve merge conflicts. Edit: and force-pushed again to fix the opnode > op-node change in the new fuzz makefile target.

@protolambda
Copy link
Collaborator Author

Deleted code cov comment since it wasn't updating anymore. Coverage is stable, no changes to actual code, and we merge all the coverage reports of different modules by pushing them all to codecov. You can find the coverage diff here: https://app.codecov.io/gh/ethereum-optimism/optimistic-specs/compare/main...go-work (some renames, basically 0 coverage difference)

Copy link
Contributor

@trianglesphere trianglesphere left a comment

Choose a reason for hiding this comment

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

I left some small comments around the makefile.
Also FYI - the integration test didn't pass locally, but I think that is probably the test being flaky b/c it passed in CI.

@@ -18,9 +18,17 @@ submodules:
fi
.PHONY: submodules

opnode:
Copy link
Contributor

Choose a reason for hiding this comment

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

Not here, but in submodules, we very likely want --recursive. Maybe this changes with the new contracts stuff, but foundry-lib seemed to need it.

@@ -35,39 +43,34 @@ clean:
.PHONY: clean
Copy link
Contributor

Choose a reason for hiding this comment

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

The clean command doesn't do anything b/c we leave binaries in the subdirectory

@@ -4,7 +4,7 @@ ITESTS_L2_HOST=http://localhost:9545
build: build-go contracts integration-tests
Copy link
Contributor

Choose a reason for hiding this comment

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

Not this, but above COMPOSEFLAGS /ITESTS_L2_HOST doesn't seem to be referenced anywhere. (

@protolambda
Copy link
Collaborator Author

I can help fix this merge conflict with a rebase in 4 hours from now. Basically need to rebase on main, drop all the old file moves + import rewrites, and redo those, then fix up some go imports, while preserving the docker and ci config fixes.

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.

3 participants