Enable architecture specific dependencies #622
Conversation
|
@paketo-buildpacks/tooling-maintainers Could someone have a look? That is a prerequisite for supporting multiarch in paketo-buildpacks/nodejs. |
|
Could you please rebase this PR so I can merge it in! |
| @@ -74,6 +74,9 @@ type Dependency struct { | |||
| // StripComponents behaves like the --strip-components flag on tar command | |||
| // removing the first n levels from the final decompression destination. | |||
| StripComponents int `toml:"strip-components"` | |||
|
|
|||
| // ARCH is the architecture of this dependency | |||
| Arch string `toml:"arch"` | |||
There was a problem hiding this comment.
There are several platforms, for example, pack cli has plenty of them when it comes to build and image https://github.com/buildpacks/pack/blob/5cca9c51a61cf1bb8b0b6ba479bbbf10a65f0377/internal/target/platform.go#L31-L47 For nodejs we have a subset of them https://github.com/nodejs/node/blob/main/BUILDING.md#platform-list which I think it would be good to start with. So my suggestion on this one would be to also add another variable called OS or operating-system which will indicate the operating system of the dependency
There was a problem hiding this comment.
I am no expert on the whole multiarch thing, but operating-system would be wrong, wouldn't it? The operating system is for all dependencies linux - it is the same run image after all. We "only" have to differ dependencies for the architecture of the image. So I thing anything other than Arch would be quite confusing, don't you think?
There was a problem hiding this comment.
I thought that we might need to support generating images for macos, but currently there is no such use case so makes sense to not over-complicate it. LGTM
| e.id, | ||
| e.version, | ||
| e.stack, | ||
| e.arch, | ||
| strings.Join(e.supportedVersions, ", "), |
There was a problem hiding this comment.
Same on this one we should add the os attribute
There was a problem hiding this comment.
If we dont use the os, this comment does not apply.
| @@ -115,6 +116,12 @@ strip-components = 1 | |||
| service = postal.NewService(transport). | |||
| WithDependencyMappingResolver(mappingResolver). | |||
| WithDependencyMirrorResolver(mirrorResolver) | |||
|
|
|||
| Expect(os.Setenv("BP_ARCH", "amd64")).To(Succeed()) | |||
There was a problem hiding this comment.
On this one, instead of having BP_ARCH and BP_OS, we could rename the BP_ARCH to something like BP_PLATFORM or BP_TARGET, which will include both architecture and operating system, similar to what pack cli does with the targets.
There was a problem hiding this comment.
If we dont use the os, this comment does not apply.
| return systemArch == dep.Arch | ||
| } | ||
|
|
||
| func archFromSystem() string { |
There was a problem hiding this comment.
In terms of clean code, one function should only do one thing, I would remove from this function, the two things that is doing 1. looking if the environment variable exists 2. fetching the system architecture. Changing that function to fetching only the architecture does not make sense as there is already a function for that. Therefore, I would suggest to move this logic out in to the Parent function (Resolve) as I think this is totally normal as there is already logic for validating other things like (dependency version and stacks). Althouh, if you decide to fetch the platform, then it would be good to wrap these two lines in a function called getSystemPlatform as it will be more than one line (one for fetching the GOARCH and one for fetching the GOOS
There was a problem hiding this comment.
I think we dont want to default into the system that the user is using as pack currently fallbacks to linux/amd64, so I would choose this platform as default, in order to avoid any errors from macos users that currently building applications
There was a problem hiding this comment.
I think we dont want to default into the system
I might be completely wrong, but that is not what is done. It looks up the architecture of the runtime, meaning the buildpack that actually called getDependency(). So no other architecture could even work. The BP_ARCH is only needed for unit tests.
I tried to keep everything as similar to libak as possible.
There was a problem hiding this comment.
I see, so seems that it is not breaking the default behavior. LGTM
I will update, but wait a bit with the merge so that @pacostas can respond to the comments. |
c0d1ngm0nk3y
force-pushed
the
arch-dep
branch
from
db0fe58 to
fec4819
Compare
LGTM |
fixes #618
Summary
When looking for an dependency, the architecture is taken into account. The default architecture (if not provided by a dependency is "amd64" to ensure backwards compatibility. The architecture to look for is taken from the system or an env variable
BP_ARCHto enable unit testing.Use Cases
Dependencies for several architectures (e.g. "arm" and "amd64")
Checklist