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

add go mod support #148

Closed
wants to merge 1 commit into from
Closed

add go mod support #148

wants to merge 1 commit into from

Conversation

jsign
Copy link

@jsign jsign commented Aug 13, 2019

Add go mod support.

@jsign
Copy link
Author

jsign commented Aug 13, 2019

Is there any particular dependency version needed?
Having defined dependency versions (and vendoring) make performance benchmarks future-proof.

@aaronbee
Copy link

I don't believe vendoring dependencies is appropriate here. go.mod will ensure all users of the library get the right dependencies without the need of the vendor directory.

@jsign
Copy link
Author

jsign commented Aug 13, 2019

Well, that's is assuming that dependencies don't mess-up with mutating versions. This might be a reasonable assumption considering the particular small set of dependencies of this project; the worst case scenario will complain about hash-checks when go mod download.

Do you consider that might be enough and better removing vendor folder?

@aaronbee
Copy link

Yes, you can assume dependencies don't mess up their versions. This is the reason for go.sum and the public notary (discussed here) It would be a crummy dependency management system if you couldn't rely on a dependency version meaning the same thing to everyone.

Disclaimer: I'm just a fan of this particular repo.

@jsign
Copy link
Author

jsign commented Aug 14, 2019

IMHO, vendoring isn't only about version meaning but also availability. Considering that public modules version code is mirrored, vendoring might be redundant.

I think that's a fair point. 👍

@vtolstov
Copy link

Library dont need to have vendor dir, also go mod when you do go get don't download vendor folder
You need to remove it

@klauspost
Copy link
Owner

I am not sure I see the need for explicit modules- it should work just fine.

#105 Should get rid of cpuid for now when it lands. And go-cmp should only be used by tests.

@jsign jsign closed this Aug 14, 2019
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.

4 participants