Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enable package-buildpack for windows #840

Merged
merged 4 commits into from
Oct 20, 2020
Merged

Enable package-buildpack for windows #840

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
85b8d7e
Enable package-buildpack for windows
Sep 2, 2020
b592d4b
Refactors Windows package-buildpack to use baselayer shim
Oct 6, 2020
af1a3ab
Add package-buildpack arg for OS
Oct 13, 2020
a4e1055
Removes skips for windows package tests
Oct 14, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
195 changes: 89 additions & 106 deletions acceptance/acceptance_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,6 +125,7 @@ func testWithoutSpecificBuilderRequirement(

it.Before(func() {
pack = invoke.NewPackInvoker(t, assert, packConfig, registryConfig.DockerConfigDir)
pack.EnableExperimental()
buildpackManager = buildpacks.NewBuildpackManager(t, assert)
})

Expand Down Expand Up @@ -240,8 +241,6 @@ func testWithoutSpecificBuilderRequirement(
"pack does not support 'package-buildpack'",
)

h.SkipIf(t, dockerHostOS() == "windows", "These tests are not yet compatible with Windows-based containers")

var err error
tmpDir, err = ioutil.TempDir("", "package-buildpack-tests")
assert.Nil(err)
Expand Down Expand Up @@ -327,15 +326,18 @@ func testWithoutSpecificBuilderRequirement(

when("--publish", func() {
it("publishes image to registry", func() {
h.SkipIf(t, !pack.Supports("package-buildpack --os"), "os not supported")

nestedPackageName := registryConfig.RepoName("test/package-" + h.RandString(10))

nestedPackage := buildpacks.NewPackageImage(
t,
pack,
nestedPackageName,
simplePackageConfigPath,
buildpacks.WithPublish(),
buildpacks.WithRequiredBuildpacks(buildpacks.SimpleLayers),
buildpacks.WithPublish(),
buildpacks.WithOS(dockerHostOS()),
)
buildpackManager.PrepareBuildpacks(tmpDir, nestedPackage)
defer h.DockerRmi(dockerCli, nestedPackageName)
Expand All @@ -346,6 +348,7 @@ func testWithoutSpecificBuilderRequirement(
"package-buildpack", packageName,
"-c", aggregatePackageToml,
"--publish",
"--os", dockerHostOS(),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if there's a way to infer this without a flag... I understand why we might not want to look at the dockerhost OS, as you might be cross-building. Though wouldn't that only work in a --publish scenario?

Back to the original question, is there some way we could derive this from the buildpack? Seems unlikely but thought I'd ask.

Copy link
Member

@micahyoung micahyoung Oct 20, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that's a good thing to discuss. Right now, for backward compatibility reasons, this implementation does still always default to linux for --publish or --format=file w/o the --os flag (it emits a warning telling you this) but for local images (which is the default for package-buildpack w/o options) it will autodetect the image type (without a warning) and save it on the dameon. Also for backward compatibility, it can't require a daemon for --publish or --format=file.

With that in mind, the autodetection options for --publish or --format=file might be to detect based on a optional daemon, but depending on if your daemon is running/stopped, you have a reachable/unreachable DOCKER_HOST set, or some other environment difference, you'd generate different images with the same command. I feel like autodetecting the image type from the dameon only really feels right in the case of a local image on the daemon, where it has to match the running daemon anyways and it unlikely to be pushed up to a registry or copied around.

Detecting from a buildpack would be hard as meta buildpacks wouldn't even have binary names to check bin/build vs bin/build{.bat,.exe}. We'd need each buildpack in the package to explicitly state that they are Windows and them all to agree.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could make the os an optional field in the package config though, instead of the --os flag. Then package-buildpack would consistently generate the same image time regardless of daemon. The only potential catch there is that to-date, the package config tends to describe CNB-related things (buildpack paths and images) inside a package and not things about the way it's packaged (ex: --format is specified on the command line, not in the package.toml).

)
defer h.DockerRmi(dockerCli, packageName)
assertions.NewOutputAssertionManager(t, output).ReportsPackagePublished(packageName)
Expand Down Expand Up @@ -537,8 +540,11 @@ func testWithoutSpecificBuilderRequirement(
pack,
packageFileLocation,
pack.FixtureManager().FixtureLocation("package_for_build_cmd.toml"),
buildpacks.FolderSimpleLayersParent,
buildpacks.FolderSimpleLayers,
buildpacks.WithRequiredBuildpacks(
buildpacks.FolderSimpleLayersParent,
buildpacks.FolderSimpleLayers,
),
buildpacks.WithOS(dockerHostOS()),
)

buildpackManager.PrepareBuildpacks(tmpDir, packageFile)
Expand All @@ -559,10 +565,6 @@ func testWithoutSpecificBuilderRequirement(
})

when("buildpack image", func() {
it.Before(func() {
h.SkipIf(t, dockerHostOS() == "windows", "These tests are not yet compatible with Windows-based containers")
})

when("inspect-buildpack", func() {
it("succeeds", func() {
packageImageName := registryConfig.RepoName("buildpack-" + h.RandString(8))
Expand Down Expand Up @@ -1244,12 +1246,8 @@ func testAcceptance(

it.Before(func() {
h.SkipUnless(t,
pack.Supports("package-buildpack"),
"--buildpack does not accept buildpackage unless package-buildpack is supported",
)
h.SkipIf(t,
dockerHostOS() == "windows",
"These tests are not yet compatible with Windows-based containers",
pack.Supports("package-buildpack --os"),
"--buildpack does not accept buildpackage unless package-buildpack --os is supported",
)
})

Expand Down Expand Up @@ -1297,11 +1295,10 @@ func testAcceptance(
var tmpDir string

it.Before(func() {
h.SkipIf(t,
!pack.Supports("package-buildpack --format"),
"--buildpack does not accept buildpackage file unless package-buildpack with --format is supported",
h.SkipUnless(t,
pack.Supports("package-buildpack --os"),
"--buildpack does not accept buildpackage unless package-buildpack --os is supported",
)
h.SkipIf(t, dockerHostOS() == "windows", "These tests are not yet compatible with Windows-based containers")

var err error
tmpDir, err = ioutil.TempDir("", "package-file")
Expand All @@ -1323,8 +1320,11 @@ func testAcceptance(
pack,
packageFileLocation,
pack.FixtureManager().FixtureLocation("package_for_build_cmd.toml"),
buildpacks.FolderSimpleLayersParent,
buildpacks.FolderSimpleLayers,
buildpacks.WithRequiredBuildpacks(
buildpacks.FolderSimpleLayersParent,
buildpacks.FolderSimpleLayers,
),
buildpacks.WithOS(dockerHostOS()),
)

buildpackManager.PrepareBuildpacks(tmpDir, packageFile)
Expand Down Expand Up @@ -2123,70 +2123,68 @@ func createComplexBuilder(t *testing.T,
"run_image_mirror": runImageMirror,
}

if dockerHostOS() != "windows" {
packageImageName := registryConfig.RepoName("nested-level-1-buildpack-" + h.RandString(8))
nestedLevelTwoBuildpackName := registryConfig.RepoName("nested-level-2-buildpack-" + h.RandString(8))
simpleLayersBuildpackName := registryConfig.RepoName("simple-layers-buildpack-" + h.RandString(8))

templateMapping["package_id"] = "simple/nested-level-1"
templateMapping["package_image_name"] = packageImageName
templateMapping["nested_level_1_buildpack"] = packageImageName
templateMapping["nested_level_2_buildpack"] = nestedLevelTwoBuildpackName
templateMapping["simple_layers_buildpack"] = simpleLayersBuildpackName

fixtureManager := pack.FixtureManager()

nestedLevelOneConfigFile, err := ioutil.TempFile(tmpDir, "nested-level-1-package.toml")
assert.Nil(err)
fixtureManager.TemplateFixtureToFile(
"nested-level-1-buildpack_package.toml",
nestedLevelOneConfigFile,
templateMapping,
)
err = nestedLevelOneConfigFile.Close()
assert.Nil(err)

nestedLevelTwoConfigFile, err := ioutil.TempFile(tmpDir, "nested-level-2-package.toml")
assert.Nil(err)
fixtureManager.TemplateFixtureToFile(
"nested-level-2-buildpack_package.toml",
nestedLevelTwoConfigFile,
templateMapping,
)
err = nestedLevelTwoConfigFile.Close()
assert.Nil(err)
packageImageName := registryConfig.RepoName("nested-level-1-buildpack-" + h.RandString(8))
nestedLevelTwoBuildpackName := registryConfig.RepoName("nested-level-2-buildpack-" + h.RandString(8))
simpleLayersBuildpackName := registryConfig.RepoName("simple-layers-buildpack-" + h.RandString(8))

packageImageBuildpack := buildpacks.NewPackageImage(
t,
pack,
packageImageName,
nestedLevelOneConfigFile.Name(),
buildpacks.WithRequiredBuildpacks(
buildpacks.NestedLevelOne,
buildpacks.NewPackageImage(
t,
pack,
nestedLevelTwoBuildpackName,
nestedLevelTwoConfigFile.Name(),
buildpacks.WithRequiredBuildpacks(
buildpacks.NestedLevelTwo,
buildpacks.NewPackageImage(
t,
pack,
simpleLayersBuildpackName,
fixtureManager.FixtureLocation("simple-layers-buildpack_package.toml"),
buildpacks.WithRequiredBuildpacks(buildpacks.SimpleLayers),
),
templateMapping["package_id"] = "simple/nested-level-1"
templateMapping["package_image_name"] = packageImageName
templateMapping["nested_level_1_buildpack"] = packageImageName
templateMapping["nested_level_2_buildpack"] = nestedLevelTwoBuildpackName
templateMapping["simple_layers_buildpack"] = simpleLayersBuildpackName

fixtureManager := pack.FixtureManager()

nestedLevelOneConfigFile, err := ioutil.TempFile(tmpDir, "nested-level-1-package.toml")
assert.Nil(err)
fixtureManager.TemplateFixtureToFile(
"nested-level-1-buildpack_package.toml",
nestedLevelOneConfigFile,
templateMapping,
)
err = nestedLevelOneConfigFile.Close()
assert.Nil(err)

nestedLevelTwoConfigFile, err := ioutil.TempFile(tmpDir, "nested-level-2-package.toml")
assert.Nil(err)
fixtureManager.TemplateFixtureToFile(
"nested-level-2-buildpack_package.toml",
nestedLevelTwoConfigFile,
templateMapping,
)
err = nestedLevelTwoConfigFile.Close()
assert.Nil(err)

packageImageBuildpack := buildpacks.NewPackageImage(
t,
pack,
packageImageName,
nestedLevelOneConfigFile.Name(),
buildpacks.WithRequiredBuildpacks(
buildpacks.NestedLevelOne,
buildpacks.NewPackageImage(
t,
pack,
nestedLevelTwoBuildpackName,
nestedLevelTwoConfigFile.Name(),
buildpacks.WithRequiredBuildpacks(
buildpacks.NestedLevelTwo,
buildpacks.NewPackageImage(
t,
pack,
simpleLayersBuildpackName,
fixtureManager.FixtureLocation("simple-layers-buildpack_package.toml"),
buildpacks.WithRequiredBuildpacks(buildpacks.SimpleLayers),
),
),
),
)
),
)

builderBuildpacks = append(
builderBuildpacks,
packageImageBuildpack,
)
}
builderBuildpacks = append(
builderBuildpacks,
packageImageBuildpack,
)

buildpackManager.PrepareBuildpacks(tmpDir, builderBuildpacks...)

Expand Down Expand Up @@ -2257,30 +2255,20 @@ func createBuilder(
buildpacks.ReadEnv,
}

// NOTE: Windows-based packages are not yet supported, so we'll add this buildpack in the usual way for now.
// Remove this block once Windows-based packages are supported.
if dockerHostOS() == "windows" {
builderBuildpacks = append(builderBuildpacks, buildpacks.SimpleLayers)
}

// NOTE: Windows-based packages are not yet supported, so we'll add this buildpack in the usual way for now (see above).
// Remove this guard once Windows-based packages are supported.
if dockerHostOS() != "windows" {
packageImageName := registryConfig.RepoName("simple-layers-package-image-buildpack-" + h.RandString(8))
packageImageName := registryConfig.RepoName("simple-layers-package-image-buildpack-" + h.RandString(8))

packageImageBuildpack := buildpacks.NewPackageImage(
t,
pack,
packageImageName,
pack.FixtureManager().FixtureLocation("package.toml"),
buildpacks.WithRequiredBuildpacks(buildpacks.SimpleLayers),
)
packageImageBuildpack := buildpacks.NewPackageImage(
t,
pack,
packageImageName,
pack.FixtureManager().FixtureLocation("package.toml"),
buildpacks.WithRequiredBuildpacks(buildpacks.SimpleLayers),
)

builderBuildpacks = append(builderBuildpacks, packageImageBuildpack)
builderBuildpacks = append(builderBuildpacks, packageImageBuildpack)

templateMapping["package_image_name"] = packageImageName
templateMapping["package_id"] = "simple/layers"
}
templateMapping["package_image_name"] = packageImageName
templateMapping["package_id"] = "simple/layers"

buildpackManager.PrepareBuildpacks(tmpDir, builderBuildpacks...)

Expand All @@ -2300,11 +2288,6 @@ func createBuilder(
// RENDER builder.toml
configFileName := "builder.toml"

// NOTE: Remove when Windows-based packages are supported (can use same toml at that point)
if dockerHostOS() == "windows" {
configFileName = "builder-windows.toml"
}

builderConfigFile, err := ioutil.TempFile(tmpDir, "builder.toml")
assert.Nil(err)

Expand Down
25 changes: 25 additions & 0 deletions acceptance/buildpacks/manager.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,3 +51,28 @@ func (b BuildpackManager) PrepareBuildpacks(destination string, buildpacks ...Te
b.assert.Nil(err)
}
}

type Modifiable interface {
SetOS(string)
SetPublish()
SetBuildpacks([]TestBuildpack)
}
type PackageModifier func(p Modifiable)

func WithRequiredBuildpacks(buildpacks ...TestBuildpack) PackageModifier {
return func(p Modifiable) {
p.SetBuildpacks(buildpacks)
}
}

func WithPublish() PackageModifier {
return func(p Modifiable) {
p.SetPublish()
}
}

func WithOS(osVal string) PackageModifier {
return func(p Modifiable) {
p.SetOS(osVal)
}
}
32 changes: 26 additions & 6 deletions acceptance/buildpacks/package_file_buildpack.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,22 +21,37 @@ type PackageFile struct {
destination string
sourceConfigLocation string
buildpacks []TestBuildpack
os string
}

func (p *PackageFile) SetOS(os string) {
p.os = os
}

func (p *PackageFile) SetBuildpacks(buildpacks []TestBuildpack) {
p.buildpacks = buildpacks
}

func (p *PackageFile) SetPublish() {}

func NewPackageFile(
t *testing.T,
pack *invoke.PackInvoker,
destination, configLocation string,
buildpacks ...TestBuildpack,
modifiers ...PackageModifier,
) PackageFile {

return PackageFile{
p := PackageFile{
testObject: t,
pack: pack,
destination: destination,
sourceConfigLocation: configLocation,
buildpacks: buildpacks,
}
for _, mod := range modifiers {
mod(&p)
}

return p
}

func (p PackageFile) Prepare(sourceDir, _ string) error {
Expand All @@ -59,13 +74,18 @@ func (p PackageFile) Prepare(sourceDir, _ string) error {
configLocation := filepath.Join(tmpDir, "package.toml")
h.CopyFile(p.testObject, p.sourceConfigLocation, configLocation)

output := p.pack.RunSuccessfully(
"package-buildpack",
packArgs := []string{
p.destination,
"--no-color",
"-c", configLocation,
"--format", "file",
)
}

if p.os != "" {
packArgs = append(packArgs, "--os", p.os)
}

output := p.pack.RunSuccessfully("package-buildpack", packArgs...)

if !strings.Contains(output, fmt.Sprintf("Successfully created package '%s'", p.destination)) {
return errors.New("failed to create package")
Expand Down
Loading