buildsys: add feature to vendor go modules #2053
Conversation
samuelkarp
force-pushed
the
buildsys-go-modules
branch
from
6d63722
to
74ac037
Compare
Signed-off-by: Samuel Karp <[email protected]>
samuelkarp
force-pushed
the
buildsys-go-modules
branch
from
74ac037
to
65bd339
Compare
Signed-off-by: Samuel Karp <[email protected]>
Signed-off-by: Samuel Karp <[email protected]>
samuelkarp
force-pushed
the
buildsys-go-modules
branch
from
65bd339
to
a7b439a
Compare
Added |
| [[package.metadata.build-package.go-mods]] | ||
| input = "hotdog-3f2ca92.tar.gz" | ||
| mod-dir = "hotdog-3f2ca9275fae8db87409c3a0999aa2c8a4bd44d1" | ||
| output-dir = "src" |
There was a problem hiding this comment.
I'd tend to prefer coming up with some standard contract and automating that in a way that's easy for spec files to consume, so there's less risk of each spec doing things in slightly different ways, which tends to obscure packaging mistakes.
Specifically, if instead of an output dir we created a tarball, then we could reference that archive in the spec file in the usual way - listing it as a Source, and using the %setup or %autosetup macro. The latter would be valuable for any patches we'd carry. There's also a modest performance benefit from not having lots of small files to copy into the build context.
Then we could specify the behavior into the external-files declaration like:
[[package.metadata.build-package.external-files]]
url = "https://github.com/bottlerocket-os/hotdog/archive/3f2ca9275fae8db87409c3a0999aa2c8a4bd44d1/hotdog-3f2ca92.tar.gz"
path = "override-upstream.tar.gz"
sha512 = "ebf8f2f8f2c7b12443eae5eccdf0d37724637ac972874b9a88eb8d21d88cbe661b0b53680015ebb8c06e17bde708acf90f39fac82002a4c72266ecdac87c3fb5"
bundle-modules = [ "go" ]
bundle-path = "override-upstream-bundled.tar.gz"
(Not entirely sure about what to name the output file but there's probably an automatic convention like x.tar.* -> x-bundled.tar.* that we could use.)
What do you think?
| %cross_go_configure %{goimport} | ||
| # We use `GO111MODULE=off` to force golang to look for the dependencies in the GOPATH | ||
| GO111MODULE=off go build -v -x -buildmode=pie -ldflags="${GOLDFLAGS}" -o oci-add-hooks | ||
| %set_cross_go_flags |
There was a problem hiding this comment.
Awesome! It'll be really nice to get rid of cross_go_setup and cross_go_configure at some point.
| if let Some(mods) = manifest.go_mods() { | ||
| GoMod::vendor(&root_dir, &manifest_dir, &mods).context(error::GoModSnafu)?; | ||
| } |
There was a problem hiding this comment.
Need to emit cargo:rerun-if-changed directives for the output files, or tarball if we switch to that.
| #[snafu(display("input must be a file and end with .tar.gz"))] | ||
| InputFileBad, |
There was a problem hiding this comment.
nit: we don't seem to check for the "ends with .tar.gz" part (and might not want to, since other compression formats might be used)
| match docker_go(root_dir, &args) { | ||
| Ok(_) => continue, | ||
| Err(e) => return Err(e), | ||
| } |
There was a problem hiding this comment.
This can be written more concisely as:
| match docker_go(root_dir, &args) { | |
| Ok(_) => continue, | |
| Err(e) => return Err(e), | |
| } | |
| let _ = docker_go(root_dir, &args)?; |
(Might be able to get rid of the let _ = part also.)
| root_dir: &PathBuf, | ||
| package_dir: &PathBuf, | ||
| gomods: &[manifest::GoModule], | ||
| ) -> Result<Self> { |
There was a problem hiding this comment.
Looks like you can just return nothing in the happy case:
| ) -> Result<Self> { | |
| ) -> Result<()> { |
| dg_args | ||
| .module_path | ||
| .to_str() | ||
| .context(error::InputFileSnafu) | ||
| .unwrap(), |
There was a problem hiding this comment.
Hmm why do we still need to unwrap() here?
Generally that would mean that we would add another .context(error::WhateverSnafu) to the chain.
| return if output.status.success() { | ||
| Ok(output) | ||
| } else { | ||
| Err(error::Error::DockerExecution { | ||
| args: "".to_string(), | ||
| }) | ||
| }; |
There was a problem hiding this comment.
| return if output.status.success() { | |
| Ok(output) | |
| } else { | |
| Err(error::Error::DockerExecution { | |
| args: "".to_string(), | |
| }) | |
| }; | |
| ensure!(output.status.success(), error::DockerExecution { args }); | |
| Ok(output) |
| return if output.status.success() { | ||
| Some(stdout.to_string()) | ||
| } else { | ||
| None | ||
| }; |
There was a problem hiding this comment.
Does this work?
| return if output.status.success() { | |
| Some(stdout.to_string()) | |
| } else { | |
| None | |
| }; | |
| output.status.success().then(|| stdout.to_string()) |
|
@bcressey Thanks for the detailed review here! I no longer work for Amazon and am not currently in a position to dedicate my own personal time or resources to moving this forward. I'd be happy for you or for someone else on the team to pick up my contribution and finish it. Please feel free to close this once a replacement PR is open. |
Issue number:
#2052
Description of changes:
This pull request implements Option C in #2052: changes to buildsys that enable automatic Go module downloads ahead of the rpmbuild invocation.
Testing done:
Launch an
aws-ecs-1AMI, verify that oci-add-hooks are built properly and have dependency licenses in/usr/share/licensesTerms of contribution:
By submitting this pull request, I agree that this contribution is dual-licensed under the terms of both the Apache License, version 2.0, and the MIT license.