Enable architecture specific dependencies #622
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,8 +4,10 @@ import ( | |
| "errors" | ||
| "fmt" | ||
| "io" | ||
| "os" | ||
| "path/filepath" | ||
| "regexp" | ||
| "runtime" | ||
| "sort" | ||
| "strings" | ||
| "time" | ||
|
|
@@ -52,15 +54,17 @@ type ErrNoDeps struct { | |
| id string | ||
| version string | ||
| stack string | ||
| arch string | ||
| supportedVersions []string | ||
| } | ||
|
|
||
| // Error implements the error.Error interface | ||
| func (e *ErrNoDeps) Error() string { | ||
| return fmt.Sprintf("failed to satisfy %q dependency version constraint %q: no compatible versions on %q stack with architecture %q. Supported versions are: [%s]", | ||
| 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. |
||
| ) | ||
| } | ||
|
|
@@ -147,6 +151,10 @@ func (s Service) Resolve(path, id, version, stack string) (Dependency, error) { | |
| continue | ||
| } | ||
|
|
||
| if !isCorrectArch(dependency) { | ||
| continue | ||
| } | ||
|
|
||
| sVersion, err := semver.NewVersion(dependency.Version) | ||
| if err != nil { | ||
| return Dependency{}, err | ||
|
|
@@ -160,7 +168,7 @@ func (s Service) Resolve(path, id, version, stack string) (Dependency, error) { | |
| } | ||
|
|
||
| if len(compatibleVersions) == 0 { | ||
| return Dependency{}, &ErrNoDeps{id, version, stack, archFromSystem(), supportedVersions} | ||
| } | ||
|
|
||
| stacksForVersion := map[string][]string{} | ||
|
|
@@ -220,6 +228,25 @@ func (s Service) Resolve(path, id, version, stack string) (Dependency, error) { | |
| return compatibleVersions[0], nil | ||
| } | ||
|
|
||
| func isCorrectArch(dep Dependency) bool { | ||
| systemArch := archFromSystem() | ||
|
|
||
| if systemArch == "amd64" && dep.Arch == "" { | ||
| return true | ||
| } | ||
|
|
||
| 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 ( 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 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 I tried to keep everything as similar to There was a problem hiding this comment. I see, so seems that it is not breaking the default behavior. LGTM |
||
| archFromEnv, ok := os.LookupEnv("BP_ARCH") | ||
| if !ok { | ||
| archFromEnv = runtime.GOARCH | ||
| } | ||
|
|
||
| return archFromEnv | ||
| } | ||
|
|
||
| func stringSliceContains(slice []string, str string) bool { | ||
| for _, s := range slice { | ||
| if s == str { | ||
|
|
||
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.
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
OSoroperating-systemwhich will indicate the operating system of the dependencyThere 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.
I am no expert on the whole multiarch thing, but
operating-systemwould be wrong, wouldn't it? The operating system is for all dependencieslinux- 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 thanArchwould be quite confusing, don't you think?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.
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