cleanup go.mod #137

zhangchiqing · 2020-11-09T23:16:25Z

Remove the replace in go.mod to fix circular dependency

jordanschalm · 2020-11-09T23:53:48Z

go.mod

@@ -60,8 +60,6 @@ require (

 replace mellium.im/sasl => github.com/mellium/sasl v0.2.1

-replace github.com/onflow/flow-go => ./


Why did we have this in the first place?

I remember there being a reason for this, but I can't remember why 😓 I'll try to dig up the PR.

I'd say that if everything builds and all the test pass without it, we should remove it.

I think this was temporary during the GH org migration

Found the relevant PR: https://github.com/dapperlabs/flow-go/pull/1959#discussion_r361214248

I think it's okay to remove all of these replaces if the tests pass without them

I can't remove them all, as you can see, the build will failure.

I can only remove this line to fix the build. But I don't understand why other "replace"s can't be removed

I think part of the issue is the integration package is pinning a very old version of flow-go. With the replace that old version gets replaced by whatever is in the replacement path on the machine but without the replace we actually fetch that old version.

go.mod

turbolent · 2020-11-10T00:05:41Z

The dependency on flow-go/crypto needs to be updated: https://github.com/onflow/flow-go/pull/137/files#diff-33ef32bf6c23acb95f5902d7097b7a1d5128ca061167ec0716715b0b9eeaa5f6R36

turbolent · 2020-11-10T00:29:19Z

@zhangchiqing flow-go is already using new code of the crypto library, even though that hasn't been "released" yet. crypto needs to get tagged with a new release. cc @tarakby @Kay-Zee

Kay-Zee · 2020-11-10T00:40:26Z

I've release the crypto package, at the same commit as v0.11.0: https://github.com/onflow/flow-go/releases/tag/crypto%2Fv0.11.0

zhangchiqing · 2020-11-10T00:40:29Z

@zhangchiqing flow-go is already using new code of the crypto library, even though that hasn't been "released" yet. crypto needs to get tagged with a new release. cc @tarakby @Kay-Zee

Does it mean, with the "replace", flow-go will refer to the crypto code on the same commit. Whereas, without the "replace", it will refer to the code on the latest "release"?

If that's the case, I think we should keep the two "replace", so that code is always compatible with the version on the same commit. no?

turbolent · 2020-11-10T00:54:32Z

Does it mean, with the "replace", flow-go will refer to the crypto code on the same commit. Whereas, without the "replace", it will refer to the code on the latest "release"?

If that's the case, I think we should keep the two "replace", so that code is always compatible with the version on the same commit. no?

Yes, when the replace statement is there/kept, the latest version of the crypto code is used.
However, Go does not use it (!) when the flow-go itself is used as a dependency elsewhere, and this is oddly by design: golang/go#30354

Keeping the statement will "fix" the issue for this repo, but it breaks downstream dependencies like the emulator, which imports flow-go.

turbolent · 2020-11-10T00:55:37Z

See golang/go#30354 (comment):

replace is intended to support local development

turbolent · 2020-11-10T00:58:36Z

go.mod

+// needed otherwise the code refers to the crypto package will refer to
+// the released version of the crypto library, which might not be the latest
 replace github.com/onflow/flow-go/crypto => ./crypto


Instead of this replace statement, crypto should be bumped to crypto/v0.11.0

I agree with Bastian on this. We should pin a specific version and keep it up to date. Otherwise importers of flow-go could have crypto-dependent flow-go packages that either don't compile or are invisibly using an older version of crypto.

I agree. I was trying that, but for some reason, it didn't work.

The version for crypto has been pinned here. Suppose we no longer need this replace. But if I remove it, make lint will start complaining.

Not sure what's the proper fix.

I guess this is why:

flow-go/crypto/README.md

Lines 16 to 26 in f84b80f

If you wish to only import the Flow cryptography package to your Go project, please follow the following steps:

- Get Flow cryptography package.

```

go get github.com/onflow/flow-go/crypto

```

- Install [CMake](https://cmake.org/install/), which is used for building the package.

- From the package directory in `$GOPATH/pkg/mod/github.com/onflow/`, build the package dependencies.

```

go generate

```

When we pin a version, Go will download a new copy in pkg/mod and that copy hasn't had the extra C build steps applied to it. We would need to follow @tarakby's instructions for the copy managed by go mod. Probably we would need to do that every time we change the version. From a bit of searching it doesn't really seem like Go has a good solution for modules that require a non-Go build step.

I think there is another reason for @zhangchiqing's linter issues that need to be solved.
When we go get Flow crypto lib, the C source code of the package is copied along with Go files to pkg/mod. What I recommended in the readme is only related to the Relic dependency.

Update: Relic dependency is actually what was missing. cgo could work properly once I followed the readme step (still v0.11.0 is an outdated version and needs to be updated)

jordanschalm · 2020-11-11T02:58:05Z

integration/go.mod

@@ -23,6 +23,6 @@ require (

 replace github.com/onflow/flow-go => ../


Here I think it's ok to keep this at least in the short term 👍. Integration tests aren't really intended to be imported to other projects.

turbolent · 2020-11-25T18:35:44Z

Brought it up yesterday again (https://axiomzen.slack.com/archives/C015G65HR2P/p1606232145256900): the replace statements should be removed and crypto should be tagged instead.

zhangchiqing · 2020-11-28T03:54:37Z

I did the following thing to resolve the dependencies for now:

I made a tag for crypto/v0.12.0
I cherry-picked changes and applied to v0.12.1, and tagged a new version v0.12.2, which pinned crypto to be v0.12.0. This allows projects like emulator / flow-mgmt to depend on flow-go v0.12.2 and crypto v0.12.0 without running into version issues. (however, this might still require to run go generate in $GOPATH/pkg/mod/github.com/onflow/flow-go/crypto@v0.12.0/).
I pinned the crypto version to be v0.12.0 in this repo.

I have tested that it works:

in a project (flow-mgmt), I imported both flow-go and flow-go/crypto:

import (
	flowcrypto "github.com/onflow/flow-go/crypto"
)

func NewBLSKey() {
	hasher := flowcrypto.NewBLSKMAC("G")
	fmt.Println("hasher", hasher)
}

ran go build -tags=relic will fail:

# github.com/onflow/flow-go/crypto
In file included from ../gopath/pkg/mod/github.com/onflow/flow-go/crypto@v0.12.0/bls.go:32:
../gopath/pkg/mod/github.com/onflow/flow-go/crypto@v0.12.0/bls_include.h:8:10: fatal error: 'relic.h' file not found
#include "relic.h"
         ^~~~~~~~~
1 error generated.

I fixed by running go generate:

cd $GOPATH/pkg/mod/github.com/onflow/flow-go/crypto@v0.12.0/
sudo go generate

After that, I'm able to make a build without the error.

crypto/build_dependency.sh

integration/go.mod

Co-authored-by: Jordan Schalm <jordan@dapperlabs.com>

turbolent · 2020-11-30T18:26:04Z

@zhangchiqing Fantastic work, thank you so much for cleaning this up! 👏 🏅

turbolent

Great work!

…eo/remove-replace

jordanschalm

This changes our setup process as well.

Do we need to update Git submodules any more if we aren't directly working on crypto?

flow-go/README.md

Lines 60 to 64 in a406926

    
           - Clone this repository's submodules: 
        
           ```bash 
        
           git submodule update --init --recursive 
        
           ```

We also need to ensure the go generate gets run after a fresh install and every time we change the version of flow-go/crypto dependency:

cd $GOPATH/pkg/mod/github.com/onflow/flow-go/crypto@v0.12.0/
sudo go generate

Should we put this in README as a manual step?

tarakby

Thank you Leo for looking at this!

zhangchiqing · 2020-11-30T18:44:20Z

We also need to ensure the go generate gets run after a fresh install and every time we change the version of flow-go/crypto dependency:

We don't need to run go generate if we are working flow-go itself, because we have the replace statement in go.mod

We also don't need to run go generate if flow-go/crypto is imported as a dependency in other project, and that project doesn't use modules like BLS signature, which requires the relic flag.

It's only needed when flow-go/crypto is imported as a dependency in other project, and that project uses modules that requires to build with relic flag.

jordanschalm · 2020-11-30T20:34:00Z

we have the replace statement in go.mod

I see, I was still was thinking we would remove that as well. Thanks @zhangchiqing

zhangchiqing · 2020-11-30T20:46:50Z

I see, I was still was thinking we would remove that as well

That's the tradeoff. If we remove the "replace", we could ensure the flow-go use the right crypto library. However, everytime we make changes to crypto, and tag a new version, people will have to run go generate, and hate Tarak :p

If we were to remove the "replace", maybe even makes sense to make crypto as a stand alone repo, but we will still have to run go generate every time crypto make change.

so I think it's easier just to keep the replace.

zhangchiqing force-pushed the leo/remove-replace branch 2 times, most recently from 64798f9 to e5e72dc Compare November 9, 2020 23:32

zhangchiqing marked this pull request as ready for review November 9, 2020 23:42

zhangchiqing requested review from m4ksio, psiemens, jordanschalm and turbolent November 9, 2020 23:42

jordanschalm reviewed Nov 9, 2020

View reviewed changes

turbolent reviewed Nov 10, 2020

View reviewed changes

go.mod Outdated Show resolved Hide resolved

turbolent reviewed Nov 10, 2020

View reviewed changes

zhangchiqing requested review from Kay-Zee, turbolent and jordanschalm and removed request for m4ksio November 10, 2020 16:49

jordanschalm reviewed Nov 11, 2020

View reviewed changes

Kay-Zee approved these changes Nov 20, 2020

View reviewed changes

Kay-Zee approved these changes Nov 24, 2020

View reviewed changes

turbolent mentioned this pull request Nov 25, 2020

Update to Cadence v0.10.2, SDK v0.12.2, and Emulator v0.12.2 onflow/flow-cli#4

Merged

zhangchiqing force-pushed the leo/remove-replace branch from 0d7bfe4 to 8961a7c Compare November 26, 2020 00:05

zhangchiqing and others added 4 commits November 27, 2020 15:47

update versions

418cdaa

add replace to go.mod for crypto

ac6a7b4

fix crypto version for v0.12

6e46f0d

update relic version and separate go generate in a script

45b7758

tarakby and others added 4 commits November 27, 2020 17:41

add a missing new line

01f3200

add a version variable

130702f

upgrade to v0.12.2

b172f41

Merge branch 'bumpup-v0.12.2' into leo/remove-replace

41abbf4

zhangchiqing force-pushed the leo/remove-replace branch from ccfb576 to 41abbf4 Compare November 28, 2020 02:40

zhangchiqing requested a review from tarakby November 28, 2020 03:54

jordanschalm approved these changes Nov 30, 2020

View reviewed changes

crypto/build_dependency.sh Outdated Show resolved Hide resolved

crypto/build_dependency.sh Outdated Show resolved Hide resolved

integration/go.mod Outdated Show resolved Hide resolved

integration/go.mod Outdated Show resolved Hide resolved

jordanschalm self-requested a review November 30, 2020 18:24

Apply suggestions from code review

eeaedae

Co-authored-by: Jordan Schalm <jordan@dapperlabs.com>

turbolent approved these changes Nov 30, 2020

View reviewed changes

zhangchiqing added 2 commits November 30, 2020 10:27

Merge branch 'master' into leo/remove-replace

5981a5c

Merge branch 'leo/remove-replace' of github.com:onflow/flow-go into l…

be3077d

…eo/remove-replace

jordanschalm reviewed Nov 30, 2020

View reviewed changes

tarakby approved these changes Nov 30, 2020

View reviewed changes

zhangchiqing added 2 commits November 30, 2020 11:23

update crypto readme

c193876

fix go mod

1efef2e

jordanschalm approved these changes Nov 30, 2020

View reviewed changes

zhangchiqing merged commit 7ea3685 into master Nov 30, 2020

zhangchiqing deleted the leo/remove-replace branch November 30, 2020 20:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cleanup go.mod #137

cleanup go.mod #137

zhangchiqing commented Nov 9, 2020

jordanschalm Nov 9, 2020

psiemens Nov 10, 2020

turbolent Nov 10, 2020

psiemens Nov 10, 2020

zhangchiqing Nov 10, 2020

jordanschalm Nov 10, 2020

turbolent commented Nov 10, 2020

turbolent commented Nov 10, 2020

Kay-Zee commented Nov 10, 2020

zhangchiqing commented Nov 10, 2020

turbolent commented Nov 10, 2020

turbolent commented Nov 10, 2020

turbolent Nov 10, 2020

jordanschalm Nov 11, 2020

zhangchiqing Nov 11, 2020

jordanschalm Nov 11, 2020

tarakby Nov 24, 2020 •

edited

Loading

jordanschalm Nov 11, 2020

turbolent commented Nov 25, 2020

zhangchiqing commented Nov 28, 2020

turbolent commented Nov 30, 2020

turbolent left a comment

jordanschalm left a comment

tarakby left a comment

zhangchiqing commented Nov 30, 2020 •

edited

Loading

jordanschalm commented Nov 30, 2020

zhangchiqing commented Nov 30, 2020

		@@ -60,8 +60,6 @@ require (

		replace mellium.im/sasl => github.com/mellium/sasl v0.2.1

		replace github.com/onflow/flow-go => ./

	If you wish to only import the Flow cryptography package to your Go project, please follow the following steps:

	- Get Flow cryptography package.
	```
	go get github.com/onflow/flow-go/crypto
	```
	- Install [CMake](https://cmake.org/install/), which is used for building the package.
	- From the package directory in `$GOPATH/pkg/mod/github.com/onflow/`, build the package dependencies.
	```
	go generate
	```

		@@ -23,6 +23,6 @@ require (

		replace github.com/onflow/flow-go => ../

	- Clone this repository's submodules:

	```bash
	git submodule update --init --recursive
	```

cleanup go.mod #137

cleanup go.mod #137

Conversation

zhangchiqing commented Nov 9, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

turbolent commented Nov 10, 2020

turbolent commented Nov 10, 2020

Kay-Zee commented Nov 10, 2020

zhangchiqing commented Nov 10, 2020

turbolent commented Nov 10, 2020

turbolent commented Nov 10, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tarakby Nov 24, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

turbolent commented Nov 25, 2020

zhangchiqing commented Nov 28, 2020

turbolent commented Nov 30, 2020

turbolent left a comment

Choose a reason for hiding this comment

jordanschalm left a comment

Choose a reason for hiding this comment

tarakby left a comment

Choose a reason for hiding this comment

zhangchiqing commented Nov 30, 2020 • edited Loading

jordanschalm commented Nov 30, 2020

zhangchiqing commented Nov 30, 2020

tarakby Nov 24, 2020 •

edited

Loading

zhangchiqing commented Nov 30, 2020 •

edited

Loading