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 container support, simplify binary generation #6

Merged
merged 24 commits into from
May 14, 2021

Conversation

car-roll
Copy link
Contributor

@car-roll car-roll commented Apr 30, 2021

The goal is to simplify the build (i.e. binary generation) process.
There are now two Dockerfiles to represent a build using linux or windows environment. Each Dockerfile has a corresponding build script to exercise their own Dockerfile properly.

The linux container is straightforward, using docker build --output (i.e. BuildKit) and a multistage Dockerfile, we can generate the needed binaries and output them directly to target/classes.

Unfortunately for windows, there is no BuildKit support, yet (see microsoft/Windows-Containers#34). In place, there is a traditional single stage Dockerfile that generates the binaries. We then use docker create to add a writeable container layer to extract the binaries to target/classes.

  • Make sure you are opening from a topic/feature/bugfix branch (right side) and not your master branch!
  • Ensure that the pull request title represents the desired changelog entry
  • Please describe what you did
  • Link to relevant issues in GitHub or Jira
  • Link to relevant pull requests, esp. upstream and downstream changes
  • Ensure you have provided tests - that demonstrates feature works or fixes the issue

@car-roll car-roll mentioned this pull request Apr 30, 2021
6 tasks
@car-roll
Copy link
Contributor Author

Always some little issue. Looks like there is an issue for windows machines (possibly due to antivirus?) that causes an access denied error to the go.mod and go.sum files.
golang/go#36568

@car-roll car-roll changed the title Remove docker maven plugin, integrate unit tests to maven Use windows container, remove docker maven plugin, integrate unit tests to maven Apr 30, 2021
Copy link
Member

@jglick jglick left a comment

Choose a reason for hiding this comment

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

I am trying to figure out how all this fits together. I ran mvn package on this PR but it did not work:

$ jar tf target/lib-durable-task.jar
META-INF/MANIFEST.MF
META-INF/
META-INF/maven/
META-INF/maven/io.jenkins.plugins/
META-INF/maven/io.jenkins.plugins/lib-durable-task/
META-INF/maven/io.jenkins.plugins/lib-durable-task/pom.xml
META-INF/maven/io.jenkins.plugins/lib-durable-task/pom.properties

I suppose you forgot to package target/src/bin/? (Which is a really weird name BTW. src and target are mutually exclusive in Maven.)

pom.xml Show resolved Hide resolved
pom.xml Show resolved Hide resolved
src/Dockerfile Outdated Show resolved Hide resolved
src/generate-binaries.sh Outdated Show resolved Hide resolved
src/test-and-compile.bat Outdated Show resolved Hide resolved
src/test-and-compile.sh Outdated Show resolved Hide resolved
pom.xml Outdated Show resolved Hide resolved
@car-roll car-roll requested a review from batmat May 4, 2021 06:13
@car-roll car-roll changed the title Use windows container, remove docker maven plugin, integrate unit tests to maven Add windows container support, simplify binary generation May 9, 2021
Jenkinsfile Outdated Show resolved Hide resolved
@car-roll car-roll force-pushed the remove-docker-maven-plugin branch from f8ed5c4 to 9194a82 Compare May 9, 2021 05:55
@car-roll
Copy link
Contributor Author

car-roll commented May 9, 2021

Windows build claims it passes, but it actually fails do to the same access denied error referenced here: #6 (comment)

golang 1.16 is supposed to fix this issue. maybe not the same problem?

@car-roll
Copy link
Contributor Author

car-roll commented May 9, 2021

looks like the culprit was golang/go#31481
go mod tidy -modcacherw did not work, but running go get -modcacherw for every dependency did.

Copy link
Member

@jglick jglick left a comment

Choose a reason for hiding this comment

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

Still producing empty JARs.

@@ -0,0 +1,30 @@
ARG BASE_DIR=/durabletask
Copy link
Member

Choose a reason for hiding this comment

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

To get syntax coloring, suggest moving this to e.g. src/docker/linux/Dockerfile. github-linguist/linguist#4566

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So I gave it a whirl and came across the issue of ADD/COPY requiring the source files to reside in the build context. Don't think this will work for this repo as I am adding the go source files into the image

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, can be worked around by copying the desired Dockerfile along with other inputs into a temp directory and running docker build -t xxx target/xxx, but it is more hassle.

VER=$1
# output directory of binaries
DEST=$2
docker build --build-arg VERSION=$VER -o $DEST -f Dockerfile.linux .
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
docker build --build-arg VERSION=$VER -o $DEST -f Dockerfile.linux .
export DOCKER_BUILDKIT=1
docker build --build-arg VERSION=$VER -o $DEST -f Dockerfile.linux .

?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ahh, was this why you were producing empty jars? Or is this a separate issue?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Set docker buildkit flag in edf248c

Copy link
Member

Choose a reason for hiding this comment

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

@car-roll car-roll requested a review from jglick May 12, 2021 15:45
VER=$1
# output directory of binaries
DEST=$2
docker build --build-arg VERSION=$VER -o $DEST -f Dockerfile.linux .
Copy link
Member

Choose a reason for hiding this comment

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

@car-roll car-roll merged commit a502943 into jenkinsci:main May 14, 2021
@car-roll car-roll deleted the remove-docker-maven-plugin branch May 14, 2021 20:47
@car-roll car-roll added the enhancement New feature or request label May 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants