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

build: perform one-by-one build #718

Closed
wants to merge 1 commit into from
Closed

build: perform one-by-one build #718

wants to merge 1 commit into from

Conversation

rockdrilla
Copy link

What this PR does / why we need it:

  1. avoid duplicate package requests to ${GOPROXY} and ${GOSUMDB} - first binary target fetches all required packages and further binary builds reuse them.

  2. respect ${GOMAXPROCS} (if any).

@rockdrilla rockdrilla requested a review from a team as a code owner July 20, 2023 14:14
@rockdrilla rockdrilla requested review from wbrowne and xnyo and removed request for a team July 20, 2023 14:14
@CLAassistant
Copy link

CLAassistant commented Jul 20, 2023

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 our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

Copy link
Member

@xnyo xnyo left a comment

Choose a reason for hiding this comment

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

Hi and thank you for contributing to Grafana!

I have some comments about your change:

@@ -223,7 +223,7 @@ func (Build) Backend() error {
// BuildAll builds production executables for all supported platforms.
func BuildAll() { //revive:disable-line
b := Build{}
mg.Deps(b.Linux, b.Windows, b.Darwin, b.DarwinARM64, b.LinuxARM64, b.LinuxARM, b.GenerateManifestFile)
mg.SerialDeps(b.Linux, b.Windows, b.Darwin, b.DarwinARM64, b.LinuxARM64, b.LinuxARM, b.GenerateManifestFile)
Copy link
Member

Choose a reason for hiding this comment

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

I would keep the parallel execution to leverage all cores when building plugins, however your point about duplicate package requests is a fair one.

To address it, I would change it so fetching the packages happens as a separate step before building the executable, which is then done in parallel, like so:

// GoModDownload downloads the packages required by the executable by running "go mod download".
func (Build) GoModDownload() error {
	return sh.Run("go", "mod", "download")
}

// BuildAll builds production executables for all supported platforms.
func BuildAll() { //revive:disable-line
	b := Build{}
	mg.Deps(b.GoModDownload)
	mg.Deps(b.Linux, b.Windows, b.Darwin, b.DarwinARM64, b.LinuxARM64, b.LinuxARM, b.GenerateManifestFile)
}

Copy link
Author

Choose a reason for hiding this comment

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

Hi again!

Each binary target does go mod download under hood (if necessary) so separate download target isn't needed.

Furthermore, each binary target utilizes as much cores as possible (or at most ${GOMAXPROCS} if specified). Running all of them at once will throttle overall system/container.

@rockdrilla rockdrilla closed this by deleting the head repository Feb 10, 2024
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

3 participants