From d543d173242127713c007a03af9c90cac560b700 Mon Sep 17 00:00:00 2001 From: WYGIN Date: Mon, 25 Sep 2023 17:33:06 +0530 Subject: [PATCH 01/13] added targets flag for buildpack new cli Signed-off-by: WYGIN --- internal/commands/buildpack_new.go | 21 +++++++++++++++++++++ pkg/client/new_buildpack.go | 8 ++++++-- 2 files changed, 27 insertions(+), 2 deletions(-) diff --git a/internal/commands/buildpack_new.go b/internal/commands/buildpack_new.go index d3d7d6d43a..a7b0f2da33 100644 --- a/internal/commands/buildpack_new.go +++ b/internal/commands/buildpack_new.go @@ -9,6 +9,7 @@ import ( "github.com/spf13/cobra" + "github.com/buildpacks/lifecycle/buildpack" "github.com/buildpacks/pack/internal/style" "github.com/buildpacks/pack/pkg/client" "github.com/buildpacks/pack/pkg/dist" @@ -19,7 +20,9 @@ import ( type BuildpackNewFlags struct { API string Path string + // Deprecated: use `targets` instead Stacks []string + Targets []buildpack.TargetMetadata Version string } @@ -66,11 +69,29 @@ func BuildpackNew(logger logging.Logger, creator BuildpackCreator) *cobra.Comman }) } + var targets []dist.Target + var distros []dist.Distribution + for _, t := range flags.Targets { + var versions []string + for _, d := range t.Distros { + distros = append(distros, dist.Distribution{ + Name: d.Name, + Versions: append(versions, d.Version), + }) + } + targets = append(targets, dist.Target{ + OS: t.OS, + Arch: t.Arch, + Distributions: distros, + }) + } + if err := creator.NewBuildpack(cmd.Context(), client.NewBuildpackOptions{ API: flags.API, ID: id, Path: path, Stacks: stacks, + Targets: targets, Version: flags.Version, }); err != nil { return err diff --git a/pkg/client/new_buildpack.go b/pkg/client/new_buildpack.go index cd23f75d30..9dde27063a 100644 --- a/pkg/client/new_buildpack.go +++ b/pkg/client/new_buildpack.go @@ -45,10 +45,13 @@ type NewBuildpackOptions struct { // The stacks this buildpack will work with Stacks []dist.Stack + + // the targets this buildpack will work with + Targets []dist.Target } func (c *Client) NewBuildpack(ctx context.Context, opts NewBuildpackOptions) error { - err := createBuildpackTOML(opts.Path, opts.ID, opts.Version, opts.API, opts.Stacks, c) + err := createBuildpackTOML(opts.Path, opts.ID, opts.Version, opts.API, opts.Stacks, opts.Targets, c) if err != nil { return err } @@ -94,7 +97,7 @@ func createBinScript(path, name, contents string, c *Client) error { return nil } -func createBuildpackTOML(path, id, version, apiStr string, stacks []dist.Stack, c *Client) error { +func createBuildpackTOML(path, id, version, apiStr string, stacks []dist.Stack, targets []dist.Target, c *Client) error { api, err := api.NewVersion(apiStr) if err != nil { return err @@ -103,6 +106,7 @@ func createBuildpackTOML(path, id, version, apiStr string, stacks []dist.Stack, buildpackTOML := dist.BuildpackDescriptor{ WithAPI: api, WithStacks: stacks, + WithTargets: targets, WithInfo: dist.ModuleInfo{ ID: id, Version: version, From 96b28c3bb196c979db3824fa1182154e63aed607 Mon Sep 17 00:00:00 2001 From: WYGIN Date: Tue, 26 Sep 2023 16:44:53 +0530 Subject: [PATCH 02/13] added format to add --targets from cli Signed-off-by: WYGIN --- internal/commands/buildpack_new.go | 25 ++-- pkg/buildpack/buildpack.go | 2 +- pkg/client/build.go | 2 +- pkg/client/new_buildpack.go | 6 +- pkg/dist/buildmodule.go | 190 ++++++++++++++++++++++++++++- pkg/dist/buildpack_descriptor.go | 4 +- pkg/dist/extension_descriptor.go | 2 +- pkg/dist/layers.go | 2 +- 8 files changed, 211 insertions(+), 22 deletions(-) diff --git a/internal/commands/buildpack_new.go b/internal/commands/buildpack_new.go index a7b0f2da33..bd09f305d1 100644 --- a/internal/commands/buildpack_new.go +++ b/internal/commands/buildpack_new.go @@ -9,7 +9,7 @@ import ( "github.com/spf13/cobra" - "github.com/buildpacks/lifecycle/buildpack" + "github.com/buildpacks/pack/internal/style" "github.com/buildpacks/pack/pkg/client" "github.com/buildpacks/pack/pkg/dist" @@ -22,7 +22,7 @@ type BuildpackNewFlags struct { Path string // Deprecated: use `targets` instead Stacks []string - Targets []buildpack.TargetMetadata + Targets dist.Targets Version string } @@ -69,20 +69,13 @@ func BuildpackNew(logger logging.Logger, creator BuildpackCreator) *cobra.Comman }) } - var targets []dist.Target - var distros []dist.Distribution + var targets dist.Targets for _, t := range flags.Targets { - var versions []string - for _, d := range t.Distros { - distros = append(distros, dist.Distribution{ - Name: d.Name, - Versions: append(versions, d.Version), - }) - } targets = append(targets, dist.Target{ OS: t.OS, Arch: t.Arch, - Distributions: distros, + ArchVariant: t.ArchVariant, + Distributions: t.Distributions, }) } @@ -106,6 +99,14 @@ func BuildpackNew(logger logging.Logger, creator BuildpackCreator) *cobra.Comman cmd.Flags().StringVarP(&flags.Path, "path", "p", "", "Path to generate the buildpack") cmd.Flags().StringVarP(&flags.Version, "version", "V", "1.0.0", "Version of the generated buildpack") cmd.Flags().StringSliceVarP(&flags.Stacks, "stacks", "s", []string{"io.buildpacks.stacks.jammy"}, "Stack(s) this buildpack will be compatible with"+stringSliceHelp("stack")) + cmd.Flags().MarkDeprecated("stacks", "stacks is deprecated in the favor of `targets`") + cmd.Flags().Var(&flags.Targets, "targets", + `Targets is a list of platforms that you wish to support. one can provide target platforms in format [os][/arch][/variant]:[name@osversion] +- Base case for two different architectures : '--targets "linux/amd64" --targets "linux/arm64"' +- case for distribution versions: '--targets "windows/amd64:windows-nano@10.0.19041.1415"' +- case for different architecture with distrubuted versions : '--targets "linux/arm/v6:ubuntu@14.04" --targets "linux/arm/v6:ubuntu@16.04"' + - If no name is provided, a random name will be generated. +`) AddHelpFlag(cmd, "new") return cmd diff --git a/pkg/buildpack/buildpack.go b/pkg/buildpack/buildpack.go index 182a3c6d0f..cbdcd60309 100644 --- a/pkg/buildpack/buildpack.go +++ b/pkg/buildpack/buildpack.go @@ -41,7 +41,7 @@ type Descriptor interface { Kind() string Order() dist.Order Stacks() []dist.Stack - Targets() []dist.Target + Targets() dist.Targets } type Blob interface { diff --git a/pkg/client/build.go b/pkg/client/build.go index c7c35465ec..1f637a7ebf 100644 --- a/pkg/client/build.go +++ b/pkg/client/build.go @@ -1307,7 +1307,7 @@ func createInlineBuildpack(bp projectTypes.Buildpack, stackID string) (string, e bp.Version = "0.0.0" } - if err = createBuildpackTOML(pathToInlineBuilpack, bp.ID, bp.Version, bp.Script.API, []dist.Stack{{ID: stackID}}, nil); err != nil { + if err = createBuildpackTOML(pathToInlineBuilpack, bp.ID, bp.Version, bp.Script.API, []dist.Stack{{ID: stackID}}, dist.Targets{}, nil); err != nil { return pathToInlineBuilpack, err } diff --git a/pkg/client/new_buildpack.go b/pkg/client/new_buildpack.go index 9dde27063a..dacd2a6a99 100644 --- a/pkg/client/new_buildpack.go +++ b/pkg/client/new_buildpack.go @@ -43,11 +43,11 @@ type NewBuildpackOptions struct { // version of the output buildpack artifact. Version string - // The stacks this buildpack will work with + // Deprecated: The stacks this buildpack will work with Stacks []dist.Stack // the targets this buildpack will work with - Targets []dist.Target + Targets dist.Targets } func (c *Client) NewBuildpack(ctx context.Context, opts NewBuildpackOptions) error { @@ -97,7 +97,7 @@ func createBinScript(path, name, contents string, c *Client) error { return nil } -func createBuildpackTOML(path, id, version, apiStr string, stacks []dist.Stack, targets []dist.Target, c *Client) error { +func createBuildpackTOML(path, id, version, apiStr string, stacks []dist.Stack, targets dist.Targets, c *Client) error { api, err := api.NewVersion(apiStr) if err != nil { return err diff --git a/pkg/dist/buildmodule.go b/pkg/dist/buildmodule.go index ec3dc99ecd..883687d2da 100644 --- a/pkg/dist/buildmodule.go +++ b/pkg/dist/buildmodule.go @@ -1,6 +1,8 @@ package dist import ( + "strings" + "github.com/pkg/errors" "github.com/buildpacks/pack/internal/style" @@ -55,10 +57,196 @@ type Stack struct { type Target struct { OS string `json:"os" toml:"os"` Arch string `json:"arch" toml:"arch"` - Distributions []Distribution `json:"distributions,omitempty" toml:"distributions,omitempty"` + ArchVariant string `json:"variant,omitempty" toml:"variant"` + Distributions Distributions `json:"distributions,omitempty" toml:"distributions,omitempty"` +} + +type Targets []Target + +func(*Targets) Type() string { + return "Targets" +} + +func(targets *Targets) String() string { + var sb strings.Builder + // each target is converted to string & is separated by `,` + for _,t := range *targets { + sb.WriteString(t.String() + ",") + } + return sb.String() +} + +// set the value of multiple targets in a single command where each target is separated by a `,` or pass multiple `--targets` to specify each target individually. +func(targets *Targets) Set(value string) error { + // spliting multiple targets into a slice of targets by `,` separater + s := strings.Split(value, ",") + for i,v := range *targets { + // for each individual target set it's value from s[i] & getting an error + err := v.Set(s[i]) + // if error is not nill return error + if err != nil { + return err + } + } + return nil +} + +func(target *Target) Type() string { + return "Target" +} + +// `--target` is passed in the format of `[os][/arch][/variant]:[name@osversion]` +// one can specify multiple distro versions in the format `name@osversion1@osversion2@...` +// one can specify multiple distros seperated by `;` +// example: +// `name1@osverion1;name2@osversion2;...` +func(target *Target) String() string { + var s string + // if target's OS is not nill append it to string + if target.OS != "" { + s += target.OS + } + // if target's Arch is not nill append it to string + if target.Arch != "" { + // if os is not defined append target's arch without '/' + if s == "" { + s += target.Arch + } + // if os is defined append target's arch separated by '/' + s += "/" + target.Arch + } + // if target's ArchVariant is not nill append it to string + if target.ArchVariant != "" { + // if os and/or arch is/are not defined append target's archVariant without '/' + if s == "" { + s += target.ArchVariant + } + // if os and/or arch is/are not defined append target's archVariant separated by '/' + s += "/" + target.ArchVariant + } + + // if distros are nill return the string `s` + if target.Distributions != nil { + // a map of distributions with distro's name as key and versions(slice of string) as value + var v map[string][]string + for _,d := range target.Distributions { + // for each distro add version to map with key as distro's name + v[d.Name] = append(v[d.Name], d.Versions...) + } + // after adding all distributions to map + // if [os] or [arch] or [archVariant] is defined append distro to string in format `:[name]@[version1]@[version2];[name1]@[version1]@[version2];` + if s == "" { + for k,d := range v { + if len(d) > 0 { + s += k + "@" + strings.Join(v[k], "@") + ";" + } + // if no version is specified append distro to string in format `[name];[name1];[name2];` without any version + s += k + ";" + } + } + // after adding all distributions to map + // if [os] or [arch] or [archVariant] is defined append distro to string in format `[name]@[version1]@[version2];[name1]@[version1]@[version2];` + s += ":" + for k,d := range v { + if len(d) > 0 { + s += k + "@" + strings.Join(v[k], "@") + ";" + } + s += k + ";" + } + } + // return the final stringified version of target + return s +} + +func(target *Target) Set(value string) error { + // each target can only have one `:` to separate [os] [arch] [archVariant] with distro + osDistro := strings.Split(value, ":") + if len(osDistro) > 2 { + return errors.Errorf("invalid format, target should not have more than one `:` separator. got %s number of separators", len(osDistro) - 1) + } + var os, distro string + + if len(osDistro) == 2 { + // get the [os][/arch][/archVariant] at index 0 if osDistro is of length 2 + os = osDistro[0] + // get [name@osversion] from index 1 if osDistro has length 2 + distro = osDistro[1] + } else { + // if osDistro is nil or with length 1 add value of osDistro[0] to + // assign to `os` variable if string has separator '/' + // else assign it to distro + if v := osDistro[0]; strings.ContainsRune(v, '/') { + os = v + } else { + distro = v + } + } + // separate [os] [arch] [archVariant] with separator '/' and assign values to their respective variable + osArch := strings.SplitN(os, "/", 3) + if os := osArch[0]; os != "" { + target.OS = os + } + if arch := osArch[1]; arch != "" { + target.Arch = arch + } + if archVariant := osArch[2]; archVariant != "" { + target.ArchVariant = archVariant + } + + // split the distros with the separator ';' + err := target.Distributions.Set(distro) + if err != nil { + return err + } + + return nil +} + +func(distros *Distributions) String() string { + var s strings.Builder + for _,d := range *distros { + s.WriteString(d.String() + ";") + } + return s.String() +} + +func(distros *Distributions) Set(value string) error { + v := strings.Split(value, ";") + for i,d := range *distros { + err := d.Set(v[i]) + if err != nil { + return err + } + } + return nil +} + +func(distros *Distributions) Type() string { + return "Distributions" +} + +func(distro *Distribution) String() string { + return distro.Name + strings.Join(distro.Versions, "@") +} + +func(distro *Distribution) Set(value string) error { + v := strings.Split(value, "@") + for i,d := range v { + if i == 0 { + distro.Name = d + } + distro.Versions = append(distro.Versions, d) + } + return nil +} + +func(distro *Distribution) Type() string { + return "Distribution" } type Distribution struct { Name string `json:"name,omitempty" toml:"name,omitempty"` Versions []string `json:"versions,omitempty" toml:"versions,omitempty"` } + +type Distributions []Distribution diff --git a/pkg/dist/buildpack_descriptor.go b/pkg/dist/buildpack_descriptor.go index 7ac46b97ba..c6011a9847 100644 --- a/pkg/dist/buildpack_descriptor.go +++ b/pkg/dist/buildpack_descriptor.go @@ -16,7 +16,7 @@ type BuildpackDescriptor struct { WithAPI *api.Version `toml:"api"` WithInfo ModuleInfo `toml:"buildpack"` WithStacks []Stack `toml:"stacks"` - WithTargets []Target `toml:"targets,omitempty"` + WithTargets Targets `toml:"targets,omitempty"` WithOrder Order `toml:"order"` WithWindowsBuild bool WithLinuxBuild bool @@ -134,7 +134,7 @@ func (b *BuildpackDescriptor) Stacks() []Stack { return b.WithStacks } -func (b *BuildpackDescriptor) Targets() []Target { +func (b *BuildpackDescriptor) Targets() Targets { return b.WithTargets } diff --git a/pkg/dist/extension_descriptor.go b/pkg/dist/extension_descriptor.go index b968ebf447..840439696a 100644 --- a/pkg/dist/extension_descriptor.go +++ b/pkg/dist/extension_descriptor.go @@ -43,6 +43,6 @@ func (e *ExtensionDescriptor) Stacks() []Stack { return nil } -func (e *ExtensionDescriptor) Targets() []Target { +func (e *ExtensionDescriptor) Targets() Targets { return nil } diff --git a/pkg/dist/layers.go b/pkg/dist/layers.go index 590454980e..3d50c0ba64 100644 --- a/pkg/dist/layers.go +++ b/pkg/dist/layers.go @@ -15,7 +15,7 @@ type Descriptor interface { Info() ModuleInfo Order() Order Stacks() []Stack - Targets() []Target + Targets() Targets } func LayerDiffID(layerTarPath string) (v1.Hash, error) { From 057f53dd962d4bad45d96f96a31c80acaf934944 Mon Sep 17 00:00:00 2001 From: WYGIN Date: Wed, 27 Sep 2023 10:39:22 +0530 Subject: [PATCH 03/13] refactored code to receive flags from []string for --targets flag Signed-off-by: WYGIN --- internal/commands/buildpack_new.go | 67 +++++++--- pkg/buildpack/buildpack.go | 2 +- pkg/client/build.go | 2 +- pkg/client/new_buildpack.go | 8 +- pkg/dist/buildmodule.go | 191 +---------------------------- pkg/dist/buildpack_descriptor.go | 4 +- pkg/dist/extension_descriptor.go | 2 +- pkg/dist/layers.go | 2 +- 8 files changed, 62 insertions(+), 216 deletions(-) diff --git a/internal/commands/buildpack_new.go b/internal/commands/buildpack_new.go index bd09f305d1..eac23c30c0 100644 --- a/internal/commands/buildpack_new.go +++ b/internal/commands/buildpack_new.go @@ -9,6 +9,7 @@ import ( "github.com/spf13/cobra" + "github.com/pkg/errors" "github.com/buildpacks/pack/internal/style" "github.com/buildpacks/pack/pkg/client" @@ -18,11 +19,11 @@ import ( // BuildpackNewFlags define flags provided to the BuildpackNew command type BuildpackNewFlags struct { - API string - Path string - // Deprecated: use `targets` instead + API string + Path string + // Deprecated: Stacks are deprecated Stacks []string - Targets dist.Targets + Targets []string Version string } @@ -69,13 +70,37 @@ func BuildpackNew(logger logging.Logger, creator BuildpackCreator) *cobra.Comman }) } - var targets dist.Targets + var targets []dist.Target for _, t := range flags.Targets { + var distroMap []dist.Distribution + var nonDistro []string + var distros []string + target := strings.Split(t, ":") + if i, e := getSliceAt[string](target, 0); e != nil { + logger.Errorf("invalid target %s, atleast one of [os][/arch][/archVariant] must be specified", t) + } else { + nonDistro = strings.Split(i, "/") + } + for _, d := range distros { + distro := strings.Split(d, "@") + if l := len(distro); l <= 0 { + logger.Error("distro is nil!") + } else if l == 1 { + logger.Warnf("forgot to specify version for distro %s ?", distro[0]) + } + distroMap = append(distroMap, dist.Distribution{ + Name: distro[0], + Versions: distro[1:], + }) + } + os, _ := getSliceAt[string](nonDistro, 0) + arch, _ := getSliceAt[string](nonDistro, 1) + variant, _ := getSliceAt[string](nonDistro, 2) targets = append(targets, dist.Target{ - OS: t.OS, - Arch: t.Arch, - ArchVariant: t.ArchVariant, - Distributions: t.Distributions, + OS: os, + Arch: arch, + ArchVariant: variant, + Distributions: distroMap, }) } @@ -99,15 +124,23 @@ func BuildpackNew(logger logging.Logger, creator BuildpackCreator) *cobra.Comman cmd.Flags().StringVarP(&flags.Path, "path", "p", "", "Path to generate the buildpack") cmd.Flags().StringVarP(&flags.Version, "version", "V", "1.0.0", "Version of the generated buildpack") cmd.Flags().StringSliceVarP(&flags.Stacks, "stacks", "s", []string{"io.buildpacks.stacks.jammy"}, "Stack(s) this buildpack will be compatible with"+stringSliceHelp("stack")) - cmd.Flags().MarkDeprecated("stacks", "stacks is deprecated in the favor of `targets`") - cmd.Flags().Var(&flags.Targets, "targets", - `Targets is a list of platforms that you wish to support. one can provide target platforms in format [os][/arch][/variant]:[name@osversion] -- Base case for two different architectures : '--targets "linux/amd64" --targets "linux/arm64"' -- case for distribution versions: '--targets "windows/amd64:windows-nano@10.0.19041.1415"' -- case for different architecture with distrubuted versions : '--targets "linux/arm/v6:ubuntu@14.04" --targets "linux/arm/v6:ubuntu@16.04"' - - If no name is provided, a random name will be generated. -`) + cmd.Flags().MarkDeprecated("stacks", "") + cmd.Flags().StringSliceVarP(&flags.Targets, "targets", "t", []string{"/"}, + `Targets are the list platforms that one targeting, these are generated as part of scaffolding inside buildpack.toml file. one can provide target platforms in format [os][/arch][/variant]:[distroname@osversion@anotherversion];[distroname@osversion] + - Base case for two different architectures : '--targets "linux/amd64" --targets "linux/arm64"' + - case for distribution version: '--targets "windows/amd64:windows-nano@10.0.19041.1415"' + - case for different architecture with distributed versions : '--targets "linux/arm/v6:ubuntu@14.04" --targets "linux/arm/v6:ubuntu@16.04"' + `) AddHelpFlag(cmd, "new") return cmd } + +func getSliceAt[T interface{}](slice []T, index int) (T, error) { + if index < 0 || index >= len(slice) { + var r T + return r, errors.Errorf("index out of bound, cannot access item at index %d of slice with length %d", index, len(slice)) + } + + return slice[index], nil +} diff --git a/pkg/buildpack/buildpack.go b/pkg/buildpack/buildpack.go index cbdcd60309..182a3c6d0f 100644 --- a/pkg/buildpack/buildpack.go +++ b/pkg/buildpack/buildpack.go @@ -41,7 +41,7 @@ type Descriptor interface { Kind() string Order() dist.Order Stacks() []dist.Stack - Targets() dist.Targets + Targets() []dist.Target } type Blob interface { diff --git a/pkg/client/build.go b/pkg/client/build.go index 1f637a7ebf..058dd89c54 100644 --- a/pkg/client/build.go +++ b/pkg/client/build.go @@ -1307,7 +1307,7 @@ func createInlineBuildpack(bp projectTypes.Buildpack, stackID string) (string, e bp.Version = "0.0.0" } - if err = createBuildpackTOML(pathToInlineBuilpack, bp.ID, bp.Version, bp.Script.API, []dist.Stack{{ID: stackID}}, dist.Targets{}, nil); err != nil { + if err = createBuildpackTOML(pathToInlineBuilpack, bp.ID, bp.Version, bp.Script.API, []dist.Stack{{ID: stackID}}, []dist.Target{}, nil); err != nil { return pathToInlineBuilpack, err } diff --git a/pkg/client/new_buildpack.go b/pkg/client/new_buildpack.go index dacd2a6a99..d5648dbda7 100644 --- a/pkg/client/new_buildpack.go +++ b/pkg/client/new_buildpack.go @@ -47,7 +47,7 @@ type NewBuildpackOptions struct { Stacks []dist.Stack // the targets this buildpack will work with - Targets dist.Targets + Targets []dist.Target } func (c *Client) NewBuildpack(ctx context.Context, opts NewBuildpackOptions) error { @@ -97,15 +97,15 @@ func createBinScript(path, name, contents string, c *Client) error { return nil } -func createBuildpackTOML(path, id, version, apiStr string, stacks []dist.Stack, targets dist.Targets, c *Client) error { +func createBuildpackTOML(path, id, version, apiStr string, stacks []dist.Stack, targets []dist.Target, c *Client) error { api, err := api.NewVersion(apiStr) if err != nil { return err } buildpackTOML := dist.BuildpackDescriptor{ - WithAPI: api, - WithStacks: stacks, + WithAPI: api, + WithStacks: stacks, WithTargets: targets, WithInfo: dist.ModuleInfo{ ID: id, diff --git a/pkg/dist/buildmodule.go b/pkg/dist/buildmodule.go index 883687d2da..5126d988e0 100644 --- a/pkg/dist/buildmodule.go +++ b/pkg/dist/buildmodule.go @@ -1,8 +1,6 @@ package dist import ( - "strings" - "github.com/pkg/errors" "github.com/buildpacks/pack/internal/style" @@ -57,196 +55,11 @@ type Stack struct { type Target struct { OS string `json:"os" toml:"os"` Arch string `json:"arch" toml:"arch"` - ArchVariant string `json:"variant,omitempty" toml:"variant"` - Distributions Distributions `json:"distributions,omitempty" toml:"distributions,omitempty"` -} - -type Targets []Target - -func(*Targets) Type() string { - return "Targets" -} - -func(targets *Targets) String() string { - var sb strings.Builder - // each target is converted to string & is separated by `,` - for _,t := range *targets { - sb.WriteString(t.String() + ",") - } - return sb.String() -} - -// set the value of multiple targets in a single command where each target is separated by a `,` or pass multiple `--targets` to specify each target individually. -func(targets *Targets) Set(value string) error { - // spliting multiple targets into a slice of targets by `,` separater - s := strings.Split(value, ",") - for i,v := range *targets { - // for each individual target set it's value from s[i] & getting an error - err := v.Set(s[i]) - // if error is not nill return error - if err != nil { - return err - } - } - return nil -} - -func(target *Target) Type() string { - return "Target" -} - -// `--target` is passed in the format of `[os][/arch][/variant]:[name@osversion]` -// one can specify multiple distro versions in the format `name@osversion1@osversion2@...` -// one can specify multiple distros seperated by `;` -// example: -// `name1@osverion1;name2@osversion2;...` -func(target *Target) String() string { - var s string - // if target's OS is not nill append it to string - if target.OS != "" { - s += target.OS - } - // if target's Arch is not nill append it to string - if target.Arch != "" { - // if os is not defined append target's arch without '/' - if s == "" { - s += target.Arch - } - // if os is defined append target's arch separated by '/' - s += "/" + target.Arch - } - // if target's ArchVariant is not nill append it to string - if target.ArchVariant != "" { - // if os and/or arch is/are not defined append target's archVariant without '/' - if s == "" { - s += target.ArchVariant - } - // if os and/or arch is/are not defined append target's archVariant separated by '/' - s += "/" + target.ArchVariant - } - - // if distros are nill return the string `s` - if target.Distributions != nil { - // a map of distributions with distro's name as key and versions(slice of string) as value - var v map[string][]string - for _,d := range target.Distributions { - // for each distro add version to map with key as distro's name - v[d.Name] = append(v[d.Name], d.Versions...) - } - // after adding all distributions to map - // if [os] or [arch] or [archVariant] is defined append distro to string in format `:[name]@[version1]@[version2];[name1]@[version1]@[version2];` - if s == "" { - for k,d := range v { - if len(d) > 0 { - s += k + "@" + strings.Join(v[k], "@") + ";" - } - // if no version is specified append distro to string in format `[name];[name1];[name2];` without any version - s += k + ";" - } - } - // after adding all distributions to map - // if [os] or [arch] or [archVariant] is defined append distro to string in format `[name]@[version1]@[version2];[name1]@[version1]@[version2];` - s += ":" - for k,d := range v { - if len(d) > 0 { - s += k + "@" + strings.Join(v[k], "@") + ";" - } - s += k + ";" - } - } - // return the final stringified version of target - return s -} - -func(target *Target) Set(value string) error { - // each target can only have one `:` to separate [os] [arch] [archVariant] with distro - osDistro := strings.Split(value, ":") - if len(osDistro) > 2 { - return errors.Errorf("invalid format, target should not have more than one `:` separator. got %s number of separators", len(osDistro) - 1) - } - var os, distro string - - if len(osDistro) == 2 { - // get the [os][/arch][/archVariant] at index 0 if osDistro is of length 2 - os = osDistro[0] - // get [name@osversion] from index 1 if osDistro has length 2 - distro = osDistro[1] - } else { - // if osDistro is nil or with length 1 add value of osDistro[0] to - // assign to `os` variable if string has separator '/' - // else assign it to distro - if v := osDistro[0]; strings.ContainsRune(v, '/') { - os = v - } else { - distro = v - } - } - // separate [os] [arch] [archVariant] with separator '/' and assign values to their respective variable - osArch := strings.SplitN(os, "/", 3) - if os := osArch[0]; os != "" { - target.OS = os - } - if arch := osArch[1]; arch != "" { - target.Arch = arch - } - if archVariant := osArch[2]; archVariant != "" { - target.ArchVariant = archVariant - } - - // split the distros with the separator ';' - err := target.Distributions.Set(distro) - if err != nil { - return err - } - - return nil -} - -func(distros *Distributions) String() string { - var s strings.Builder - for _,d := range *distros { - s.WriteString(d.String() + ";") - } - return s.String() -} - -func(distros *Distributions) Set(value string) error { - v := strings.Split(value, ";") - for i,d := range *distros { - err := d.Set(v[i]) - if err != nil { - return err - } - } - return nil -} - -func(distros *Distributions) Type() string { - return "Distributions" -} - -func(distro *Distribution) String() string { - return distro.Name + strings.Join(distro.Versions, "@") -} - -func(distro *Distribution) Set(value string) error { - v := strings.Split(value, "@") - for i,d := range v { - if i == 0 { - distro.Name = d - } - distro.Versions = append(distro.Versions, d) - } - return nil -} - -func(distro *Distribution) Type() string { - return "Distribution" + ArchVariant string `json:"variant,omitempty" toml:"variant,omitempty"` + Distributions []Distribution `json:"distributions,omitempty" toml:"distributions,omitempty"` } type Distribution struct { Name string `json:"name,omitempty" toml:"name,omitempty"` Versions []string `json:"versions,omitempty" toml:"versions,omitempty"` } - -type Distributions []Distribution diff --git a/pkg/dist/buildpack_descriptor.go b/pkg/dist/buildpack_descriptor.go index c6011a9847..7ac46b97ba 100644 --- a/pkg/dist/buildpack_descriptor.go +++ b/pkg/dist/buildpack_descriptor.go @@ -16,7 +16,7 @@ type BuildpackDescriptor struct { WithAPI *api.Version `toml:"api"` WithInfo ModuleInfo `toml:"buildpack"` WithStacks []Stack `toml:"stacks"` - WithTargets Targets `toml:"targets,omitempty"` + WithTargets []Target `toml:"targets,omitempty"` WithOrder Order `toml:"order"` WithWindowsBuild bool WithLinuxBuild bool @@ -134,7 +134,7 @@ func (b *BuildpackDescriptor) Stacks() []Stack { return b.WithStacks } -func (b *BuildpackDescriptor) Targets() Targets { +func (b *BuildpackDescriptor) Targets() []Target { return b.WithTargets } diff --git a/pkg/dist/extension_descriptor.go b/pkg/dist/extension_descriptor.go index 840439696a..b968ebf447 100644 --- a/pkg/dist/extension_descriptor.go +++ b/pkg/dist/extension_descriptor.go @@ -43,6 +43,6 @@ func (e *ExtensionDescriptor) Stacks() []Stack { return nil } -func (e *ExtensionDescriptor) Targets() Targets { +func (e *ExtensionDescriptor) Targets() []Target { return nil } diff --git a/pkg/dist/layers.go b/pkg/dist/layers.go index 3d50c0ba64..590454980e 100644 --- a/pkg/dist/layers.go +++ b/pkg/dist/layers.go @@ -15,7 +15,7 @@ type Descriptor interface { Info() ModuleInfo Order() Order Stacks() []Stack - Targets() Targets + Targets() []Target } func LayerDiffID(layerTarPath string) (v1.Hash, error) { From 16b83501b1ffde032eb341fa487f87ce33b43a37 Mon Sep 17 00:00:00 2001 From: WYGIN Date: Wed, 27 Sep 2023 16:34:43 +0530 Subject: [PATCH 04/13] fix: bug of distro not being added to toml file Signed-off-by: WYGIN --- internal/commands/buildpack_new.go | 7 ++++++- internal/commands/buildpack_new_test.go | 9 +++++++++ pkg/dist/buildpack_descriptor.go | 2 +- 3 files changed, 16 insertions(+), 2 deletions(-) diff --git a/internal/commands/buildpack_new.go b/internal/commands/buildpack_new.go index eac23c30c0..bab84673fa 100644 --- a/internal/commands/buildpack_new.go +++ b/internal/commands/buildpack_new.go @@ -81,6 +81,11 @@ func BuildpackNew(logger logging.Logger, creator BuildpackCreator) *cobra.Comman } else { nonDistro = strings.Split(i, "/") } + if i, e := getSliceAt[string](target, 1); e != nil { + logger.Errorf("invalid target %s, atleast one of [name][version] must be specified", t) + } else { + distros = strings.Split(i, ";") + } for _, d := range distros { distro := strings.Split(d, "@") if l := len(distro); l <= 0 { @@ -123,7 +128,7 @@ func BuildpackNew(logger logging.Logger, creator BuildpackCreator) *cobra.Comman cmd.Flags().StringVarP(&flags.API, "api", "a", "0.8", "Buildpack API compatibility of the generated buildpack") cmd.Flags().StringVarP(&flags.Path, "path", "p", "", "Path to generate the buildpack") cmd.Flags().StringVarP(&flags.Version, "version", "V", "1.0.0", "Version of the generated buildpack") - cmd.Flags().StringSliceVarP(&flags.Stacks, "stacks", "s", []string{"io.buildpacks.stacks.jammy"}, "Stack(s) this buildpack will be compatible with"+stringSliceHelp("stack")) + cmd.Flags().StringSliceVarP(&flags.Stacks, "stacks", "s", []string{}, "Stack(s) this buildpack will be compatible with"+stringSliceHelp("stack")) cmd.Flags().MarkDeprecated("stacks", "") cmd.Flags().StringSliceVarP(&flags.Targets, "targets", "t", []string{"/"}, `Targets are the list platforms that one targeting, these are generated as part of scaffolding inside buildpack.toml file. one can provide target platforms in format [os][/arch][/variant]:[distroname@osversion@anotherversion];[distroname@osversion] diff --git a/internal/commands/buildpack_new_test.go b/internal/commands/buildpack_new_test.go index 178061c0ec..e804abd528 100644 --- a/internal/commands/buildpack_new_test.go +++ b/internal/commands/buildpack_new_test.go @@ -64,6 +64,15 @@ func testBuildpackNewCommand(t *testing.T, when spec.G, it spec.S) { ID: "io.buildpacks.stacks.jammy", Mixins: []string{}, }}, + Targets: []dist.Target{{ + OS: "linux", + Arch: "arm", + ArchVariant: "v6", + Distributions: []dist.Distribution{{ + Name: "ubuntu", + Versions: []string{"14.04", "16.04"}, + }}, + }}, }).Return(nil).MaxTimes(1) path := filepath.Join(tmpDir, "some-cnb") diff --git a/pkg/dist/buildpack_descriptor.go b/pkg/dist/buildpack_descriptor.go index 7ac46b97ba..cf93daa2a2 100644 --- a/pkg/dist/buildpack_descriptor.go +++ b/pkg/dist/buildpack_descriptor.go @@ -15,7 +15,7 @@ import ( type BuildpackDescriptor struct { WithAPI *api.Version `toml:"api"` WithInfo ModuleInfo `toml:"buildpack"` - WithStacks []Stack `toml:"stacks"` + WithStacks []Stack `toml:"stacks,omitempty"` WithTargets []Target `toml:"targets,omitempty"` WithOrder Order `toml:"order"` WithWindowsBuild bool From e79226e5a0fc4b91c990cd20f2fbc405e067878f Mon Sep 17 00:00:00 2001 From: WYGIN Date: Thu, 28 Sep 2023 14:47:16 +0530 Subject: [PATCH 05/13] added tests & show error msg when unknow target is passed Signed-off-by: WYGIN --- internal/commands/buildpack_new.go | 112 +++++++++++++--- internal/commands/buildpack_new_test.go | 168 ++++++++++++++++++++++-- 2 files changed, 254 insertions(+), 26 deletions(-) diff --git a/internal/commands/buildpack_new.go b/internal/commands/buildpack_new.go index bab84673fa..c98ffa9462 100644 --- a/internal/commands/buildpack_new.go +++ b/internal/commands/buildpack_new.go @@ -5,6 +5,7 @@ import ( "fmt" "os" "path/filepath" + "runtime" "strings" "github.com/spf13/cobra" @@ -73,23 +74,17 @@ func BuildpackNew(logger logging.Logger, creator BuildpackCreator) *cobra.Comman var targets []dist.Target for _, t := range flags.Targets { var distroMap []dist.Distribution - var nonDistro []string - var distros []string - target := strings.Split(t, ":") - if i, e := getSliceAt[string](target, 0); e != nil { - logger.Errorf("invalid target %s, atleast one of [os][/arch][/archVariant] must be specified", t) - } else { - nonDistro = strings.Split(i, "/") + target, nonDistro, distros, err := getTarget(t) + if err != nil { + logger.Error(err.Error()) } - if i, e := getSliceAt[string](target, 1); e != nil { - logger.Errorf("invalid target %s, atleast one of [name][version] must be specified", t) - } else { + if i, e := getSliceAt[string](target, 1); e == nil { distros = strings.Split(i, ";") } for _, d := range distros { distro := strings.Split(d, "@") if l := len(distro); l <= 0 { - logger.Error("distro is nil!") + return errors.Errorf("distro is nil!") } else if l == 1 { logger.Warnf("forgot to specify version for distro %s ?", distro[0]) } @@ -98,9 +93,10 @@ func BuildpackNew(logger logging.Logger, creator BuildpackCreator) *cobra.Comman Versions: distro[1:], }) } - os, _ := getSliceAt[string](nonDistro, 0) - arch, _ := getSliceAt[string](nonDistro, 1) - variant, _ := getSliceAt[string](nonDistro, 2) + os, arch, variant, err := getPlatform(nonDistro) + if err != nil { + logger.Error(err.Error()) + } targets = append(targets, dist.Target{ OS: os, Arch: arch, @@ -129,8 +125,8 @@ func BuildpackNew(logger logging.Logger, creator BuildpackCreator) *cobra.Comman cmd.Flags().StringVarP(&flags.Path, "path", "p", "", "Path to generate the buildpack") cmd.Flags().StringVarP(&flags.Version, "version", "V", "1.0.0", "Version of the generated buildpack") cmd.Flags().StringSliceVarP(&flags.Stacks, "stacks", "s", []string{}, "Stack(s) this buildpack will be compatible with"+stringSliceHelp("stack")) - cmd.Flags().MarkDeprecated("stacks", "") - cmd.Flags().StringSliceVarP(&flags.Targets, "targets", "t", []string{"/"}, + cmd.Flags().MarkDeprecated("stacks", "prefer `--targets` instead: https://github.com/buildpacks/rfcs/blob/main/text/0096-remove-stacks-mixins.md") + cmd.PersistentFlags().StringSliceVarP(&flags.Targets, "targets", "t", []string{runtime.GOOS + "/" + runtime.GOARCH}, `Targets are the list platforms that one targeting, these are generated as part of scaffolding inside buildpack.toml file. one can provide target platforms in format [os][/arch][/variant]:[distroname@osversion@anotherversion];[distroname@osversion] - Base case for two different architectures : '--targets "linux/amd64" --targets "linux/arm64"' - case for distribution version: '--targets "windows/amd64:windows-nano@10.0.19041.1415"' @@ -149,3 +145,87 @@ func getSliceAt[T interface{}](slice []T, index int) (T, error) { return slice[index], nil } + +var GOOSArch = map[string][]string{ + "aix": {"ppc64"}, + "android": {"386", "amd64", "arm", "arm64"}, + "darwin": {"amd64", "arm64"}, + "dragonfly": {"amd64"}, + "freebsd": {"386", "amd64", "arm"}, + "illumos": {"amd64"}, + "ios": {"arm64"}, + "js": {"wasm"}, + "linux": {"386", "amd64", "arm", "arm64", "loong64", "mips", "mipsle", "mips64", "mips64le", "ppc64", "ppc64le", "riscv64", "s390x"}, + "netbsd": {"386", "amd64", "arm"}, + "openbsd": {"386", "amd64", "arm", "arm64"}, + "plan9": {"386", "amd64", "arm"}, + "solaris": {"amd64"}, + "wasip1": {"wasm"}, + "windows": {"386", "amd64", "arm", "arm64"}, +} + +var GOArchVariant = map[string][]string{ + "386": {"softfloat", "sse2"}, + "arm": {"v5", "v6", "v7"}, + "amd64": {"v1", "v2", "v3", "v4"}, + "mips": {"hardfloat", "softfloat"}, + "mipsle": {"hardfloat", "softfloat"}, + "mips64": {"hardfloat", "softfloat"}, + "mips64le": {"hardfloat", "softfloat"}, + "ppc64": {"power8", "power9"}, + "ppc64le": {"power8", "power9"}, + "wasm": {"satconv", "signext"}, +} + +func isOS(os string) bool { + return GOOSArch[os] != nil +} + +func supportsArch(os string, arch string) bool { + if isOS(os) { + var supported bool + for _, s := range GOOSArch[os] { + if s == arch { + supported = true + break + } + } + return supported + } + return false +} + +func supportsVariant(arch string, variant string) bool { + if variant == "" || len(variant) == 0 { + return true + } + var supported bool + for _, s := range GOArchVariant[arch] { + if s == variant { + supported = true + break + } + } + return supported +} + +func getTarget(t string) ([]string, []string, []string, error) { + var nonDistro, distro []string + target := strings.Split(t, ":") + if i, e := getSliceAt[string](target, 0); e != nil { + return target, nonDistro, distro, errors.Errorf("invalid target %s, atleast one of [os][/arch][/archVariant] must be specified", t) + } else { + nonDistro = strings.Split(i, "/") + } + return target, nonDistro, distro, nil +} + +func getPlatform(t []string) (string, string, string, error) { + os, _ := getSliceAt[string](t, 0) + arch, _ := getSliceAt[string](t, 1) + variant, _ := getSliceAt[string](t, 2) + if !isOS(os) || !supportsArch(os, arch) || !supportsVariant(arch, variant) { + return os, arch, variant, errors.Errorf("unknown target: %s", style.Symbol(strings.Join(t, "/"))) + } + return os, arch, variant, nil +} diff --git a/internal/commands/buildpack_new_test.go b/internal/commands/buildpack_new_test.go index e804abd528..31f278cca6 100644 --- a/internal/commands/buildpack_new_test.go +++ b/internal/commands/buildpack_new_test.go @@ -64,19 +64,10 @@ func testBuildpackNewCommand(t *testing.T, when spec.G, it spec.S) { ID: "io.buildpacks.stacks.jammy", Mixins: []string{}, }}, - Targets: []dist.Target{{ - OS: "linux", - Arch: "arm", - ArchVariant: "v6", - Distributions: []dist.Distribution{{ - Name: "ubuntu", - Versions: []string{"14.04", "16.04"}, - }}, - }}, }).Return(nil).MaxTimes(1) path := filepath.Join(tmpDir, "some-cnb") - command.SetArgs([]string{"--path", path, "example/some-cnb"}) + command.SetArgs([]string{"--path", path, "example/some-cnb", "--stacks", "io.buildpacks.stacks.jammy"}) err := command.Execute() h.AssertNil(t, err) @@ -91,5 +82,162 @@ func testBuildpackNewCommand(t *testing.T, when spec.G, it spec.S) { h.AssertNotNil(t, err) h.AssertContains(t, outBuf.String(), "ERROR: directory") }) + + when("target flag is specified, ", func() { + it("it uses target to generate artifacts", func() { + mockClient.EXPECT().NewBuildpack(gomock.Any(), client.NewBuildpackOptions{ + API: "0.8", + ID: "example/targets", + Path: filepath.Join(tmpDir, "targets"), + Version: "1.0.0", + Targets: []dist.Target{{ + OS: "linux", + Arch: "arm", + ArchVariant: "v6", + Distributions: []dist.Distribution{{ + Name: "ubuntu", + Versions: []string{"14.04", "16.04"}, + }}, + }}, + }).Return(nil).MaxTimes(1) + + path := filepath.Join(tmpDir, "targets") + command.SetArgs([]string{"--path", path, "example/targets", "--targets", "linux/arm/v6:ubuntu@14.04@16.04"}) + + err := command.Execute() + h.AssertNil(t, err) + }) + it("it should show error when invalid [os]/[arch] passed", func() { + mockClient.EXPECT().NewBuildpack(gomock.Any(), client.NewBuildpackOptions{ + API: "0.8", + ID: "example/targets", + Path: filepath.Join(tmpDir, "targets"), + Version: "1.0.0", + Targets: []dist.Target{{ + OS: "os", + Arch: "arm", + ArchVariant: "v6", + Distributions: []dist.Distribution{{ + Name: "ubuntu", + Versions: []string{"14.04", "16.04"}, + }}, + }}, + }).Return(nil).MaxTimes(1) + + path := filepath.Join(tmpDir, "targets") + command.SetArgs([]string{"--path", path, "example/targets", "--targets", "os/arm/v6:ubuntu@14.04@16.04"}) + + err := command.Execute() + h.AssertNotNil(t, err) + }) + when("it should", func() { + it("support format [os][/arch][/variant]:[name@version@version2];[some-name@version@version2]", func() { + mockClient.EXPECT().NewBuildpack(gomock.Any(), client.NewBuildpackOptions{ + API: "0.8", + ID: "example/targets", + Path: filepath.Join(tmpDir, "targets"), + Version: "1.0.0", + Targets: []dist.Target{ + { + OS: "linux", + Arch: "arm", + ArchVariant: "v6", + Distributions: []dist.Distribution{ + { + Name: "ubuntu", + Versions: []string{"14.04", "16.04"}, + }, + { + Name: "debian", + Versions: []string{"8.10", "10.9"}, + }, + }, + }, + { + OS: "windows", + Arch: "amd64", + Distributions: []dist.Distribution{ + { + Name: "windows-nano", + Versions: []string{"10.0.19041.1415"}, + }, + }, + }, + }, + }).Return(nil).MaxTimes(1) + + path := filepath.Join(tmpDir, "targets") + command.SetArgs([]string{"--path", path, "example/targets", "--targets", "linux/arm/v6:ubuntu@14.04@16.04;debian@8.10@10.9", "-t", "windows/amd64:windows-nano@10.0.19041.1415"}) + + err := command.Execute() + h.AssertNil(t, err) + }) + + it("generate a buildpack.toml file with os and arch as empty strings when flag is not specified", func() { + mockClient.EXPECT().NewBuildpack(gomock.Any(), client.NewBuildpackOptions{ + API: "0.8", + ID: "example/targets", + Path: filepath.Join(tmpDir, "targets"), + Version: "1.0.0", + Targets: []dist.Target{{ + OS: "", + Arch: "", + }}, + }).Return(nil).MaxTimes(1) + + path := filepath.Join(tmpDir, "targets") + command.SetArgs([]string{"--path", path, "example/targets"}) + + err := command.Execute() + h.AssertNil(t, err) + }) + }) + + when("stacks ", func() { + it("flag should show deprecated message when used", func() { + mockClient.EXPECT().NewBuildpack(gomock.Any(), client.NewBuildpackOptions{ + API: "0.8", + ID: "example/stacks", + Path: filepath.Join(tmpDir, "stacks"), + Version: "1.0.0", + Stacks: []dist.Stack{{ + ID: "io.buildpacks.stacks.jammy", + Mixins: []string{}, + }}, + }).Return(nil).MaxTimes(1) + + path := filepath.Join(tmpDir, "stacks") + output := new(bytes.Buffer) + command.SetOut(output) + command.SetErr(output) + command.SetArgs([]string{"--path", path, "example/stacks", "--stacks", "io.buildpacks.stacks.jammy"}) + + err := command.Execute() + h.AssertNil(t, err) + h.AssertContains(t, output.String(), "Flag --stacks has been deprecated,") + }) + + it("should be omitted from buildpack.toml file when flag is not specified", func() { + options := client.NewBuildpackOptions{ + API: "0.8", + ID: "example/stacks", + Path: filepath.Join(tmpDir, "stacks"), + Version: "1.0.0", + } + + mockClient.EXPECT().NewBuildpack(gomock.Any(), options).Return(nil).MaxTimes(1) + + path := filepath.Join(tmpDir, "stacks") + tomlFile := filepath.Join(path, "buildpack.toml") + command.SetArgs([]string{"--path", path, "example/stacks"}) + + err := command.Execute() + h.AssertNil(t, err) + output, err := os.ReadFile(tomlFile) + h.AssertNil(t, err) + h.AssertNotContains(t, string(output), "[[stacks]]") + }) + }) + }) }) } From ca5d09f3a4c7dc71e1aff9f2867a34ba764cf138 Mon Sep 17 00:00:00 2001 From: WYGIN Date: Thu, 28 Sep 2023 14:52:16 +0000 Subject: [PATCH 06/13] fixed tests Signed-off-by: WYGIN --- internal/commands/buildpack_new.go | 2 +- internal/commands/buildpack_new_test.go | 54 +++++-------------------- 2 files changed, 10 insertions(+), 46 deletions(-) diff --git a/internal/commands/buildpack_new.go b/internal/commands/buildpack_new.go index c98ffa9462..13bb4bf49f 100644 --- a/internal/commands/buildpack_new.go +++ b/internal/commands/buildpack_new.go @@ -95,7 +95,7 @@ func BuildpackNew(logger logging.Logger, creator BuildpackCreator) *cobra.Comman } os, arch, variant, err := getPlatform(nonDistro) if err != nil { - logger.Error(err.Error()) + return err } targets = append(targets, dist.Target{ OS: os, diff --git a/internal/commands/buildpack_new_test.go b/internal/commands/buildpack_new_test.go index 31f278cca6..467b704ee8 100644 --- a/internal/commands/buildpack_new_test.go +++ b/internal/commands/buildpack_new_test.go @@ -4,6 +4,7 @@ import ( "bytes" "os" "path/filepath" + "runtime" "testing" "github.com/buildpacks/pack/pkg/client" @@ -60,14 +61,14 @@ func testBuildpackNewCommand(t *testing.T, when spec.G, it spec.S) { ID: "example/some-cnb", Path: filepath.Join(tmpDir, "some-cnb"), Version: "1.0.0", - Stacks: []dist.Stack{{ - ID: "io.buildpacks.stacks.jammy", - Mixins: []string{}, + Targets: []dist.Target{{ + OS: runtime.GOOS, + Arch: runtime.GOARCH, }}, }).Return(nil).MaxTimes(1) path := filepath.Join(tmpDir, "some-cnb") - command.SetArgs([]string{"--path", path, "example/some-cnb", "--stacks", "io.buildpacks.stacks.jammy"}) + command.SetArgs([]string{"--path", path, "example/some-cnb"}) err := command.Execute() h.AssertNil(t, err) @@ -172,27 +173,7 @@ func testBuildpackNewCommand(t *testing.T, when spec.G, it spec.S) { err := command.Execute() h.AssertNil(t, err) }) - - it("generate a buildpack.toml file with os and arch as empty strings when flag is not specified", func() { - mockClient.EXPECT().NewBuildpack(gomock.Any(), client.NewBuildpackOptions{ - API: "0.8", - ID: "example/targets", - Path: filepath.Join(tmpDir, "targets"), - Version: "1.0.0", - Targets: []dist.Target{{ - OS: "", - Arch: "", - }}, - }).Return(nil).MaxTimes(1) - - path := filepath.Join(tmpDir, "targets") - command.SetArgs([]string{"--path", path, "example/targets"}) - - err := command.Execute() - h.AssertNil(t, err) - }) }) - when("stacks ", func() { it("flag should show deprecated message when used", func() { mockClient.EXPECT().NewBuildpack(gomock.Any(), client.NewBuildpackOptions{ @@ -204,6 +185,10 @@ func testBuildpackNewCommand(t *testing.T, when spec.G, it spec.S) { ID: "io.buildpacks.stacks.jammy", Mixins: []string{}, }}, + Targets: []dist.Target{{ + OS: runtime.GOOS, + Arch: runtime.GOARCH, + }}, }).Return(nil).MaxTimes(1) path := filepath.Join(tmpDir, "stacks") @@ -216,27 +201,6 @@ func testBuildpackNewCommand(t *testing.T, when spec.G, it spec.S) { h.AssertNil(t, err) h.AssertContains(t, output.String(), "Flag --stacks has been deprecated,") }) - - it("should be omitted from buildpack.toml file when flag is not specified", func() { - options := client.NewBuildpackOptions{ - API: "0.8", - ID: "example/stacks", - Path: filepath.Join(tmpDir, "stacks"), - Version: "1.0.0", - } - - mockClient.EXPECT().NewBuildpack(gomock.Any(), options).Return(nil).MaxTimes(1) - - path := filepath.Join(tmpDir, "stacks") - tomlFile := filepath.Join(path, "buildpack.toml") - command.SetArgs([]string{"--path", path, "example/stacks"}) - - err := command.Execute() - h.AssertNil(t, err) - output, err := os.ReadFile(tomlFile) - h.AssertNil(t, err) - h.AssertNotContains(t, string(output), "[[stacks]]") - }) }) }) }) From 90c2195d1493590e106a4403b9d2b15f7b73bd26 Mon Sep 17 00:00:00 2001 From: WYGIN Date: Thu, 28 Sep 2023 20:58:04 +0530 Subject: [PATCH 07/13] removed persistent flag Signed-off-by: WYGIN --- internal/commands/buildpack_new.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/commands/buildpack_new.go b/internal/commands/buildpack_new.go index 13bb4bf49f..8ced00b9f0 100644 --- a/internal/commands/buildpack_new.go +++ b/internal/commands/buildpack_new.go @@ -126,7 +126,7 @@ func BuildpackNew(logger logging.Logger, creator BuildpackCreator) *cobra.Comman cmd.Flags().StringVarP(&flags.Version, "version", "V", "1.0.0", "Version of the generated buildpack") cmd.Flags().StringSliceVarP(&flags.Stacks, "stacks", "s", []string{}, "Stack(s) this buildpack will be compatible with"+stringSliceHelp("stack")) cmd.Flags().MarkDeprecated("stacks", "prefer `--targets` instead: https://github.com/buildpacks/rfcs/blob/main/text/0096-remove-stacks-mixins.md") - cmd.PersistentFlags().StringSliceVarP(&flags.Targets, "targets", "t", []string{runtime.GOOS + "/" + runtime.GOARCH}, + cmd.Flags().StringSliceVarP(&flags.Targets, "targets", "t", []string{runtime.GOOS + "/" + runtime.GOARCH}, `Targets are the list platforms that one targeting, these are generated as part of scaffolding inside buildpack.toml file. one can provide target platforms in format [os][/arch][/variant]:[distroname@osversion@anotherversion];[distroname@osversion] - Base case for two different architectures : '--targets "linux/amd64" --targets "linux/arm64"' - case for distribution version: '--targets "windows/amd64:windows-nano@10.0.19041.1415"' From 210757cb79996c2f2b704e25a8ef63f560cbfe81 Mon Sep 17 00:00:00 2001 From: WYGIN Date: Fri, 29 Sep 2023 17:48:50 +0530 Subject: [PATCH 08/13] refactored code & still need to fix bugs Signed-off-by: WYGIN --- internal/commands/buildpack_new.go | 131 +----------------------- internal/commands/buildpack_new_test.go | 14 ++- internal/target/parse.go | 74 +++++++++++++ internal/target/parse_test.go | 97 ++++++++++++++++++ internal/target/platform.go | 80 +++++++++++++++ internal/target/platform_test.go | 35 +++++++ 6 files changed, 296 insertions(+), 135 deletions(-) create mode 100644 internal/target/parse.go create mode 100644 internal/target/parse_test.go create mode 100644 internal/target/platform.go create mode 100644 internal/target/platform_test.go diff --git a/internal/commands/buildpack_new.go b/internal/commands/buildpack_new.go index 8ced00b9f0..8b3c118496 100644 --- a/internal/commands/buildpack_new.go +++ b/internal/commands/buildpack_new.go @@ -10,9 +10,8 @@ import ( "github.com/spf13/cobra" - "github.com/pkg/errors" - "github.com/buildpacks/pack/internal/style" + "github.com/buildpacks/pack/internal/target" "github.com/buildpacks/pack/pkg/client" "github.com/buildpacks/pack/pkg/dist" "github.com/buildpacks/pack/pkg/logging" @@ -71,38 +70,9 @@ func BuildpackNew(logger logging.Logger, creator BuildpackCreator) *cobra.Comman }) } - var targets []dist.Target - for _, t := range flags.Targets { - var distroMap []dist.Distribution - target, nonDistro, distros, err := getTarget(t) - if err != nil { - logger.Error(err.Error()) - } - if i, e := getSliceAt[string](target, 1); e == nil { - distros = strings.Split(i, ";") - } - for _, d := range distros { - distro := strings.Split(d, "@") - if l := len(distro); l <= 0 { - return errors.Errorf("distro is nil!") - } else if l == 1 { - logger.Warnf("forgot to specify version for distro %s ?", distro[0]) - } - distroMap = append(distroMap, dist.Distribution{ - Name: distro[0], - Versions: distro[1:], - }) - } - os, arch, variant, err := getPlatform(nonDistro) - if err != nil { - return err - } - targets = append(targets, dist.Target{ - OS: os, - Arch: arch, - ArchVariant: variant, - Distributions: distroMap, - }) + targets, err := target.ParseTargets(flags.Targets) + if err != nil { + return err } if err := creator.NewBuildpack(cmd.Context(), client.NewBuildpackOptions{ @@ -136,96 +106,3 @@ func BuildpackNew(logger logging.Logger, creator BuildpackCreator) *cobra.Comman AddHelpFlag(cmd, "new") return cmd } - -func getSliceAt[T interface{}](slice []T, index int) (T, error) { - if index < 0 || index >= len(slice) { - var r T - return r, errors.Errorf("index out of bound, cannot access item at index %d of slice with length %d", index, len(slice)) - } - - return slice[index], nil -} - -var GOOSArch = map[string][]string{ - "aix": {"ppc64"}, - "android": {"386", "amd64", "arm", "arm64"}, - "darwin": {"amd64", "arm64"}, - "dragonfly": {"amd64"}, - "freebsd": {"386", "amd64", "arm"}, - "illumos": {"amd64"}, - "ios": {"arm64"}, - "js": {"wasm"}, - "linux": {"386", "amd64", "arm", "arm64", "loong64", "mips", "mipsle", "mips64", "mips64le", "ppc64", "ppc64le", "riscv64", "s390x"}, - "netbsd": {"386", "amd64", "arm"}, - "openbsd": {"386", "amd64", "arm", "arm64"}, - "plan9": {"386", "amd64", "arm"}, - "solaris": {"amd64"}, - "wasip1": {"wasm"}, - "windows": {"386", "amd64", "arm", "arm64"}, -} - -var GOArchVariant = map[string][]string{ - "386": {"softfloat", "sse2"}, - "arm": {"v5", "v6", "v7"}, - "amd64": {"v1", "v2", "v3", "v4"}, - "mips": {"hardfloat", "softfloat"}, - "mipsle": {"hardfloat", "softfloat"}, - "mips64": {"hardfloat", "softfloat"}, - "mips64le": {"hardfloat", "softfloat"}, - "ppc64": {"power8", "power9"}, - "ppc64le": {"power8", "power9"}, - "wasm": {"satconv", "signext"}, -} - -func isOS(os string) bool { - return GOOSArch[os] != nil -} - -func supportsArch(os string, arch string) bool { - if isOS(os) { - var supported bool - for _, s := range GOOSArch[os] { - if s == arch { - supported = true - break - } - } - return supported - } - return false -} - -func supportsVariant(arch string, variant string) bool { - if variant == "" || len(variant) == 0 { - return true - } - var supported bool - for _, s := range GOArchVariant[arch] { - if s == variant { - supported = true - break - } - } - return supported -} - -func getTarget(t string) ([]string, []string, []string, error) { - var nonDistro, distro []string - target := strings.Split(t, ":") - if i, e := getSliceAt[string](target, 0); e != nil { - return target, nonDistro, distro, errors.Errorf("invalid target %s, atleast one of [os][/arch][/archVariant] must be specified", t) - } else { - nonDistro = strings.Split(i, "/") - } - return target, nonDistro, distro, nil -} - -func getPlatform(t []string) (string, string, string, error) { - os, _ := getSliceAt[string](t, 0) - arch, _ := getSliceAt[string](t, 1) - variant, _ := getSliceAt[string](t, 2) - if !isOS(os) || !supportsArch(os, arch) || !supportsVariant(arch, variant) { - return os, arch, variant, errors.Errorf("unknown target: %s", style.Symbol(strings.Join(t, "/"))) - } - return os, arch, variant, nil -} diff --git a/internal/commands/buildpack_new_test.go b/internal/commands/buildpack_new_test.go index 467b704ee8..b23be1f24d 100644 --- a/internal/commands/buildpack_new_test.go +++ b/internal/commands/buildpack_new_test.go @@ -37,6 +37,10 @@ func testBuildpackNewCommand(t *testing.T, when spec.G, it spec.S) { mockClient *testmocks.MockPackClient tmpDir string ) + targets := []dist.Target{{ + OS: runtime.GOOS, + Arch: runtime.GOARCH, + }} it.Before(func() { var err error @@ -61,10 +65,7 @@ func testBuildpackNewCommand(t *testing.T, when spec.G, it spec.S) { ID: "example/some-cnb", Path: filepath.Join(tmpDir, "some-cnb"), Version: "1.0.0", - Targets: []dist.Target{{ - OS: runtime.GOOS, - Arch: runtime.GOARCH, - }}, + Targets: targets, }).Return(nil).MaxTimes(1) path := filepath.Join(tmpDir, "some-cnb") @@ -185,10 +186,7 @@ func testBuildpackNewCommand(t *testing.T, when spec.G, it spec.S) { ID: "io.buildpacks.stacks.jammy", Mixins: []string{}, }}, - Targets: []dist.Target{{ - OS: runtime.GOOS, - Arch: runtime.GOARCH, - }}, + Targets: targets, }).Return(nil).MaxTimes(1) path := filepath.Join(tmpDir, "stacks") diff --git a/internal/target/parse.go b/internal/target/parse.go new file mode 100644 index 0000000000..73b56f60d9 --- /dev/null +++ b/internal/target/parse.go @@ -0,0 +1,74 @@ +package target + +import ( + "fmt" + "strings" + + "github.com/buildpacks/pack/pkg/dist" +) + +func ParseTargets(t []string) (targets []dist.Target, err error) { + for _, v := range t { + target, err := ParseTarget(v) + if err != nil { + return targets, err + } + targets = append(targets, target) + } + return targets, err +} + +func ParseTarget(t string) (output dist.Target, err error) { + nonDistro, distros, err := getTarget(t) + if err != nil { + return output, err + } + os, arch, variant, err := getPlatform(nonDistro) + if err != nil { + return output, err + } + output = dist.Target{ + OS: os, + Arch: arch, + ArchVariant: variant, + Distributions: ParseDistros(distros), + } + return output, err +} + +func ParseDistros(distroSlice []string) (distros []dist.Distribution) { + for _, d := range distroSlice { + distros = append(distros, ParseDistro(d)) + } + return distros +} + +func ParseDistro(distroString string) (distro dist.Distribution) { + d := strings.Split(distroString, "@") + distro = dist.Distribution{ + Name: d[0], + Versions: d[1:], + } + return distro +} + +func getTarget(t string) (nonDistro, distros []string, err error) { + target := strings.Split(t, ":") + if i, err := getSliceAt[string](target, 0); err != nil { + return nonDistro, distros, fmt.Errorf("invalid target %s, atleast one of [os][/arch][/archVariant] must be specified", t) + } else { + nonDistro = strings.Split(i, "/") + } + if i, err := getSliceAt[string](target, 1); err != nil { + distros = strings.Split(i, ";") + } + return nonDistro, distros, err +} + +func getSliceAt[T interface{}](slice []T, index int) (value T, err error) { + if index < 0 || index >= len(slice) { + return value, fmt.Errorf("index out of bound, cannot access item at index %d of slice with length %d", index, len(slice)) + } + + return slice[index], err +} diff --git a/internal/target/parse_test.go b/internal/target/parse_test.go new file mode 100644 index 0000000000..87fc1ad290 --- /dev/null +++ b/internal/target/parse_test.go @@ -0,0 +1,97 @@ +package target_test + +import ( + "testing" + + "github.com/heroku/color" + "github.com/sclevine/spec" + "github.com/sclevine/spec/report" + + "github.com/buildpacks/pack/internal/target" + "github.com/buildpacks/pack/pkg/dist" + h "github.com/buildpacks/pack/testhelpers" +) + +func TestParseTargets(t *testing.T) { + color.Disable(true) + defer color.Disable(false) + spec.Run(t, "ParseTargets", testParseTargets, spec.Parallel(), spec.Report(report.Terminal{})) +} + +func testParseTargets(t *testing.T, when spec.G, it spec.S) { + it.Before(func() { + var err error + h.AssertNil(t, err) + }) + + when("target#ParseTarget", func() { + it("should throw an error when [os][/arch][/variant] is nil", func() { + _, err := target.ParseTarget(":distro@version") + h.AssertNotNil(t, err) + }) + it("should parse target as expected", func() { + output, err := target.ParseTarget("linux/arm/v6") + h.AssertNil(t, err) + h.AssertEq(t, output, &dist.Target{ + OS: "linux", + Arch: "arm", + ArchVariant: "v6", + }) + }) + }) + when("target#ParseTargets", func() { + it("should throw an error when atleast one target throws error", func() { + _, err := target.ParseTargets([]string{"linux/arm/v6", ":distro@version"}) + h.AssertNotNil(t, err) + }) + it("should parse targets as expected", func() { + output, err := target.ParseTargets([]string{"linux/arm/v6", "linux/amd:ubuntu@22.04;debian@8.10@10.06"}) + h.AssertNil(t, err) + h.AssertEq(t, output, &[]dist.Target{ + { + OS: "linux", + Arch: "arm", + ArchVariant: "v6", + }, + { + OS: "linux", + Arch: "amd", + Distributions: []dist.Distribution{ + { + Name: "ubuntu", + Versions: []string{"22.04"}, + }, + { + Name: "debian", + Versions: []string{"8.10", "10.06"}, + }, + }, + }, + }) + }) + }) + when("target#ParseDistro", func() { + it("should parse distro as expected", func() { + output := target.ParseDistro("ubuntu@22.04@20.08") + h.AssertEq(t, output, &dist.Distribution{ + Name: "ubuntu", + Versions: []string{"22.04", "20.08"}, + }) + }) + }) + when("target#ParseDistros", func() { + it("should parse distros as expected", func() { + output := target.ParseDistros([]string{"ubuntu@22.04@20.08;debian@8.10@10.06"}) + h.AssertEq(t, output, &[]dist.Distribution{ + { + Name: "ubuntu", + Versions: []string{"22.04", "20.08"}, + }, + { + Name: "debian", + Versions: []string{"8.10", "10.06"}, + }, + }) + }) + }) +} diff --git a/internal/target/platform.go b/internal/target/platform.go new file mode 100644 index 0000000000..6a7386accf --- /dev/null +++ b/internal/target/platform.go @@ -0,0 +1,80 @@ +package target + +import ( + "fmt" + "strings" + + "github.com/buildpacks/pack/internal/style" +) + +func getPlatform(t []string) (os, arch, variant string, err error) { + os, _ = getSliceAt[string](t, 0) + arch, _ = getSliceAt[string](t, 1) + variant, _ = getSliceAt[string](t, 2) + if !SupportsPlatform(os, arch, variant) { + return os, arch, variant, fmt.Errorf("unknown target: %s", style.Symbol(strings.Join(t, "/"))) + } + return os, arch, variant, err +} + +var supportedOSArchs = map[string][]string{ + "aix": {"ppc64"}, + "android": {"386", "amd64", "arm", "arm64"}, + "darwin": {"amd64", "arm64"}, + "dragonfly": {"amd64"}, + "freebsd": {"386", "amd64", "arm"}, + "illumos": {"amd64"}, + "ios": {"arm64"}, + "js": {"wasm"}, + "linux": {"386", "amd64", "arm", "arm64", "loong64", "mips", "mipsle", "mips64", "mips64le", "ppc64", "ppc64le", "riscv64", "s390x"}, + "netbsd": {"386", "amd64", "arm"}, + "openbsd": {"386", "amd64", "arm", "arm64"}, + "plan9": {"386", "amd64", "arm"}, + "solaris": {"amd64"}, + "wasip1": {"wasm"}, + "windows": {"386", "amd64", "arm", "arm64"}, +} + +var supportedArchVariants = map[string][]string{ + "386": {"softfloat", "sse2"}, + "arm": {"v5", "v6", "v7"}, + "amd64": {"v1", "v2", "v3", "v4"}, + "mips": {"hardfloat", "softfloat"}, + "mipsle": {"hardfloat", "softfloat"}, + "mips64": {"hardfloat", "softfloat"}, + "mips64le": {"hardfloat", "softfloat"}, + "ppc64": {"power8", "power9"}, + "ppc64le": {"power8", "power9"}, + "wasm": {"satconv", "signext"}, +} + +func supportsOS(os string) bool { + return supportedOSArchs[os] != nil +} + +func supportsArch(os, arch string) bool { + if supportsOS(os) { + for _, s := range supportedOSArchs[os] { + if s == arch { + return true + } + } + } + return false +} + +func supportsVariant(arch, variant string) (supported bool) { + if variant == "" || len(variant) == 0 { + return true + } + for _, s := range supportedArchVariants[arch] { + if s == variant { + return true + } + } + return supported +} + +func SupportsPlatform(os, arch, variant string) bool { + return supportsArch(os, arch) || supportsVariant(arch, variant) +} diff --git a/internal/target/platform_test.go b/internal/target/platform_test.go new file mode 100644 index 0000000000..43a73a9b65 --- /dev/null +++ b/internal/target/platform_test.go @@ -0,0 +1,35 @@ +package target_test + +import ( + "testing" + + "github.com/heroku/color" + "github.com/sclevine/spec" + "github.com/sclevine/spec/report" + + "github.com/buildpacks/pack/internal/target" + h "github.com/buildpacks/pack/testhelpers" +) + +func TestPlatforms(t *testing.T) { + color.Disable(true) + defer color.Disable(false) + spec.Run(t, "TestPlatforms", testPlatforms, spec.Parallel(), spec.Report(report.Terminal{})) +} + +func testPlatforms(t *testing.T, when spec.G, it spec.S) { + it.Before(func() { + var err error + h.AssertNil(t, err) + }) + when("target#SupportsPlatform", func() { + it("should return false when target not supported", func() { + b := target.SupportsPlatform("os", "arch", "variant") + h.AssertFalse(t, b) + }) + it("should parse targets as expected", func() { + b := target.SupportsPlatform("linux", "arm", "v6") + h.AssertTrue(t, b) + }) + }) +} From 77dac7f894c298520662209286328e4a0a7d1745 Mon Sep 17 00:00:00 2001 From: WYGIN Date: Fri, 29 Sep 2023 14:57:41 +0000 Subject: [PATCH 09/13] fix bugs Signed-off-by: WYGIN --- internal/target/parse.go | 59 ++++++++++++++++++++++---------- internal/target/parse_test.go | 19 ++++++---- internal/target/platform.go | 9 ++--- internal/target/platform_test.go | 2 +- 4 files changed, 59 insertions(+), 30 deletions(-) diff --git a/internal/target/parse.go b/internal/target/parse.go index 73b56f60d9..edd22ecf50 100644 --- a/internal/target/parse.go +++ b/internal/target/parse.go @@ -1,9 +1,11 @@ package target import ( - "fmt" "strings" + "github.com/pkg/errors" + + "github.com/buildpacks/pack/internal/style" "github.com/buildpacks/pack/pkg/dist" ) @@ -11,15 +13,18 @@ func ParseTargets(t []string) (targets []dist.Target, err error) { for _, v := range t { target, err := ParseTarget(v) if err != nil { - return targets, err + return nil, err } targets = append(targets, target) } - return targets, err + return targets, nil } func ParseTarget(t string) (output dist.Target, err error) { nonDistro, distros, err := getTarget(t) + if len(nonDistro) <= 1 && nonDistro[0] == "" { + return output, errors.Errorf("%s", style.Error("os/arch must be defined")) + } if err != nil { return output, err } @@ -27,47 +32,63 @@ func ParseTarget(t string) (output dist.Target, err error) { if err != nil { return output, err } + v, err := ParseDistros(distros) + if err != nil { + return output, err + } output = dist.Target{ OS: os, Arch: arch, ArchVariant: variant, - Distributions: ParseDistros(distros), + Distributions: v, } return output, err } -func ParseDistros(distroSlice []string) (distros []dist.Distribution) { - for _, d := range distroSlice { - distros = append(distros, ParseDistro(d)) +func ParseDistros(distroSlice string) (distros []dist.Distribution, err error) { + distro := strings.Split(distroSlice, ";") + if l := len(distro); l == 1 && distro[0] == "" { + return nil, nil + } + for _, d := range distro { + v, err, isWarn := ParseDistro(d) + if err != nil && !isWarn { + return nil, err + } + distros = append(distros, v) } - return distros + return distros, nil } -func ParseDistro(distroString string) (distro dist.Distribution) { +func ParseDistro(distroString string) (distro dist.Distribution, err error, isWarn bool) { d := strings.Split(distroString, "@") - distro = dist.Distribution{ - Name: d[0], - Versions: d[1:], + if d[0] == "" || len(d) == 0 { + return distro, errors.Errorf("distro's versions %s cannot be specified without distro's name", style.Symbol("@"+strings.Join(d[1:], "@"))), false + } + if len(d) <= 2 && d[0] == "" { + return distro, errors.Errorf("distro with name %s has no specific version!", style.Symbol(d[0])), true } - return distro + distro.Name = d[0] + distro.Versions = d[1:] + return distro, nil, false } -func getTarget(t string) (nonDistro, distros []string, err error) { +func getTarget(t string) (nonDistro []string, distros string, err error) { target := strings.Split(t, ":") - if i, err := getSliceAt[string](target, 0); err != nil { - return nonDistro, distros, fmt.Errorf("invalid target %s, atleast one of [os][/arch][/archVariant] must be specified", t) + if i, err := getSliceAt[string](target, 0); err != nil || len(target) == 0 { + return nonDistro, distros, errors.Errorf("invalid target %s, atleast one of [os][/arch][/archVariant] must be specified", t) } else { nonDistro = strings.Split(i, "/") } - if i, err := getSliceAt[string](target, 1); err != nil { - distros = strings.Split(i, ";") + if i, err := getSliceAt[string](target, 1); err == nil { + distros = i } return nonDistro, distros, err } func getSliceAt[T interface{}](slice []T, index int) (value T, err error) { if index < 0 || index >= len(slice) { - return value, fmt.Errorf("index out of bound, cannot access item at index %d of slice with length %d", index, len(slice)) + return value, errors.Errorf("index out of bound, cannot access item at index %d of slice with length %d", index, len(slice)) } return slice[index], err diff --git a/internal/target/parse_test.go b/internal/target/parse_test.go index 87fc1ad290..b136f8c318 100644 --- a/internal/target/parse_test.go +++ b/internal/target/parse_test.go @@ -32,7 +32,7 @@ func testParseTargets(t *testing.T, when spec.G, it spec.S) { it("should parse target as expected", func() { output, err := target.ParseTarget("linux/arm/v6") h.AssertNil(t, err) - h.AssertEq(t, output, &dist.Target{ + h.AssertEq(t, output, dist.Target{ OS: "linux", Arch: "arm", ArchVariant: "v6", @@ -47,7 +47,7 @@ func testParseTargets(t *testing.T, when spec.G, it spec.S) { it("should parse targets as expected", func() { output, err := target.ParseTargets([]string{"linux/arm/v6", "linux/amd:ubuntu@22.04;debian@8.10@10.06"}) h.AssertNil(t, err) - h.AssertEq(t, output, &[]dist.Target{ + h.AssertEq(t, output, []dist.Target{ { OS: "linux", Arch: "arm", @@ -72,17 +72,18 @@ func testParseTargets(t *testing.T, when spec.G, it spec.S) { }) when("target#ParseDistro", func() { it("should parse distro as expected", func() { - output := target.ParseDistro("ubuntu@22.04@20.08") - h.AssertEq(t, output, &dist.Distribution{ + output, err, _ := target.ParseDistro("ubuntu@22.04@20.08") + h.AssertEq(t, output, dist.Distribution{ Name: "ubuntu", Versions: []string{"22.04", "20.08"}, }) + h.AssertNil(t, err) }) }) when("target#ParseDistros", func() { it("should parse distros as expected", func() { - output := target.ParseDistros([]string{"ubuntu@22.04@20.08;debian@8.10@10.06"}) - h.AssertEq(t, output, &[]dist.Distribution{ + output, err := target.ParseDistros("ubuntu@22.04@20.08;debian@8.10@10.06") + h.AssertEq(t, output, []dist.Distribution{ { Name: "ubuntu", Versions: []string{"22.04", "20.08"}, @@ -92,6 +93,12 @@ func testParseTargets(t *testing.T, when spec.G, it spec.S) { Versions: []string{"8.10", "10.06"}, }, }) + h.AssertNil(t, err) + }) + it("result should be nil", func() { + output, err := target.ParseDistros("") + h.AssertEq(t, output, []dist.Distribution(nil)) + h.AssertNil(t, err) }) }) } diff --git a/internal/target/platform.go b/internal/target/platform.go index 6a7386accf..9da8c76f86 100644 --- a/internal/target/platform.go +++ b/internal/target/platform.go @@ -1,9 +1,10 @@ package target import ( - "fmt" "strings" + "github.com/pkg/errors" + "github.com/buildpacks/pack/internal/style" ) @@ -12,9 +13,9 @@ func getPlatform(t []string) (os, arch, variant string, err error) { arch, _ = getSliceAt[string](t, 1) variant, _ = getSliceAt[string](t, 2) if !SupportsPlatform(os, arch, variant) { - return os, arch, variant, fmt.Errorf("unknown target: %s", style.Symbol(strings.Join(t, "/"))) + return os, arch, variant, errors.Errorf("unknown target: %s", style.Symbol(strings.Join(t, "/"))) } - return os, arch, variant, err + return os, arch, variant, nil } var supportedOSArchs = map[string][]string{ @@ -76,5 +77,5 @@ func supportsVariant(arch, variant string) (supported bool) { } func SupportsPlatform(os, arch, variant string) bool { - return supportsArch(os, arch) || supportsVariant(arch, variant) + return supportsArch(os, arch) && supportsVariant(arch, variant) } diff --git a/internal/target/platform_test.go b/internal/target/platform_test.go index 43a73a9b65..469bd88962 100644 --- a/internal/target/platform_test.go +++ b/internal/target/platform_test.go @@ -24,7 +24,7 @@ func testPlatforms(t *testing.T, when spec.G, it spec.S) { }) when("target#SupportsPlatform", func() { it("should return false when target not supported", func() { - b := target.SupportsPlatform("os", "arch", "variant") + b := target.SupportsPlatform("os", "arm", "v6") h.AssertFalse(t, b) }) it("should parse targets as expected", func() { From 184f6c3ca9fe65524194a101b2b479fd52ed33f8 Mon Sep 17 00:00:00 2001 From: WYGIN Date: Sun, 1 Oct 2023 20:17:22 +0530 Subject: [PATCH 10/13] added warns to targets flag Signed-off-by: WYGIN --- internal/commands/buildpack_new.go | 5 ++- internal/target/parse.go | 63 +++++++++++++++++------------- internal/target/parse_test.go | 14 +++---- internal/target/platform.go | 16 ++++++-- internal/warn/warn.go | 20 ++++++++++ 5 files changed, 80 insertions(+), 38 deletions(-) create mode 100644 internal/warn/warn.go diff --git a/internal/commands/buildpack_new.go b/internal/commands/buildpack_new.go index 8b3c118496..6904658ce0 100644 --- a/internal/commands/buildpack_new.go +++ b/internal/commands/buildpack_new.go @@ -70,7 +70,10 @@ func BuildpackNew(logger logging.Logger, creator BuildpackCreator) *cobra.Comman }) } - targets, err := target.ParseTargets(flags.Targets) + targets, warn, err := target.ParseTargets(flags.Targets) + for _,w := range warn.Messages { + logger.Warn(w) + } if err != nil { return err } diff --git a/internal/target/parse.go b/internal/target/parse.go index edd22ecf50..bb4c893b83 100644 --- a/internal/target/parse.go +++ b/internal/target/parse.go @@ -1,40 +1,46 @@ package target import ( + "fmt" "strings" "github.com/pkg/errors" "github.com/buildpacks/pack/internal/style" + "github.com/buildpacks/pack/internal/warn" "github.com/buildpacks/pack/pkg/dist" ) -func ParseTargets(t []string) (targets []dist.Target, err error) { +func ParseTargets(t []string) (targets []dist.Target, warn *warn.Warn, err error) { for _, v := range t { - target, err := ParseTarget(v) + target, w, err := ParseTarget(v) + warn.AddWarn(w) if err != nil { - return nil, err + return nil, warn, err } targets = append(targets, target) } - return targets, nil + return targets, warn, nil } -func ParseTarget(t string) (output dist.Target, err error) { - nonDistro, distros, err := getTarget(t) +func ParseTarget(t string) (output dist.Target, warn *warn.Warn, err error) { + nonDistro, distros, w, err := getTarget(t) + warn.AddWarn(w) if len(nonDistro) <= 1 && nonDistro[0] == "" { - return output, errors.Errorf("%s", style.Error("os/arch must be defined")) + warn.Add(style.Error("os/arch must be defined")) } if err != nil { - return output, err + return output, warn, err } - os, arch, variant, err := getPlatform(nonDistro) + os, arch, variant,w, err := getPlatform(nonDistro) + warn.AddWarn(w) if err != nil { - return output, err + return output, warn, err } - v, err := ParseDistros(distros) + v, w, err := ParseDistros(distros) + warn.AddWarn(w) if err != nil { - return output, err + return output, warn, err } output = dist.Target{ OS: os, @@ -42,48 +48,51 @@ func ParseTarget(t string) (output dist.Target, err error) { ArchVariant: variant, Distributions: v, } - return output, err + return output, warn, err } -func ParseDistros(distroSlice string) (distros []dist.Distribution, err error) { +func ParseDistros(distroSlice string) (distros []dist.Distribution, warn *warn.Warn, err error) { distro := strings.Split(distroSlice, ";") if l := len(distro); l == 1 && distro[0] == "" { - return nil, nil + return nil, warn, err } for _, d := range distro { - v, err, isWarn := ParseDistro(d) - if err != nil && !isWarn { - return nil, err + v, w, err := ParseDistro(d) + warn.AddWarn(w) + if err != nil { + return nil, warn, err } distros = append(distros, v) } - return distros, nil + return distros, warn, nil } -func ParseDistro(distroString string) (distro dist.Distribution, err error, isWarn bool) { +func ParseDistro(distroString string) (distro dist.Distribution, warn *warn.Warn, err error) { d := strings.Split(distroString, "@") if d[0] == "" || len(d) == 0 { - return distro, errors.Errorf("distro's versions %s cannot be specified without distro's name", style.Symbol("@"+strings.Join(d[1:], "@"))), false + return distro, warn, errors.Errorf("distro's versions %s cannot be specified without distro's name", style.Symbol("@"+strings.Join(d[1:], "@"))) } if len(d) <= 2 && d[0] == "" { - return distro, errors.Errorf("distro with name %s has no specific version!", style.Symbol(d[0])), true + warn.Add(fmt.Sprintf("distro with name %s has no specific version!", style.Symbol(d[0]))) } distro.Name = d[0] distro.Versions = d[1:] - return distro, nil, false + return distro, warn, err } -func getTarget(t string) (nonDistro []string, distros string, err error) { +func getTarget(t string) (nonDistro []string, distros string, warn *warn.Warn, err error) { target := strings.Split(t, ":") - if i, err := getSliceAt[string](target, 0); err != nil || len(target) == 0 { - return nonDistro, distros, errors.Errorf("invalid target %s, atleast one of [os][/arch][/archVariant] must be specified", t) + if i, err := getSliceAt[string](target, 0); err != nil { + return nonDistro, distros, warn, errors.Errorf("invalid target %s, atleast one of [os][/arch][/archVariant] must be specified", t) + } else if len(target) == 2 && target[0] == "" { + warn.Add(style.Warn("adding distros %s without [os][/arch][/variant]", target[2])) } else { nonDistro = strings.Split(i, "/") } if i, err := getSliceAt[string](target, 1); err == nil { distros = i } - return nonDistro, distros, err + return nonDistro, distros, warn, err } func getSliceAt[T interface{}](slice []T, index int) (value T, err error) { diff --git a/internal/target/parse_test.go b/internal/target/parse_test.go index b136f8c318..b4d51ee37d 100644 --- a/internal/target/parse_test.go +++ b/internal/target/parse_test.go @@ -26,11 +26,11 @@ func testParseTargets(t *testing.T, when spec.G, it spec.S) { when("target#ParseTarget", func() { it("should throw an error when [os][/arch][/variant] is nil", func() { - _, err := target.ParseTarget(":distro@version") + _,_, err := target.ParseTarget(":distro@version") h.AssertNotNil(t, err) }) it("should parse target as expected", func() { - output, err := target.ParseTarget("linux/arm/v6") + output,_, err := target.ParseTarget("linux/arm/v6") h.AssertNil(t, err) h.AssertEq(t, output, dist.Target{ OS: "linux", @@ -41,11 +41,11 @@ func testParseTargets(t *testing.T, when spec.G, it spec.S) { }) when("target#ParseTargets", func() { it("should throw an error when atleast one target throws error", func() { - _, err := target.ParseTargets([]string{"linux/arm/v6", ":distro@version"}) + _,_, err := target.ParseTargets([]string{"linux/arm/v6", ":distro@version"}) h.AssertNotNil(t, err) }) it("should parse targets as expected", func() { - output, err := target.ParseTargets([]string{"linux/arm/v6", "linux/amd:ubuntu@22.04;debian@8.10@10.06"}) + output,_, err := target.ParseTargets([]string{"linux/arm/v6", "linux/amd:ubuntu@22.04;debian@8.10@10.06"}) h.AssertNil(t, err) h.AssertEq(t, output, []dist.Target{ { @@ -72,7 +72,7 @@ func testParseTargets(t *testing.T, when spec.G, it spec.S) { }) when("target#ParseDistro", func() { it("should parse distro as expected", func() { - output, err, _ := target.ParseDistro("ubuntu@22.04@20.08") + output, _, err := target.ParseDistro("ubuntu@22.04@20.08") h.AssertEq(t, output, dist.Distribution{ Name: "ubuntu", Versions: []string{"22.04", "20.08"}, @@ -82,7 +82,7 @@ func testParseTargets(t *testing.T, when spec.G, it spec.S) { }) when("target#ParseDistros", func() { it("should parse distros as expected", func() { - output, err := target.ParseDistros("ubuntu@22.04@20.08;debian@8.10@10.06") + output, _, err := target.ParseDistros("ubuntu@22.04@20.08;debian@8.10@10.06") h.AssertEq(t, output, []dist.Distribution{ { Name: "ubuntu", @@ -96,7 +96,7 @@ func testParseTargets(t *testing.T, when spec.G, it spec.S) { h.AssertNil(t, err) }) it("result should be nil", func() { - output, err := target.ParseDistros("") + output, _, err := target.ParseDistros("") h.AssertEq(t, output, []dist.Distribution(nil)) h.AssertNil(t, err) }) diff --git a/internal/target/platform.go b/internal/target/platform.go index 9da8c76f86..8ca085d2b8 100644 --- a/internal/target/platform.go +++ b/internal/target/platform.go @@ -6,16 +6,26 @@ import ( "github.com/pkg/errors" "github.com/buildpacks/pack/internal/style" + "github.com/buildpacks/pack/internal/warn" ) -func getPlatform(t []string) (os, arch, variant string, err error) { +func getPlatform(t []string) (os, arch, variant string, warn *warn.Warn, err error) { os, _ = getSliceAt[string](t, 0) arch, _ = getSliceAt[string](t, 1) variant, _ = getSliceAt[string](t, 2) + if !supportsOS(os) && supportsVariant(arch, variant) { + warn.Add(style.Warn("unknown os %s, is this a typo", os)) + } + if supportsArch(os, arch) && !supportsVariant(arch, variant) { + warn.Add(style.Warn("unknown variant %s", variant)) + } + if supportsOS(os) && !supportsArch(os, arch) && supportsVariant(arch, variant) { + warn.Add(style.Warn("unknown arch %s", arch)) + } if !SupportsPlatform(os, arch, variant) { - return os, arch, variant, errors.Errorf("unknown target: %s", style.Symbol(strings.Join(t, "/"))) + return os, arch, variant, warn, errors.Errorf("unknown target: %s", style.Symbol(strings.Join(t, "/"))) } - return os, arch, variant, nil + return os, arch, variant, warn, err } var supportedOSArchs = map[string][]string{ diff --git a/internal/warn/warn.go b/internal/warn/warn.go new file mode 100644 index 0000000000..5a6f6b18b0 --- /dev/null +++ b/internal/warn/warn.go @@ -0,0 +1,20 @@ +package warn + +type Warn struct { + Messages []string +} + +func(warn *Warn) Add(mesage string) *Warn { + warn.Messages = append(warn.Messages, mesage) + return warn +} + +func(warn *Warn) AddSlice(messages... string) *Warn { + warn.Messages = append(warn.Messages, messages...) + return warn +} + +func(w *Warn) AddWarn(warn *Warn) *Warn { + w.Messages = append(w.Messages, warn.Messages...) + return w +} \ No newline at end of file From 9a5036c4b47305d5e210740fc1c71943884c1765 Mon Sep 17 00:00:00 2001 From: WYGIN Date: Sun, 1 Oct 2023 15:28:36 +0000 Subject: [PATCH 11/13] fix tests Signed-off-by: WYGIN --- internal/target/parse.go | 15 ++++++++------- internal/target/parse_test.go | 10 +++++----- internal/target/platform.go | 2 +- internal/warn/warn.go | 6 +++--- 4 files changed, 17 insertions(+), 16 deletions(-) diff --git a/internal/target/parse.go b/internal/target/parse.go index bb4c893b83..dda23b84c9 100644 --- a/internal/target/parse.go +++ b/internal/target/parse.go @@ -11,7 +11,7 @@ import ( "github.com/buildpacks/pack/pkg/dist" ) -func ParseTargets(t []string) (targets []dist.Target, warn *warn.Warn, err error) { +func ParseTargets(t []string) (targets []dist.Target, warn warn.Warn, err error) { for _, v := range t { target, w, err := ParseTarget(v) warn.AddWarn(w) @@ -23,10 +23,10 @@ func ParseTargets(t []string) (targets []dist.Target, warn *warn.Warn, err error return targets, warn, nil } -func ParseTarget(t string) (output dist.Target, warn *warn.Warn, err error) { +func ParseTarget(t string) (output dist.Target, warn warn.Warn, err error) { nonDistro, distros, w, err := getTarget(t) warn.AddWarn(w) - if len(nonDistro) <= 1 && nonDistro[0] == "" { + if v, _ := getSliceAt[string](nonDistro, 0); len(nonDistro) <= 1 && v == "" { warn.Add(style.Error("os/arch must be defined")) } if err != nil { @@ -51,7 +51,7 @@ func ParseTarget(t string) (output dist.Target, warn *warn.Warn, err error) { return output, warn, err } -func ParseDistros(distroSlice string) (distros []dist.Distribution, warn *warn.Warn, err error) { +func ParseDistros(distroSlice string) (distros []dist.Distribution, warn warn.Warn, err error) { distro := strings.Split(distroSlice, ";") if l := len(distro); l == 1 && distro[0] == "" { return nil, warn, err @@ -67,7 +67,7 @@ func ParseDistros(distroSlice string) (distros []dist.Distribution, warn *warn.W return distros, warn, nil } -func ParseDistro(distroString string) (distro dist.Distribution, warn *warn.Warn, err error) { +func ParseDistro(distroString string) (distro dist.Distribution, warn warn.Warn, err error) { d := strings.Split(distroString, "@") if d[0] == "" || len(d) == 0 { return distro, warn, errors.Errorf("distro's versions %s cannot be specified without distro's name", style.Symbol("@"+strings.Join(d[1:], "@"))) @@ -80,12 +80,13 @@ func ParseDistro(distroString string) (distro dist.Distribution, warn *warn.Warn return distro, warn, err } -func getTarget(t string) (nonDistro []string, distros string, warn *warn.Warn, err error) { +func getTarget(t string) (nonDistro []string, distros string, warn warn.Warn, err error) { target := strings.Split(t, ":") if i, err := getSliceAt[string](target, 0); err != nil { return nonDistro, distros, warn, errors.Errorf("invalid target %s, atleast one of [os][/arch][/archVariant] must be specified", t) } else if len(target) == 2 && target[0] == "" { - warn.Add(style.Warn("adding distros %s without [os][/arch][/variant]", target[2])) + v,_ := getSliceAt[string](target, 1) + warn.Add(style.Warn("adding distros %s without [os][/arch][/variant]", v)) } else { nonDistro = strings.Split(i, "/") } diff --git a/internal/target/parse_test.go b/internal/target/parse_test.go index b4d51ee37d..3c15cb24cc 100644 --- a/internal/target/parse_test.go +++ b/internal/target/parse_test.go @@ -25,9 +25,9 @@ func testParseTargets(t *testing.T, when spec.G, it spec.S) { }) when("target#ParseTarget", func() { - it("should throw an error when [os][/arch][/variant] is nil", func() { - _,_, err := target.ParseTarget(":distro@version") - h.AssertNotNil(t, err) + it("should show a warn when [os][/arch][/variant] is nil", func() { + _, warn, _ := target.ParseTarget(":distro@version") + h.AssertNotNil(t, warn.Messages) }) it("should parse target as expected", func() { output,_, err := target.ParseTarget("linux/arm/v6") @@ -45,7 +45,7 @@ func testParseTargets(t *testing.T, when spec.G, it spec.S) { h.AssertNotNil(t, err) }) it("should parse targets as expected", func() { - output,_, err := target.ParseTargets([]string{"linux/arm/v6", "linux/amd:ubuntu@22.04;debian@8.10@10.06"}) + output,_, err := target.ParseTargets([]string{"linux/arm/v6", "linux/amd64:ubuntu@22.04;debian@8.10@10.06"}) h.AssertNil(t, err) h.AssertEq(t, output, []dist.Target{ { @@ -55,7 +55,7 @@ func testParseTargets(t *testing.T, when spec.G, it spec.S) { }, { OS: "linux", - Arch: "amd", + Arch: "amd64", Distributions: []dist.Distribution{ { Name: "ubuntu", diff --git a/internal/target/platform.go b/internal/target/platform.go index 8ca085d2b8..3122d3b789 100644 --- a/internal/target/platform.go +++ b/internal/target/platform.go @@ -9,7 +9,7 @@ import ( "github.com/buildpacks/pack/internal/warn" ) -func getPlatform(t []string) (os, arch, variant string, warn *warn.Warn, err error) { +func getPlatform(t []string) (os, arch, variant string, warn warn.Warn, err error) { os, _ = getSliceAt[string](t, 0) arch, _ = getSliceAt[string](t, 1) variant, _ = getSliceAt[string](t, 2) diff --git a/internal/warn/warn.go b/internal/warn/warn.go index 5a6f6b18b0..e4b6a65d1b 100644 --- a/internal/warn/warn.go +++ b/internal/warn/warn.go @@ -4,17 +4,17 @@ type Warn struct { Messages []string } -func(warn *Warn) Add(mesage string) *Warn { +func(warn Warn) Add(mesage string) Warn { warn.Messages = append(warn.Messages, mesage) return warn } -func(warn *Warn) AddSlice(messages... string) *Warn { +func(warn Warn) AddSlice(messages... string) Warn { warn.Messages = append(warn.Messages, messages...) return warn } -func(w *Warn) AddWarn(warn *Warn) *Warn { +func(w Warn) AddWarn(warn Warn) Warn { w.Messages = append(w.Messages, warn.Messages...) return w } \ No newline at end of file From e7902f875b63396d8b76d89d21282f32dc8ab282 Mon Sep 17 00:00:00 2001 From: WYGIN Date: Sun, 1 Oct 2023 16:30:18 +0000 Subject: [PATCH 12/13] lint fix Signed-off-by: WYGIN --- internal/commands/buildpack_new.go | 2 +- internal/target/parse.go | 10 ++++++---- internal/target/parse_test.go | 6 +++--- internal/warn/warn.go | 15 ++++++--------- 4 files changed, 16 insertions(+), 17 deletions(-) diff --git a/internal/commands/buildpack_new.go b/internal/commands/buildpack_new.go index 6904658ce0..9ee62679fe 100644 --- a/internal/commands/buildpack_new.go +++ b/internal/commands/buildpack_new.go @@ -71,7 +71,7 @@ func BuildpackNew(logger logging.Logger, creator BuildpackCreator) *cobra.Comman } targets, warn, err := target.ParseTargets(flags.Targets) - for _,w := range warn.Messages { + for _, w := range warn.Messages { logger.Warn(w) } if err != nil { diff --git a/internal/target/parse.go b/internal/target/parse.go index dda23b84c9..7efc079c79 100644 --- a/internal/target/parse.go +++ b/internal/target/parse.go @@ -32,7 +32,7 @@ func ParseTarget(t string) (output dist.Target, warn warn.Warn, err error) { if err != nil { return output, warn, err } - os, arch, variant,w, err := getPlatform(nonDistro) + os, arch, variant, w, err := getPlatform(nonDistro) warn.AddWarn(w) if err != nil { return output, warn, err @@ -82,12 +82,14 @@ func ParseDistro(distroString string) (distro dist.Distribution, warn warn.Warn, func getTarget(t string) (nonDistro []string, distros string, warn warn.Warn, err error) { target := strings.Split(t, ":") - if i, err := getSliceAt[string](target, 0); err != nil { + if _, err := getSliceAt[string](target, 0); err != nil { return nonDistro, distros, warn, errors.Errorf("invalid target %s, atleast one of [os][/arch][/archVariant] must be specified", t) - } else if len(target) == 2 && target[0] == "" { - v,_ := getSliceAt[string](target, 1) + } + if len(target) == 2 && target[0] == "" { + v, _ := getSliceAt[string](target, 1) warn.Add(style.Warn("adding distros %s without [os][/arch][/variant]", v)) } else { + i, _ := getSliceAt[string](target, 0) nonDistro = strings.Split(i, "/") } if i, err := getSliceAt[string](target, 1); err == nil { diff --git a/internal/target/parse_test.go b/internal/target/parse_test.go index 3c15cb24cc..541263fb9a 100644 --- a/internal/target/parse_test.go +++ b/internal/target/parse_test.go @@ -30,7 +30,7 @@ func testParseTargets(t *testing.T, when spec.G, it spec.S) { h.AssertNotNil(t, warn.Messages) }) it("should parse target as expected", func() { - output,_, err := target.ParseTarget("linux/arm/v6") + output, _, err := target.ParseTarget("linux/arm/v6") h.AssertNil(t, err) h.AssertEq(t, output, dist.Target{ OS: "linux", @@ -41,11 +41,11 @@ func testParseTargets(t *testing.T, when spec.G, it spec.S) { }) when("target#ParseTargets", func() { it("should throw an error when atleast one target throws error", func() { - _,_, err := target.ParseTargets([]string{"linux/arm/v6", ":distro@version"}) + _, _, err := target.ParseTargets([]string{"linux/arm/v6", ":distro@version"}) h.AssertNotNil(t, err) }) it("should parse targets as expected", func() { - output,_, err := target.ParseTargets([]string{"linux/arm/v6", "linux/amd64:ubuntu@22.04;debian@8.10@10.06"}) + output, _, err := target.ParseTargets([]string{"linux/arm/v6", "linux/amd64:ubuntu@22.04;debian@8.10@10.06"}) h.AssertNil(t, err) h.AssertEq(t, output, []dist.Target{ { diff --git a/internal/warn/warn.go b/internal/warn/warn.go index e4b6a65d1b..07829f0c37 100644 --- a/internal/warn/warn.go +++ b/internal/warn/warn.go @@ -4,17 +4,14 @@ type Warn struct { Messages []string } -func(warn Warn) Add(mesage string) Warn { - warn.Messages = append(warn.Messages, mesage) - return warn +func (warn *Warn) Add(message string) { + warn.Messages = append(warn.Messages, message) } -func(warn Warn) AddSlice(messages... string) Warn { +func (warn *Warn) AddSlice(messages ...string) { warn.Messages = append(warn.Messages, messages...) - return warn } -func(w Warn) AddWarn(warn Warn) Warn { - w.Messages = append(w.Messages, warn.Messages...) - return w -} \ No newline at end of file +func (warn *Warn) AddWarn(w Warn) { + warn.Messages = append(warn.Messages, w.Messages...) +} From cdd941717a891708300d4db6e2032bb184f34dec Mon Sep 17 00:00:00 2001 From: WYGIN Date: Tue, 3 Oct 2023 05:54:30 +0000 Subject: [PATCH 13/13] will not be added to when is defined and not defined Signed-off-by: WYGIN --- internal/commands/buildpack_new.go | 20 ++++---- internal/commands/buildpack_new_test.go | 1 - internal/target/parse.go | 64 +++++++++++-------------- internal/target/parse_test.go | 50 +++++++++++++++---- internal/target/platform.go | 14 +++--- internal/warn/warn.go | 17 ------- 6 files changed, 90 insertions(+), 76 deletions(-) delete mode 100644 internal/warn/warn.go diff --git a/internal/commands/buildpack_new.go b/internal/commands/buildpack_new.go index 9ee62679fe..263cfd3753 100644 --- a/internal/commands/buildpack_new.go +++ b/internal/commands/buildpack_new.go @@ -70,12 +70,16 @@ func BuildpackNew(logger logging.Logger, creator BuildpackCreator) *cobra.Comman }) } - targets, warn, err := target.ParseTargets(flags.Targets) - for _, w := range warn.Messages { - logger.Warn(w) - } - if err != nil { - return err + var targets []dist.Target + if len(flags.Targets) == 0 && len(flags.Stacks) == 0 { + targets = []dist.Target{{ + OS: runtime.GOOS, + Arch: runtime.GOARCH, + }} + } else { + if targets, err = target.ParseTargets(flags.Targets, logger); err != nil { + return err + } } if err := creator.NewBuildpack(cmd.Context(), client.NewBuildpackOptions{ @@ -97,9 +101,9 @@ func BuildpackNew(logger logging.Logger, creator BuildpackCreator) *cobra.Comman cmd.Flags().StringVarP(&flags.API, "api", "a", "0.8", "Buildpack API compatibility of the generated buildpack") cmd.Flags().StringVarP(&flags.Path, "path", "p", "", "Path to generate the buildpack") cmd.Flags().StringVarP(&flags.Version, "version", "V", "1.0.0", "Version of the generated buildpack") - cmd.Flags().StringSliceVarP(&flags.Stacks, "stacks", "s", []string{}, "Stack(s) this buildpack will be compatible with"+stringSliceHelp("stack")) + cmd.Flags().StringSliceVarP(&flags.Stacks, "stacks", "s", nil, "Stack(s) this buildpack will be compatible with"+stringSliceHelp("stack")) cmd.Flags().MarkDeprecated("stacks", "prefer `--targets` instead: https://github.com/buildpacks/rfcs/blob/main/text/0096-remove-stacks-mixins.md") - cmd.Flags().StringSliceVarP(&flags.Targets, "targets", "t", []string{runtime.GOOS + "/" + runtime.GOARCH}, + cmd.Flags().StringSliceVarP(&flags.Targets, "targets", "t", nil, `Targets are the list platforms that one targeting, these are generated as part of scaffolding inside buildpack.toml file. one can provide target platforms in format [os][/arch][/variant]:[distroname@osversion@anotherversion];[distroname@osversion] - Base case for two different architectures : '--targets "linux/amd64" --targets "linux/arm64"' - case for distribution version: '--targets "windows/amd64:windows-nano@10.0.19041.1415"' diff --git a/internal/commands/buildpack_new_test.go b/internal/commands/buildpack_new_test.go index b23be1f24d..4a988672df 100644 --- a/internal/commands/buildpack_new_test.go +++ b/internal/commands/buildpack_new_test.go @@ -186,7 +186,6 @@ func testBuildpackNewCommand(t *testing.T, when spec.G, it spec.S) { ID: "io.buildpacks.stacks.jammy", Mixins: []string{}, }}, - Targets: targets, }).Return(nil).MaxTimes(1) path := filepath.Join(tmpDir, "stacks") diff --git a/internal/target/parse.go b/internal/target/parse.go index 7efc079c79..f5ea955892 100644 --- a/internal/target/parse.go +++ b/internal/target/parse.go @@ -1,46 +1,41 @@ package target import ( - "fmt" "strings" "github.com/pkg/errors" "github.com/buildpacks/pack/internal/style" - "github.com/buildpacks/pack/internal/warn" "github.com/buildpacks/pack/pkg/dist" + "github.com/buildpacks/pack/pkg/logging" ) -func ParseTargets(t []string) (targets []dist.Target, warn warn.Warn, err error) { +func ParseTargets(t []string, logger logging.Logger) (targets []dist.Target, err error) { for _, v := range t { - target, w, err := ParseTarget(v) - warn.AddWarn(w) + target, err := ParseTarget(v, logger) if err != nil { - return nil, warn, err + return nil, err } targets = append(targets, target) } - return targets, warn, nil + return targets, nil } -func ParseTarget(t string) (output dist.Target, warn warn.Warn, err error) { - nonDistro, distros, w, err := getTarget(t) - warn.AddWarn(w) +func ParseTarget(t string, logger logging.Logger) (output dist.Target, err error) { + nonDistro, distros, err := getTarget(t, logger) if v, _ := getSliceAt[string](nonDistro, 0); len(nonDistro) <= 1 && v == "" { - warn.Add(style.Error("os/arch must be defined")) + logger.Warn("os/arch must be defined") } if err != nil { - return output, warn, err + return output, err } - os, arch, variant, w, err := getPlatform(nonDistro) - warn.AddWarn(w) + os, arch, variant, err := getPlatform(nonDistro, logger) if err != nil { - return output, warn, err + return output, err } - v, w, err := ParseDistros(distros) - warn.AddWarn(w) + v, err := ParseDistros(distros, logger) if err != nil { - return output, warn, err + return output, err } output = dist.Target{ OS: os, @@ -48,46 +43,45 @@ func ParseTarget(t string) (output dist.Target, warn warn.Warn, err error) { ArchVariant: variant, Distributions: v, } - return output, warn, err + return output, err } -func ParseDistros(distroSlice string) (distros []dist.Distribution, warn warn.Warn, err error) { +func ParseDistros(distroSlice string, logger logging.Logger) (distros []dist.Distribution, err error) { distro := strings.Split(distroSlice, ";") if l := len(distro); l == 1 && distro[0] == "" { - return nil, warn, err + return nil, err } for _, d := range distro { - v, w, err := ParseDistro(d) - warn.AddWarn(w) + v, err := ParseDistro(d, logger) if err != nil { - return nil, warn, err + return nil, err } distros = append(distros, v) } - return distros, warn, nil + return distros, nil } -func ParseDistro(distroString string) (distro dist.Distribution, warn warn.Warn, err error) { +func ParseDistro(distroString string, logger logging.Logger) (distro dist.Distribution, err error) { d := strings.Split(distroString, "@") if d[0] == "" || len(d) == 0 { - return distro, warn, errors.Errorf("distro's versions %s cannot be specified without distro's name", style.Symbol("@"+strings.Join(d[1:], "@"))) + return distro, errors.Errorf("distro's versions %s cannot be specified without distro's name", style.Symbol("@"+strings.Join(d[1:], "@"))) } - if len(d) <= 2 && d[0] == "" { - warn.Add(fmt.Sprintf("distro with name %s has no specific version!", style.Symbol(d[0]))) + if len(d) <= 2 && (strings.Contains(strings.Join(d[1:], ""), "") || d[1] == "") { + logger.Warnf("distro with name %s has no specific version!", style.Symbol(d[0])) } distro.Name = d[0] distro.Versions = d[1:] - return distro, warn, err + return distro, err } -func getTarget(t string) (nonDistro []string, distros string, warn warn.Warn, err error) { +func getTarget(t string, logger logging.Logger) (nonDistro []string, distros string, err error) { target := strings.Split(t, ":") - if _, err := getSliceAt[string](target, 0); err != nil { - return nonDistro, distros, warn, errors.Errorf("invalid target %s, atleast one of [os][/arch][/archVariant] must be specified", t) + if (len(target) == 1 && target[0] == "") || len(target) == 0 { + return nonDistro, distros, errors.Errorf("invalid target %s, atleast one of [os][/arch][/archVariant] must be specified", t) } if len(target) == 2 && target[0] == "" { v, _ := getSliceAt[string](target, 1) - warn.Add(style.Warn("adding distros %s without [os][/arch][/variant]", v)) + logger.Warn(style.Warn("adding distros %s without [os][/arch][/variant]", v)) } else { i, _ := getSliceAt[string](target, 0) nonDistro = strings.Split(i, "/") @@ -95,7 +89,7 @@ func getTarget(t string) (nonDistro []string, distros string, warn warn.Warn, er if i, err := getSliceAt[string](target, 1); err == nil { distros = i } - return nonDistro, distros, warn, err + return nonDistro, distros, err } func getSliceAt[T interface{}](slice []T, index int) (value T, err error) { diff --git a/internal/target/parse_test.go b/internal/target/parse_test.go index 541263fb9a..030f2c772d 100644 --- a/internal/target/parse_test.go +++ b/internal/target/parse_test.go @@ -1,6 +1,7 @@ package target_test import ( + "bytes" "testing" "github.com/heroku/color" @@ -9,6 +10,7 @@ import ( "github.com/buildpacks/pack/internal/target" "github.com/buildpacks/pack/pkg/dist" + "github.com/buildpacks/pack/pkg/logging" h "github.com/buildpacks/pack/testhelpers" ) @@ -19,18 +21,22 @@ func TestParseTargets(t *testing.T) { } func testParseTargets(t *testing.T, when spec.G, it spec.S) { + outBuf := bytes.Buffer{} it.Before(func() { + outBuf = bytes.Buffer{} + h.AssertEq(t, outBuf.String(), "") var err error h.AssertNil(t, err) }) when("target#ParseTarget", func() { it("should show a warn when [os][/arch][/variant] is nil", func() { - _, warn, _ := target.ParseTarget(":distro@version") - h.AssertNotNil(t, warn.Messages) + target.ParseTarget(":distro@version", logging.NewLogWithWriters(&outBuf, &outBuf)) + h.AssertNotEq(t, outBuf.String(), "") }) it("should parse target as expected", func() { - output, _, err := target.ParseTarget("linux/arm/v6") + output, err := target.ParseTarget("linux/arm/v6", logging.NewLogWithWriters(&outBuf, &outBuf)) + h.AssertEq(t, outBuf.String(), "") h.AssertNil(t, err) h.AssertEq(t, output, dist.Target{ OS: "linux", @@ -38,14 +44,30 @@ func testParseTargets(t *testing.T, when spec.G, it spec.S) { ArchVariant: "v6", }) }) + it("should return an error", func() { + _, err := target.ParseTarget("", logging.NewLogWithWriters(&outBuf, &outBuf)) + h.AssertNotNil(t, err) + }) + it("should log a warning when only [os] has typo or is unknown", func() { + target.ParseTarget("os/arm/v6", logging.NewLogWithWriters(&outBuf, &outBuf)) + h.AssertNotEq(t, outBuf.String(), "") + }) + it("should log a warning when only [arch] has typo or is unknown", func() { + target.ParseTarget("darwin/arm/v6", logging.NewLogWithWriters(&outBuf, &outBuf)) + h.AssertNotEq(t, outBuf.String(), "") + }) + it("should log a warning when only [variant] has typo or is unknown", func() { + target.ParseTarget("linux/arm/unknown", logging.NewLogWithWriters(&outBuf, &outBuf)) + h.AssertNotEq(t, outBuf.String(), "") + }) }) when("target#ParseTargets", func() { it("should throw an error when atleast one target throws error", func() { - _, _, err := target.ParseTargets([]string{"linux/arm/v6", ":distro@version"}) + _, err := target.ParseTargets([]string{"linux/arm/v6", ":distro@version"}, logging.NewLogWithWriters(&outBuf, &outBuf)) h.AssertNotNil(t, err) }) it("should parse targets as expected", func() { - output, _, err := target.ParseTargets([]string{"linux/arm/v6", "linux/amd64:ubuntu@22.04;debian@8.10@10.06"}) + output, err := target.ParseTargets([]string{"linux/arm/v6", "linux/amd64:ubuntu@22.04;debian@8.10@10.06"}, logging.NewLogWithWriters(&outBuf, &outBuf)) h.AssertNil(t, err) h.AssertEq(t, output, []dist.Target{ { @@ -72,17 +94,25 @@ func testParseTargets(t *testing.T, when spec.G, it spec.S) { }) when("target#ParseDistro", func() { it("should parse distro as expected", func() { - output, _, err := target.ParseDistro("ubuntu@22.04@20.08") + output, err := target.ParseDistro("ubuntu@22.04@20.08", logging.NewLogWithWriters(&outBuf, &outBuf)) h.AssertEq(t, output, dist.Distribution{ Name: "ubuntu", Versions: []string{"22.04", "20.08"}, }) h.AssertNil(t, err) }) + it("should return an error", func() { + _, err := target.ParseDistro("@22.04@20.08", logging.NewLogWithWriters(&outBuf, &outBuf)) + h.AssertNotNil(t, err) + }) + it("should warn when distro version is not specified", func() { + target.ParseDistro("ubuntu", logging.NewLogWithWriters(&outBuf, &outBuf)) + h.AssertNotEq(t, outBuf.String(), "") + }) }) when("target#ParseDistros", func() { it("should parse distros as expected", func() { - output, _, err := target.ParseDistros("ubuntu@22.04@20.08;debian@8.10@10.06") + output, err := target.ParseDistros("ubuntu@22.04@20.08;debian@8.10@10.06", logging.NewLogWithWriters(&outBuf, &outBuf)) h.AssertEq(t, output, []dist.Distribution{ { Name: "ubuntu", @@ -96,9 +126,13 @@ func testParseTargets(t *testing.T, when spec.G, it spec.S) { h.AssertNil(t, err) }) it("result should be nil", func() { - output, _, err := target.ParseDistros("") + output, err := target.ParseDistros("", logging.NewLogWithWriters(&outBuf, &outBuf)) h.AssertEq(t, output, []dist.Distribution(nil)) h.AssertNil(t, err) }) + it("should return an error", func() { + _, err := target.ParseDistros(";", logging.NewLogWithWriters(&outBuf, &outBuf)) + h.AssertNotNil(t, err) + }) }) } diff --git a/internal/target/platform.go b/internal/target/platform.go index 3122d3b789..4d48d6845f 100644 --- a/internal/target/platform.go +++ b/internal/target/platform.go @@ -6,26 +6,26 @@ import ( "github.com/pkg/errors" "github.com/buildpacks/pack/internal/style" - "github.com/buildpacks/pack/internal/warn" + "github.com/buildpacks/pack/pkg/logging" ) -func getPlatform(t []string) (os, arch, variant string, warn warn.Warn, err error) { +func getPlatform(t []string, logger logging.Logger) (os, arch, variant string, err error) { os, _ = getSliceAt[string](t, 0) arch, _ = getSliceAt[string](t, 1) variant, _ = getSliceAt[string](t, 2) if !supportsOS(os) && supportsVariant(arch, variant) { - warn.Add(style.Warn("unknown os %s, is this a typo", os)) + logger.Warn(style.Warn("unknown os %s, is this a typo", os)) } if supportsArch(os, arch) && !supportsVariant(arch, variant) { - warn.Add(style.Warn("unknown variant %s", variant)) + logger.Warn(style.Warn("unknown variant %s", variant)) } if supportsOS(os) && !supportsArch(os, arch) && supportsVariant(arch, variant) { - warn.Add(style.Warn("unknown arch %s", arch)) + logger.Warn(style.Warn("unknown arch %s", arch)) } if !SupportsPlatform(os, arch, variant) { - return os, arch, variant, warn, errors.Errorf("unknown target: %s", style.Symbol(strings.Join(t, "/"))) + return os, arch, variant, errors.Errorf("unknown target: %s", style.Symbol(strings.Join(t, "/"))) } - return os, arch, variant, warn, err + return os, arch, variant, err } var supportedOSArchs = map[string][]string{ diff --git a/internal/warn/warn.go b/internal/warn/warn.go deleted file mode 100644 index 07829f0c37..0000000000 --- a/internal/warn/warn.go +++ /dev/null @@ -1,17 +0,0 @@ -package warn - -type Warn struct { - Messages []string -} - -func (warn *Warn) Add(message string) { - warn.Messages = append(warn.Messages, message) -} - -func (warn *Warn) AddSlice(messages ...string) { - warn.Messages = append(warn.Messages, messages...) -} - -func (warn *Warn) AddWarn(w Warn) { - warn.Messages = append(warn.Messages, w.Messages...) -}