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

Enable Go Modules for tool dependencies #65

Merged
merged 2 commits into from
Jun 12, 2020
Merged

Conversation

itchyny
Copy link
Contributor

@itchyny itchyny commented Jun 8, 2020

Currently the Makefile disables Go Modules for tools but this is dangerous. Many Go tools are tested locking their dependencies with Go Modules. But disabling Go Modules on installing them can break easily when the tools' dependencies are updated without compatibility in the master.
See https://github.com/golang/go/wiki/Modules#how-can-i-track-tool-dependencies-for-a-module.

@itchyny itchyny force-pushed the tools-dependencies branch 3 times, most recently from ff7fb3e to d4a3511 Compare June 8, 2020 10:58
Copy link
Member

@tarao tarao left a comment

Choose a reason for hiding this comment

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

@itchyny The idea looks good but make credits stops working.

tools.go Outdated
_ "github.com/Songmu/gocredits"
_ "github.com/golang/mock/mockgen"
_ "github.com/jessevdk/go-assets-builder"
_ "github.com/jpillora/go-tcp-proxy"
Copy link
Member

Choose a reason for hiding this comment

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

It looks that this breaks make credits because it lacks license file. Should we somehow exclude license files of tools since they are not included in the generated binary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's better to include in the CREDITS file even if it is a tool dependency. I just opened jpillora/go-tcp-proxy#13 and wait for the information.

Copy link
Member

Choose a reason for hiding this comment

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

I understand that tool dependencies should be listed but I don't think tool (or test library) licenses should be included because we are not redistributing them. Including them might be even misleading to users.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, but we can't distinguish them from single go.mod file so maybe we can either

  • create _tools/go.mod and always change the working directory to _tools on installing them in Makefile
  • or change the working directory to a temporary directory (or just cd && which I often use in my project) and give up locking the versions of tools dependencies.

Copy link
Member

Choose a reason for hiding this comment

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

_tools/go.mod sounds good.

@itchyny itchyny marked this pull request as draft June 9, 2020 03:02
@itchyny itchyny marked this pull request as ready for review June 11, 2020 09:49
Copy link
Member

@tarao tarao left a comment

Choose a reason for hiding this comment

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

@itchyny LGTM

@itchyny
Copy link
Contributor Author

itchyny commented Jun 12, 2020

Thank you.

@itchyny itchyny merged commit e0ed7cf into master Jun 12, 2020
@itchyny itchyny deleted the tools-dependencies branch June 12, 2020 05:27
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.

None yet

2 participants