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

switch from dep to go modules #178

Merged
merged 20 commits into from Aug 4, 2019

Conversation

@zeldovich
Copy link
Contributor

zeldovich commented Jul 24, 2019

This allows checking out and building the go-algorand repo in any directory (doesn't have to be under $GOPATH). This also means that it's possible to have multiple checkouts, and it's possible to check out a forked repo of go-algorand and have it build as expected.

@zeldovich zeldovich requested review from Vervious and Karmastic Jul 24, 2019
@Vervious

This comment has been minimized.

Copy link
Contributor

Vervious commented Jul 24, 2019

The swagger refactor on go-swagger:master must have broken/fixed the (indirect) strfmt annotation... It looks like the following works now:

...
	// Note is a free form data
	//
	// required: true
	// swagger:strfmt binary
	Note string `json:"noteb64,omitempty"`
...

generating:

        "noteb64": {
          "description": "Note is a free form data",
          "type": "string",
          "format": "binary",
          "x-go-name": "Note"
        },

(for some reason, the ordering of required: and swagger:strfmt matter here. In addition, if Note is of type []byte the new swagger parser generates an items field, causing the validator to throw an error. I haven't found any other workaround).

perhaps this means we now need to manually base64-encode byte slices when sticking them into swagger models...

@Vervious

This comment has been minimized.

Copy link
Contributor

Vervious commented Jul 25, 2019

Perhaps the right approach here is to manually maintain the swagger.json files... We don't modify them much, and the upkeep cost of go-swagger has been extraordinary.

@Vervious

This comment has been minimized.

Copy link
Contributor

Vervious commented Jul 25, 2019

Let me dig into go-swagger, see what is broken with strfmt and perhaps make a PR. in the meantime if we want to push go mod support I'm happy with disabling the go generate step temporarily, and commiting/manually updating swagger.json (perhaps even adopting that long term)

@zeldovich

This comment has been minimized.

Copy link
Contributor Author

zeldovich commented Jul 25, 2019

Thank you for tracking it down!

To the extent the swagger file is used (and I think the Java SDK does actually use it), it seems worth generating it. Otherwise we will surely forget to revise it when we change the Go code.

If we can't find a better workaround, then switching the binary types into Go type string and manually annotating them as // swagger:strfmt base64 seems not too bad. This means we'll have to also manually do the base64 encoding/decoding, since the JSON codec will not base64-encode the Go string type (but does base64-encode the []byte type).

@Vervious

This comment has been minimized.

Copy link
Contributor

Vervious commented Jul 25, 2019

Doing the base64 conversion isn't that bad - there are only a couple of places. The loss of type checking seems manageable as well

Speaking of // swagger:strfmt base64 - I recall intentionally choosing //swagger:strfmt binary over // swagger:strfmt base64 and // swagger:strfmt byte... but never documented it (and have since forgotten). Let me also look into that again. probably has to do with swagger-codegen support in java for the java sdk.

Makefile Outdated

default: build

# tools

fmt:
cd $(SRCPATH) && \
go fmt `go list ./... | grep -v /vendor/`
go fmt `go list ./... | grep -v /vendor/`

This comment has been minimized.

Copy link
@winder

This comment has been minimized.

Copy link
@zeldovich

zeldovich Jul 26, 2019

Author Contributor

Hah, yeah, thanks! That's cleaner.

@Vervious

This comment has been minimized.

Copy link
Contributor

Vervious commented Jul 31, 2019

We should be able to push this forward now, zeldovich#1

zeldovich and others added 5 commits Jun 20, 2019
vendor using vend:

    go get github.com/nomad-software/vend
    ~/go/bin/vend
no need to exclude /vendor/ in many commands
@zeldovich zeldovich force-pushed the zeldovich:gomod branch from ff0c9eb to 0a30346 Jul 31, 2019
zeldovich added 7 commits Jul 31, 2019
See https://research.swtch.com/vgo-module under "The End of Vendoring".

go.sum gives us cryptographic checksums of our dependent code, and
Google's Go proxy and/or https://docs.gomods.io/ can help us get dependent
packages if those repositories are deleted.
@zeldovich zeldovich force-pushed the zeldovich:gomod branch from d16af37 to 6a8a106 Aug 2, 2019
Same behavior regardless of whether the checkout directory is under
$GOPATH/src or not.
@zeldovich zeldovich force-pushed the zeldovich:gomod branch from 6a8a106 to b750b2c Aug 2, 2019
Karmastic and others added 2 commits Aug 2, 2019
Fix scripts to work with go modules / arbitrary root source location
Copy link
Contributor

Karmastic left a comment

Just a couple of nits if you want to fix them

Makefile Outdated

UNIT_TEST_SOURCES := $(sort $(shell cd $(SRCPATH) && go list ./... | grep -v /go-algorand/test/ | grep -v /go-algorand/vendor/ ))
E2E_TEST_SOURCES := $(shell cd $(SRCPATH)/test/e2e-go && go list ./...)
UNIT_TEST_SOURCES := $(sort $(shell go list ./... | grep -v /go-algorand/test/ | grep -v /go-algorand/vendor/ ))

This comment has been minimized.

Copy link
@Karmastic

Karmastic Aug 3, 2019

Contributor

Not a blocker but we can remove /vendor/ here.

Makefile Outdated

sanity: vet fix lint fmt

cover:
cd $(SRCPATH) && \
go test $(GOTAGS) -coverprofile=cover.out $(UNIT_TEST_SOURCES)

This comment has been minimized.

Copy link
@Karmastic

Karmastic Aug 3, 2019

Contributor

nit - this can be unindented

make build
export BUILD_DEB=1
scripts/build_packages.sh "${PLATFORM}"
# Anchor our repo root reference location

This comment has been minimized.

Copy link
@Karmastic

Karmastic Aug 3, 2019

Contributor

This looks like an accidental duplicate of this code block and should be removed (my mistake)

@zeldovich zeldovich merged commit 27136f7 into algorand:master Aug 4, 2019
2 checks passed
2 checks passed
Travis CI - Pull Request Build Passed
Details
license/cla Contributor License Agreement is signed.
Details
@zeldovich zeldovich deleted the zeldovich:gomod branch Aug 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.