Add ability to export/launch Windows-based images #310
Conversation
| @echo "> Packaging lifecycle for $(GOOS)..." | ||
| tar czf $(BUILD_DIR)/$(ARCHIVE_NAME).tgz -C $(GOOS_DIR) lifecycle.toml lifecycle | ||
| $(GOCMD) run tools/packager/main.go -os $(GOOS) -launcherExePath $(GOOS_DIR)/lifecycle/launcher -lifecycleExePath $(GOOS_DIR)/lifecycle/lifecycle -lifecycleVersion $(LIFECYCLE_VERSION) -platformAPI $(PLATFORM_API) -buildpackAPI $(BUILDPACK_API) -outputPackagePath $(BUILD_DIR)/$(ARCHIVE_NAME).tgz |
There was a problem hiding this comment.
Using the packager allows consistent tarball artifacts regardless of OS and identical target syntax. Issues were:
tar.exeis not equivalent to POSIX tar (i.e. can't do symlinks)- echoing
LIFECYCLE_DESCRIPTORto a file was messy on Windows and felt better outsideMakefile.
There was a problem hiding this comment.
I'm not attached to go run over go install like our other tools. Happy to change if preferred.
| docker run -v lifecycle-out:c:/lifecycle/out -e LIFECYCLE_VERSION -e PLATFORM_API -e BUILDPACK_API -v gopathcache:c:/gopath -v '\\.\pipe\docker_engine:\\.\pipe\docker_engine' --isolation=process --rm $(SOURCE_COMPILATION_IMAGE) $(DOCKER_CMD) | ||
| docker run -v lifecycle-out:c:/lifecycle/out --rm $(SOURCE_COMPILATION_IMAGE) tar -cf- out | tar -xf- | ||
| @docker volume rm -f lifecycle-out | ||
|
|
There was a problem hiding this comment.
This target is meant to be an easy and consistent wrapper for any Windows command (i.e. make unit|acceptance|test, make build-windows package-windows, go run ...)
Our hope is to make building, running, and troubleshooting on Windows extremely consistent for any CNB contributor and CI by ensuring everyone's using an identical Dockerfile-based Windows environment w/o any manual setup.
There was a problem hiding this comment.
An alternative implementation we tried was docker build w/o COPY . src, docker create, docker cp, docker start but symlinks were not copied over properly (ended up as empty files)
| - uses: actions/upload-artifact@v2 | ||
| with: | ||
| name: lifecycle-windows-x86-64 | ||
| path: out/lifecycle-v*+windows.x86-64.tgz |
There was a problem hiding this comment.
The Windows jobs ends up simpler than Linux since dependencies are in the tools/Dockerfile.windows and it's run through the make docker-run-windows target.
The tradeoff is that Windows build is slower than Linux (~7 mins vs ~4 mins resp) but I feel the consistency/simplicity is worth it. More thoughts on the Makefile#docker-run-windows target below
Keeping these test buidpacks as directory symlinks instead of file symlinks would at minimum break our It's worth nothing that this underlying issue with directory symlinks will also effect users during We recorded some experiments and findings here There's not a specific WCOW Docker/moby issue I can find for this, though a similar open issue for LCOW is present (which also would have effected |
ameyer-pivotal
changed the title
| volumeName := filepath.VolumeName(path) | ||
| path = strings.TrimPrefix(path, volumeName) | ||
| return filepath.ToSlash(path) |
There was a problem hiding this comment.
Question: Are the constraints we are addressing here constraints of the TAR header spec itself or constraints of the OCI spec and the format it expects from layers? I tried to find this requirement in the TAR spec but couldn't (doesn't mean they don't exist I just didn't find them in the time I allocated to looking).
There was a problem hiding this comment.
Interesting question... Originally, this wasn't based on any concrete evidence other than the patterns in our code for both pack and lifecycle. We chose to "normalize" the creation of tars to simplify the code (and because somewhere along the way it was stuck in our heads that tar headers always used forward slashes).
Looking at the TAR spec seems to be consistent...
For the slashes (vs backslashes), I found some evidence for that here:
The
namefield is the file name of the file, with directory names (if any) preceding the file name, separated by slashes.
The volume part is harder to nail down in the TAR spec as it's not mentioned in the above quote. Based on pages like this, it doesn't seem like volume names would be expected in the name field though. I'd imagine having a volume name in the header path would make untarring just have to do the same trimming that we're doing upon creation here.
| if len(profileCmds) > 0 { | ||
| launcher = strings.Join(profileCmds, " && ") + " && " | ||
| } |
There was a problem hiding this comment.
Joining with " && " should work for both bash and cmd
There was a problem hiding this comment.
I'm happy to do this, but we were trying to leave the Linux case rather untouched, as they were joined with newlines before.
sys/sys_unix.go
Outdated
| package sys | ||
|
|
||
| const ( | ||
| Root = "/" | ||
| ExecExt = "" | ||
| ) |
There was a problem hiding this comment.
I don't think these two constants deserve their own package. Can we move them to cmd? If there are import issues I think it is okay to repeat these constants in other packages. I would prioritize keeping the import graph tidy. There may be cases like the usage in restorer where we could remove them entirely (we can make UntarLayer default to the root when given an empty destination).
There was a problem hiding this comment.
The benefit to having these in their own package is that it addresses two concerns:
- Avoids import cycles since this can be seen as a "leaf" node in the import tree
- This was created when addressing the concern about keeping the default paths in sync between OSes (refactored separate OS-dependent files to a single file that uses
filepath.Join, etc. in combination with these constants)
It seems the second bullet would sort of go against repeating these constants in multiple places. I'll see if I can move them to a different package though.
Could you clarify what you meant about UntarLayer? I think using these there will still cause import cycles. I'll have to try though.
There was a problem hiding this comment.
So tinkering a bit, cmd seems like too high-level of a package to move this to without causing import cycles. How important is this? It feels like one of those cases that can go either way, depending on stylistic opinions. I'd argue that the import tree is simpler to understand and use with this as a leaf node (i.e. modular, self-contained packages), but other folks might argue that simpler means fewer packages altogether.
I see a few options:
- Leave as is
- Find a lower-level package where this could live instead
- Force it into
cmdand spend time debugging import cycles
Thoughts?
There was a problem hiding this comment.
@ekcasey Actually, it worked pretty nicely to move into the env package. That feels like a reasonable compromise. Let me know what you think :)
There was a problem hiding this comment.
It seems the second bullet would sort of go against repeating these constants in multiple places. I'll see if I can move them to a different package though.
I feel like this concern is less relevant for these constants than for the default input values. We will realistically change default input values at some point and we don't want linux/windows values to fall out of sync. The posix and windows fs root dirs on the other hand, should not change. This is how I would handle it:
I have removed some unnecessary usage of these constants leaving us with the original motivating usage in cmd and one other usage in archive (since we already have very specific OS-dependent path logic there it doesn't feel crazy to me to redefine these in that context).
There was a problem hiding this comment.
My main concern is that when these are exported they become part of the public package level interface for env and I don't think we want that. I would like the keep the import graph and the godoc as clean and straightforward as possible.
There was a problem hiding this comment.
OK. Makes sense. We'll pull in your suggested change :)
Signed-off-by: Andrew Meyer <[email protected]> Signed-off-by: Anthony Emengo <[email protected]> Signed-off-by: Micah Young <[email protected]>
Signed-off-by: Andrew Meyer <[email protected]>
- Add os-specific slash variable, for convenience - Move PWD redefinition into Windows-only if-case Signed-off-by: Andrew Meyer <[email protected]>
Signed-off-by: Andrew Meyer <[email protected]>
- Also fixed typo in makefile Signed-off-by: Andrew Meyer <[email protected]>
Signed-off-by: Andrew Meyer <[email protected]>
Signed-off-by: Andrew Meyer <[email protected]>
Signed-off-by: Andrew Meyer <[email protected]>
Signed-off-by: Andrew Meyer <[email protected]>
Signed-off-by: Andrew Meyer <[email protected]>
- Pending discussion on how to handle potential Docker-in-Docker needs for build-linux - Will open separate PR once other work is merged Signed-off-by: Micah Young <[email protected]>
Signed-off-by: Andrew Meyer <[email protected]>
- the Files/ Hives/ directories must be trimmed of the container image layer before writing to the container filesystem - Untar() -> UntarLayer() to justify adding container specific behavior Signed-off-by: Anthony Emengo <[email protected]>
- launcher increased in size due `runtime` import for `GOOS` detection - inlined commands do not work well on Windows Signed-off-by: Micah Young <[email protected]>
Signed-off-by: Anthony Emengo <[email protected]>
Signed-off-by: Micah Young <[email protected]>
- Allows faster builds - Inline OS check since it is only used for -symlinks target Signed-off-by: Micah Young <[email protected]>
Signed-off-by: Andrew Meyer <[email protected]> Signed-off-by: Anthony Emengo <[email protected]>
Signed-off-by: Andrew Meyer <[email protected]> Signed-off-by: Anthony Emengo <[email protected]>
Signed-off-by: Andrew Meyer <[email protected]> Signed-off-by: Anthony Emengo <[email protected]> Signed-off-by: Micah Young <[email protected]>
Signed-off-by: Andrew Meyer <[email protected]>
- Restore removed LDFLAGS - Bump linux compilation image go version Signed-off-by: Andrew Meyer <[email protected]>
- More robust 'Files' prefix trimming - Skip 'Hives' paths instead of trimming - Make tests easier to debug - Some minor test cleanup Signed-off-by: Andrew Meyer <[email protected]>
Signed-off-by: Andrew Meyer <[email protected]> Signed-off-by: Micah Young <[email protected]>
- Add LayerOS() helper to communicate intention Signed-off-by: Micah Young <[email protected]> Signed-off-by: Andrew Meyer <[email protected]>
testdata/buildpack/bin/detect.bat
Outdated
| @@ -0,0 +1,2 @@ | |||
| @echo off | |||
| bash %~dp0\detect %1 %2 | |||
There was a problem hiding this comment.
I was surprised to find that I couldn't run the detect unit tests on a windows machine w/o bash installed. The failures give the false impression that bash is a dependency of the detector. However the implementation is correctly calling detect.bat which in turn requires bash. Given that we generally won't have bash available when the lifecycle runs on windows I am not sure I am comfortable with the dependency in the test. It makes it impossible to validate that the lifecycle can execute correctly in an environment sans bash. Can we reimplement the detect test logic in the *.bat files?
There was a problem hiding this comment.
We can rewrite these in pure batch. The makefile provides a way to run the tests in a docker container that has all the necessary dependencies, but I understand the overall concerns.
Signed-off-by: Andrew Meyer <[email protected]>
- Skips all non-Files-prefixed entries - Make test fixture clearer Signed-off-by: Andrew Meyer <[email protected]> Signed-off-by: Malini Valliath <[email protected]>
Signed-off-by: Andrew Meyer <[email protected]> Signed-off-by: Malini Valliath <[email protected]>
Signed-off-by: Emily Casey <[email protected]>
- We were writing improper layers for restorer_test.go Signed-off-by: Andrew Meyer <[email protected]>
Signed-off-by: Andrew Meyer <[email protected]> Signed-off-by: Victoria Henry <[email protected]> Signed-off-by: Micah Young<[email protected]>
In general, changes relate to the following:
tar.NewWritercalls with a factory call which can get a layer writer appropriate for the target image OSSigned-off-by: Andrew Meyer [email protected]
Signed-off-by: Anthony Emengo [email protected]
Signed-off-by: Micah Young [email protected]