Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

Comments

Makefile: use go run instead of globally installing Go tools#1210

Open
marten-seemann wants to merge 2 commits intomasterfrom
makefile-go-run-install
Open

Makefile: use go run instead of globally installing Go tools#1210
marten-seemann wants to merge 2 commits intomasterfrom
makefile-go-run-install

Conversation

@marten-seemann
Copy link
Contributor

Fixes#1209. See issue for motivation.

Possible future improvements:

  • Hard-code the version of the respective tools to make the build (more) deterministic. At the moment, an update of (say) the linter might break the build, if the linter becomes stricter in the new version.
  • Use thetools.go method (example) to havego.mod manage the versions of the tools. Main advantage: We could use Dependabot to upgrade to new versions, and have CI test that upgrade doesn't break anything.

@CLAassistant
Copy link

CLAassistant commentedJun 19, 2024
edited
Loading

CLA assistant check
All committers have signed the CLA.

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign ourContributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let usrecheck it.

@github-actionsgithub-actionsbot added the needs triageWaiting for discussion / prioritization by team labelJun 19, 2024
@hslatman
Copy link
Member

If we make changes to this, then I would prefer thetools.go method. I've used it myself before, and there's at least one other internal project where we make use of it. I believe you'll get a specific, hardcoded version there too, based on what's ingo.mod.

Note that much of the Makefile is cloned in pretty much all of our projects, so if we make changes here, it makes sense to do something similar in the other projects too. I think that's also the reason the tools are now installed globally: they're needed in just about every project.

Another thing to take into account, and that seems somewhat related, is that we have shared CI workflows that also install tools with certain versions. It would be nice if 1) CI would install the same version as what we would install locally and 2) if we can somehow update the version of tools (centrally). I think 2 would come down to updating thetools.go dependencies using Dependabot, but it would still have to happen in each repository.

@marten-seemann
Copy link
ContributorAuthor

If we make changes to this, then I would prefer the tools.go method. I've used it myself before, and there's at least one other internal project where we make use of it. I believe you'll get a specific, hardcoded version there too, based on what's in go.mod.

Just wondering why that's a requirement to get this PR merged. As before, it uses the@latest version of the tools, but doesn't require them to be installed in the global path. So it seems like that's an improvement already, isn't it?

Note that much of the Makefile is cloned in pretty much all of our projects, so if we make changes here, it makes sense to do something similar in the other projects too. I think that's also the reason the tools are now installed globally: they're needed in just about every project.

Happy to propagate the change to other repos, once we've agreed on how to proceed here.

I think 2 would come down to updating the tools.go dependencies using Dependabot, but it would still have to happen in each repository.

I added thego.mod method in88b6b38, but I'm not sure it's an improvement. It massively inflatesgo.mod (especially golangci-lint).

@hslatman
Copy link
Member

Just wondering why that's a requirement to get this PR merged. As before, it uses the@latest version of the tools, but doesn't require them to be installed in the global path. So it seems like that's an improvement already, isn't it?

There are tools (e.g.golangci-lint) for which we depend on a specific version through a shared workflow. If we're going to change things, then imo we would like these to be the same, and it could be different if we use@latest.

I added thego.mod method in88b6b38, but I'm not sure it's an improvement. It massively inflatesgo.mod (especially golangci-lint).

I'll let others chime in on this.@azazeal,@maraino,@dopey thoughts on this?

@azazeal
Copy link
Contributor

Just wondering why that's a requirement to get this PR merged. As before, it uses the@latest version of the tools, but doesn't require them to be installed in the global path. So it seems like that's an improvement already, isn't it?

There are tools (e.g.golangci-lint) for which we depend on a specific version through a shared workflow. If we're going to change things, then imo we would like these to be the same, and it could be different if we use@latest.

I added thego.mod method in88b6b38, but I'm not sure it's an improvement. It massively inflatesgo.mod (especially golangci-lint).

I'll let others chime in on this.@azazeal,@maraino,@dopey thoughts on this?

We should be using the version of the tools defined ingo.mod. So, when trying to install those viaMakefile, we should be opting forgo install -mod=readonly #{tool}.

@marten-seemann
Copy link
ContributorAuthor

So, when trying to install those viaMakefile, we should be opting forgo install -mod=readonly #{tool}.

My point is: Whyinstall any tools on the system anyway? We can justrun the tools, using whatever version we like, without touching the system configuration at all.

Imagine the following situation: A developer has golangci-lint version X installed on their system for his work on a non-Smallstep project. Now they want to build the cli. We shouldn't change their system configuration by installing golangci-lint version Y.

Copy link
Contributor

@azazealazazeal left a comment

Choose a reason for hiding this comment

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

Looks reasonable. Don't know ifgo run -mod=readonly ... would be a bad idea or not. Perhaps it might be required.


govulncheck:
$Q govulncheck ./...
$Qgo run golang.org/x/vuln/cmd/govulncheck@latest ./...
Copy link
Contributor

Choose a reason for hiding this comment

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

This shouldn't be using the@latest tag, I think.

@azazeal
Copy link
Contributor

azazeal commentedJun 24, 2024
edited
Loading

So, when trying to install those viaMakefile, we should be opting forgo install -mod=readonly #{tool}.

My point is: Whyinstall any tools on the system anyway? We can justrun the tools, using whatever version we like, without touching the system configuration at all.

Imagine the following situation: A developer has golangci-lint version X installed on their system for his work on a non-Smallstep project. Now they want to build the cli. We shouldn't change their system configuration by installing golangci-lint version Y.

I'm not advocating that we must install said tools. I'm saying thatwhen trying to install them, we should be respectful of their versions in go.mod. Obviously, if we don't need to install them, and we can simply run them viago run -mod=readonly, that's absolutely fine with me.

There may be patterns lying around in these repos,@marten-seemann, that were written (or carried over) beforego.mod was a thing. This does not mean that we intend to do things in a way that'd complicate things for others. Additionally, when something changes in our GitHub workflows, this does not necessarily mean that the same change will find itself reflected into theMakefile.

Overall, this PR looks reasonable assuming that the-mod=readonly flag is indeed optional and that last@latest tag is removed.

Edit: There might be some muscle memory that needs to be retrained for those in the team that are used to typingmake bootstrap, but I think, overall, this is a welcome change.

Edit 2: The CI build doesn't have the same behaviorref.

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@azazealazazealazazeal left review comments

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

needs triageWaiting for discussion / prioritization by team

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

make-ing requires bootstrapping, which globally installs various Go tools

4 participants

@marten-seemann@CLAassistant@hslatman@azazeal

[8]ページ先頭

©2009-2026 Movatter.jp