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.

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

Add Windows builder and buildpacks #85

Merged
merged 13 commits into from
Aug 24, 2020

Conversation

micahyoung
Copy link
Member

@micahyoung micahyoung commented Aug 12, 2020

This adds:

  • buildpacks/hello-world-windows
    • .bat clone of hello-world
  • builders/nanoserver-1809
    • uses stack nanoserver-1809
    • uses buildpacks/hello-world-windows
  • apps/batch-script
    • .bat clone of bash-script
  • apps/batch-script/batch-script-buildpack
    • .bat clone of bash-script-buildpack

Additional:

  • Fix nanoserver UID/GID
  • Fix bash-script app ASCII art

Known issues:

  • Windows pack doesn't yet support directory-based buildpacks so the Makefile has a workaround
  • build-packages don't yet work for Windows daemons so no examples are included

Signed-off-by: Micah Young ymicah@vmware.com

@dfreilich
Copy link
Member

Closes #53

@micahyoung micahyoung force-pushed the windows-builder-and-bps branch 2 times, most recently from bb6c0d0 to 7b5438b Compare August 12, 2020 16:16
@micahyoung micahyoung marked this pull request as ready for review August 12, 2020 16:25
@micahyoung micahyoung requested a review from a team as a code owner August 12, 2020 16:25
@jromero jromero linked an issue Aug 12, 2020 that may be closed by this pull request
@micahyoung micahyoung force-pushed the windows-builder-and-bps branch 4 times, most recently from 6b0b061 to 0022039 Compare August 20, 2020 10:18
Micah Young and others added 12 commits August 20, 2020 12:21
- Change to pack flags to new syntax

Signed-off-by: Micah Young <ymicah@vmware.com>
- Add Makefile targets for Windows
- Fix ASCII art bin bash-app
- Change stack to use ContainerUser (avoid permissions issues on k8s and docker)

Signed-off-by: Micah Young <ymicah@vmware.com>
Signed-off-by: Micah Young <ymicah@vmware.com>
- tar up buildpacks when make runs on Windows

Signed-off-by: Micah Young <ymicah@vmware.com>
Signed-off-by: Javier Romero <rjavier@vmware.com>
- missing path option

Signed-off-by: Micah Young <ymicah@vmware.com>
Signed-off-by: Micah Young <ymicah@vmware.com>
- Add missing PATH env var to run image so that it gets properly set in app images
- Can be removed once buildpacks/pack#800 is resolved

Signed-off-by: Micah Young <ymicah@vmware.com>
Signed-off-by: Micah Young <ymicah@vmware.com>
Signed-off-by: Micah Young <ymicah@vmware.com>
Signed-off-by: Micah Young <ymicah@vmware.com>
- Workaround for missing Windows pack support for directory-based buildpacks
- Pivot samples-root to a temp path with tgz-buildpacks of the same name

Signed-off-by: Micah Young <ymicah@vmware.com>
Copy link
Member Author

@micahyoung micahyoung left a comment

Choose a reason for hiding this comment

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

Ready for review! All green now thanks to pack 0.13.

- uses: azure/docker-login@v1
if: github.event_name == 'push' && github.ref == 'refs/heads/main'
Copy link
Member Author

Choose a reason for hiding this comment

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

I had to gate this so it would pass on my repo branch which doesn't have a valid secret.

I also moved the step below Build so make build-*, which is usually more likely to fail than docker login, happen sooner.

@@ -6,7 +6,7 @@ PACK_CMD?=pack
## Linux
####################

build-linux: build-linux-stacks build-packages build-builders build-buildpacks
build-linux: build-linux-stacks build-linux-packages build-linux-builders build-linux-buildpacks
Copy link
Member Author

Choose a reason for hiding this comment

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

I moved all Linux targets that don't have alpine or bionic to have the linux in the name

Makefile Outdated Show resolved Hide resolved
Comment on lines +207 to +221
ifeq ($(OS),Windows_NT)
SAMPLES_ROOT:=$(shell mkdir -p .tmp && mktemp --directory -p . .tmp/samples-XXX)
build-sample-root:
@mkdir -p $(SAMPLES_ROOT)/buildpacks/

@for bp_dir in ./buildpacks/*/; do \
tar -czf $(SAMPLES_ROOT)/buildpacks/$$(basename $$bp_dir) -C $$bp_dir . ; \
done

@cp -r builders packages $(SAMPLES_ROOT)/
else
# No-op for posix pack
SAMPLES_ROOT:=.
build-sample-root:
endif
Copy link
Member Author

Choose a reason for hiding this comment

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

This makes build-linux- targets work for LCOW for anyone using bash.exe, and also let's build-windows- targets work for WCOW (if they are run from a Mac using DOCKER_HOST, this branch is not used).

This can all be removed once directory buildpacks are supported for pack on Windows.

RUN net users /ADD cnb /passwordreq:no /expires:never
# non-zero sets all user-owned directories to BUILTIN\Users
ARG cnb_uid=1
ARG cnb_gid=1

ENV CNB_USER_ID=${cnb_uid}
Copy link
Contributor

Choose a reason for hiding this comment

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

These vars are only needed in the build image, according to the spec

Copy link
Member Author

Choose a reason for hiding this comment

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

Good call. That's mostly to follow the pattern set by alpine and bionic's base/Dockerfile. We might need to file a separate issue to fix them all, but I feel like this nanoserver example works better by matching them, so Windows differences can be understood by contrast.

Copy link
Member

@natalieparellano natalieparellano left a comment

Choose a reason for hiding this comment

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

In general this looks great! I added a few nits and a couple suggestions for how the README / examples could be clearer.

stacks/nanoserver-1809/run/Dockerfile Outdated Show resolved Hide resolved
buildpacks/hello-world/bin/build Outdated Show resolved Hide resolved
buildpacks/hello-world-windows/buildpack.toml Outdated Show resolved Hide resolved
buildpacks/hello-world-windows/README.md Outdated Show resolved Hide resolved
builders/nanoserver-1809/README.md Outdated Show resolved Hide resolved
builders/nanoserver-1809/builder.toml Outdated Show resolved Hide resolved
apps/batch-script/project.toml Outdated Show resolved Hide resolved
apps/batch-script/batch-script-buildpack/bin/build.bat Outdated Show resolved Hide resolved
apps/batch-script/README.md Show resolved Hide resolved
apps/batch-script/app.bat Outdated Show resolved Hide resolved
Signed-off-by: Micah Young <ymicah@vmware.com>
@jromero jromero merged commit 8a9935c into buildpacks:main Aug 24, 2020
@jromero jromero mentioned this pull request Aug 25, 2020
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Need minimal buildpack that can be run on Windows
5 participants