-
Notifications
You must be signed in to change notification settings - Fork 293
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.
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
Enable package-buildpack for windows #840
Conversation
Codecov Report
@@ Coverage Diff @@
## main #840 +/- ##
==========================================
- Coverage 76.15% 75.56% -0.59%
==========================================
Files 87 81 -6
Lines 4464 4213 -251
==========================================
- Hits 3399 3183 -216
+ Misses 710 690 -20
+ Partials 355 340 -15
Flags with carried forward coverage won't be shown. Click here to find out more. |
4866a02
to
b25523f
Compare
Awesome to see this working! Just to start the discussion, is there a detailed description we can give for the behavior here? What's images are created for local, publish, file? Which layers are created in them and in which format for each case? (and if not ready for review no worries) |
b25523f
to
8b006ab
Compare
67a5e0d
to
c9815f7
Compare
89e29d0
to
ba9942f
Compare
5bfd35d
to
881fa34
Compare
@@ -346,6 +347,7 @@ func testWithoutSpecificBuilderRequirement( | |||
"package-buildpack", packageName, | |||
"-c", aggregatePackageToml, | |||
"--publish", | |||
"--os", dockerHostOS(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I 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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks really good. I did notice that Windows buildpackage support seems to be missing the experimental flag requirement. Other than that, this is looking great!
37d87f2
to
8b22e5c
Compare
Signed-off-by: Anthony Emengo <[email protected]> Signed-off-by: Andrew Meyer <[email protected]> Signed-off-by: Victoria Henry <[email protected]>
Signed-off-by: Micah Young <[email protected]>
- Important for publish scenarios to not depend on daemon Signed-off-by: Micah Young <[email protected]>
Signed-off-by: Micah Young <[email protected]>
8b22e5c
to
a4e1055
Compare
Great catch. Now fixed! I added the experimental flag for both |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is great! Clean implementation.
Summary
This change enables creation of Windows buildpackages. Windows buildpackages differ from Linux buildpackages in these ways:
Linux buildpackage image (no change from main)
os
property islinux
/cnb/buildpacks/...
Linux buildpackage file (no change from main)
.cnb
Windows buildpackage image (new)
os
property iswindows
(depends on: Adds setters for OS/OSVersion/Architecture imgutil#66)Files/cnb/buildpacks/...
pack
ordocker
(depends on: Adds support for creating Windows scratch-equivalent baselayers imgutil#64)Windows buildpackage file (new)
.cnb
Before
The
package-buildpack
command always generated buildpackage files/images as Linux OCI images.After
The
package-buildpack
command preserves existing behavior but supports creating files/images as Windows OCI images based on--os=windows
flags or daemon-based OS detection.Preserves existing behavior (with or without pack config
experimental=true
):package-buildpack --format=file ...
=> Linux buildpackage file (No daemon required)package-buildpack --format=image --publish ...
=> Linux buildpackage image on registry (No daemon required)package-buildpack --format=image ...
=> Linux buildpackage image on daemon (when Linux daemon present)With pack config
experimental=true
:package-buildpack --format=file --os=windows ...
=> Windows buildpackage file (No daemon required.)package-buildpack --format=image --publish --os=windows...
=> Windows buildpackage image on registry (No daemon required)package-buildpack --format=image --os=windows ...
=> Windows buildpackage image on daemon (when WCOW daemon present, otherwise fail)package-buildpack --format=image ...
=> Linux/Windows buildpackage image on daemon (when Linux/WCOW daemon present, resp)With pack config
experimental
not enabled:package-buildpack --os=linux
=> Fails with experimental support missing for "os" flagpackage-buildpack --format=image ...
=> Fails with experimental support missing for Windows packages (when Windows daemon present)Example artifacts:
cnbs/sample-package:hello-world-windows (built from draft PR):
hello-universe-windows.cnb
(Google Drive download)dive micahyoung/sample-package:hello-universe-windows
(requires WCOW daemon)Signed-off-by: Victoria Henry [email protected]
Signed-off-by: Andrew Meyer [email protected]
Signed-off-by: Micah Young [email protected]