-
Notifications
You must be signed in to change notification settings - Fork 67
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
Feat: Build manifest on every backend build #1191
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work! 🙌 Just a couple of small comments
build/common.go
Outdated
// TODO: Change to sh.RunWithV once available. | ||
return sh.RunWith(cfg.Env, "go", args...) | ||
err = sh.RunWith(cfg.Env, "go", args...) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit, unrelated: the comment above specifies "TODO: Change to sh.RunWithV once available.", it is now available, maybe we can kill two birds with one stone? 🙂
// TODO: Change to sh.RunWithV once available. | |
return sh.RunWith(cfg.Env, "go", args...) | |
err = sh.RunWith(cfg.Env, "go", args...) | |
err = sh.RunWithV(cfg.Env, "go", args...) |
Another comment: if we're worried about performance and we want to make sure that the manifest is generated only once, we can wrap diff --git a/build/common.go b/build/common.go
index c104d30..40d0972 100644
--- a/build/common.go
+++ b/build/common.go
@@ -252,8 +252,7 @@ func (Build) DarwinARM64() error {
return buildBackend(newBuildConfig("darwin", "arm64"))
}
-// GenerateManifestFile generates a manifest file for plugin submissions
-func (Build) GenerateManifestFile() error {
+var generateManifestOnce = sync.OnceValue(func() error {
config := Config{}
config, err := beforeBuild(config)
if err != nil {
@@ -279,6 +278,11 @@ func (Build) GenerateManifestFile() error {
return err
}
return nil
+})
+
+// GenerateManifestFile generates a manifest file for plugin submissions
+func (Build) GenerateManifestFile() error {
+ return generateManifestOnce()
}
// Debug builds the debug version for the current platform. However what we have right now is more readable and not a concern, so unless there are other issues down the line I'd keep it like this |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! 🚀
What this PR does / why we need it:
This PR is adding the build manifest step after every backend build. This would simplify the build process for specific architecture and make selecting specific architectures in the plugin-actions easier
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
The drawback is that in the
buildAll
target the manifest will be built multiple times