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

go modules for fabric #954

Closed
wants to merge 3 commits into from
Closed

go modules for fabric #954

wants to merge 3 commits into from

Conversation

MHBauer
Copy link
Contributor

@MHBauer MHBauer commented Mar 31, 2020

wip

Type of change

Description

Would like to use some of fabric as a dependency for golang plugins.

Additional details

pkcs11 unit-tests always fail for me. On master and this branch.
idemix integration-tests always fail for me. On master and this
branch.

Completed work:

Related issues

https://jira.hyperledger.org/browse/FAB-10562

A tools.go based dependency management.
https://github.com/golang/go/wiki/Modules#how-can-i-track-tool-dependencies-for-a-module

@MHBauer MHBauer requested a review from a team as a code owner March 31, 2020 04:07
@MHBauer

This comment has been minimized.

Makefile Outdated Show resolved Hide resolved
scripts/check_deps.sh Show resolved Hide resolved
go.mod Show resolved Hide resolved
integration/nwo/components.go Outdated Show resolved Hide resolved
scripts/check_deps.sh Show resolved Hide resolved
gotools.mk Show resolved Hide resolved
@MHBauer MHBauer changed the title wip - checking server build wip - go modules (please glance at to see if you have any ideas) Mar 31, 2020
@MHBauer

This comment has been minimized.

@MHBauer
Copy link
Contributor Author

MHBauer commented Mar 31, 2020

Option 2 worked locally.

2Qs:

  1. Is there a way to speed up the build? I see some stuff in the azure config + the run script, but I'm not sure if I can use it locally.
  2. Is there a way to focus on a specific integration test? It doesn't seem available in the script, so I've been editing the grep line locally.

@MHBauer
Copy link
Contributor Author

MHBauer commented Mar 31, 2020

added gossip flake to https://jira.hyperledger.org/browse/FAB-17451

/ci-run

@github-actions
Copy link

AZP build triggered!

@MHBauer MHBauer changed the title wip - go modules (please glance at to see if you have any ideas) go modules for fabric Mar 31, 2020
Copy link
Contributor

@sykesm sykesm left a comment

Choose a reason for hiding this comment

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

I understand it's a WIP but it should not be merged. There a number of hacks in here that have some unsavory side effects.

I've dusted off my previous work and I think (go figure) it's a better path forward.

https://github.com/sykesm/fabric/commits/modules-playground-2

integration/externalbuilders/golang/bin/build Outdated Show resolved Hide resolved
scripts/check_deps.sh Show resolved Hide resolved
integration/nwo/components.go Outdated Show resolved Hide resolved
integration/discovery/discovery_test.go Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
go.mod Show resolved Hide resolved
Copy link
Contributor Author

@MHBauer MHBauer left a comment

Choose a reason for hiding this comment

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

Thanks for review @sykesm
What's left in https://github.com/sykesm/fabric/commits/modules-playground-2
I'll give it a run and see what shakes out.

integration/externalbuilders/golang/bin/build Outdated Show resolved Hide resolved
go.mod Show resolved Hide resolved
scripts/check_deps.sh Show resolved Hide resolved
integration/nwo/components.go Outdated Show resolved Hide resolved
integration/discovery/discovery_test.go Outdated Show resolved Hide resolved
@sykesm
Copy link
Contributor

sykesm commented Mar 31, 2020

What's left in https://github.com/sykesm/fabric/commits/modules-playground-2

  • There's no implementation in check_deps.sh
  • Tools are installed into $(go env GOPATH)/bin. That's problematic in cases where $GOPATH is really a path or when $GOPATH is not on the $PATH.
  • Miscellaneous doc updates
  • Maybe more ...

@sykesm
Copy link
Contributor

sykesm commented Apr 1, 2020

@MHBauer - Some of the prep changes have been merged into master. Are you planning to continue this thread by implementing some of the remaining items I enumerated above?

Also, with respect to the vendor vs. non-vendor: You're assuming that the modules required for a fabric build will be cached. While that's true for most local development, it's not generally true. For example, our docker images are built within an alpine container that has no cache. That means each time we build a binary, we go off to get all of the packages we need for build. The time it takes can vary widely depending on where you are in the world and what the network looks like.

I'd also assert that vendored packages help when doing code-scans and reproducible builds when building with -mod=vendor as (almost) all of the source comes from our tree or the go standard library.

Copy link
Contributor Author

@MHBauer MHBauer left a comment

Choose a reason for hiding this comment

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

flake, but the dependencies look weird to me from the tools update.

go.mod Outdated
go.uber.org/atomic v1.4.0 // indirect
go.uber.org/multierr v1.2.0 // indirect
go.uber.org/tools v0.0.0-20190618225709-2cfd321de3ee // indirect
go.uber.org/zap v1.11.0
Copy link
Contributor Author

Choose a reason for hiding this comment

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

especially weird that this would go backwards.

Copy link
Contributor

@sykesm sykesm Apr 6, 2020

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah, you did the tools first. I can slap the third commit for the verify on top of those two and give it a go.

I'm worried about what happens in a clean environment in terms of it changing go.mod when building the tools.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had a CRLF git issue with
vendor/github.com/Knetic/govaluate/.gitignore
but otherwise got it cleanly building.

Looks to work in the build as well, besides some gossip flaknes: https://dev.azure.com/mbauer0254/fabric/_build/results?buildId=4&view=results

I'll push a modified rebase with everything, and you can check it out or modify it yourself on my fork, or push your own. Let me know what you want to do.

github.com/vektra/mockery v0.0.0-20181123154057-e78b021dcbb5
golang.org/x/lint v0.0.0-20191125180803-fdd1cda4f05f
golang.org/x/tools v0.0.0-20191227053925-7b8e75db28f4
)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh I get it now, a separate module for the tools.

_ "github.com/vektra/mockery/cmd/mockery"
_ "golang.org/x/lint/golint"
_ "golang.org/x/tools/cmd/goimports"
)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

is this going to need to be vendored as well?

@MHBauer
Copy link
Contributor Author

MHBauer commented Apr 7, 2020

I can repro a bunch of other flakes, but not this one.

/ci-run

@github-actions
Copy link

github-actions bot commented Apr 7, 2020

AZP build triggered!

sykesm and others added 3 commits April 8, 2020 10:03
Signed-off-by: Matthew Sykes <sykesmat@us.ibm.com>
Change-Id: I6e0b4666e12aec65ed9e3f2f6b871cd942ca66d9
original has crlf

Signed-off-by: Matthew Sykes <sykesmat@us.ibm.com>
Change-Id: I2212e14f574d9e2c3d870bd718aee50ebcf1fc85
Change-Id: Iad450b21a63156a73b4299fbc22745cd2fe96e1f
Signed-off-by: Morgan Bauer <mbauer@us.ibm.com>
@sykesm
Copy link
Contributor

sykesm commented Apr 10, 2020

@MHBauer tools as module looks to have been merged into master in #1057. One more step forward.

@MHBauer
Copy link
Contributor Author

MHBauer commented Apr 15, 2020

Looks mostly superceded by #1064, but it doesn't look like we actually care about replicating check-deps.

@MHBauer MHBauer closed this Apr 15, 2020
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