Add docker run script by howardjohn · Pull Request #172 · istio/common-files

howardjohn · 2019-12-23T17:27:39Z

Take over https://github.com/istio/common-files/pull/94/files

This makes it possible to run interactive commands inside the build container

resolves istio/istio#17476 - all commands arguments to run-docker.sh are passed
into the container

resolves istio/istio#17474 - the script will pass along a filtered picture of the host's environment to the container

howardjohn · 2019-12-23T18:24:24Z

/hold need some more work

howardjohn · 2019-12-23T18:34:06Z

Ok I split out the enviroment variables, should be ok now

ericvn

Will need a rebase.

sdake

looks like a good restructure of the existing Makefile (that has grown too unwieldy). Still, I can't help but think this will introduce unwanted side-effects and compound the difficulty of moving istio/istio to the container plan. Can you hold off on merging this work until istio/istio is on the container plan?

Cheers
-steve

/hold

sdake · 2020-01-26T12:04:06Z

files/Makefile

+# Set up environment variables
+$(shell mkdir -p out)
+$(shell $(PWD)/common/scripts/setup_env.sh envfile > out/.env)
+include out/.env


I don't understand this include. Why not simply pass the variables in this file? I am not keen on using filesystem contents in the top level Makefile.

We need a way to have the environment setting consistent across the makefile and the shell script. This was the only way I could come up with this, but if there are better approaches I am all ears, I didn't feel great about this

I don't have any immediate ideas, but I am beat after 5 hours of popcorn ceiling removal. Wow what a PITA. We can always adjust this in the future.

sdake · 2020-01-26T12:04:33Z

files/Makefile

-ENV_VARS+=-e TAG="$(TAG)"
-endif
+RUN = ./common/scripts/run.sh
+IMG ?= gcr.io/istio-testing/build-tools:master-2019-12-15T16-17-48


IMG version needs updating.

The whole PR is out of date a bit, I'll rebase once we are ready to merge

I am not sure why I have this IMG, it should just be defined in the script. Ill see if I can remove this one

sdake · 2020-01-26T12:06:17Z

files/Makefile

-TIMEZONE=`readlink $(READLINK_FLAGS) /etc/localtime | sed -e 's/^.*zoneinfo\///'`
+# Override variables with container specific
+export TARGET_OUT=$(CONTAINER_TARGET_OUT)
+export TARGET_OUT_LINUX=$(CONTAINER_TARGET_OUT_LINUX)


The introduction of CONTAINER_* variables seems confusing to me?

The intent here was that we are defining thins like TARGET_OUT conditionally in the Makefile so we need to have both defined in the script -- no one other than Makefile uses the container_out though.

However, we can probably do the BUILD_WITH_CONTAINER check inside the script an remove the need for this

sdake · 2020-01-26T12:07:21Z

files/common/scripts/run.sh

+    -v /etc/passwd:/etc/passwd:ro \
+    -v /etc/group:/etc/group:ro \
+    $CONTAINER_OPTIONS \
+    --env-file <(env | grep -v ${ENV_BLACKLIST}) \


nice simple approach here).

sdake · 2020-01-26T12:07:47Z

files/common/scripts/run.sh

+    --mount "type=bind,source=${PWD},destination=/work" \
+    --mount "type=volume,source=go,destination=/go" \
+    --mount "type=volume,source=gocache,destination=/gocache" \
+    ${CONDITIONAL_HOST_MOUNTS} \


default CONDITIOANL_HOST_MOUNTS missing.

Its in the environment setup script

sdake · 2020-01-26T12:08:38Z

files/common/scripts/setup_env.sh

+    exit 1
+fi
+
+export UID


where is UID set? Not in the environment:

sdake@beast-01:~/go/src/istio.io/istio$ env | grep UID

Oh maybe this isn't right then. UID is a builtin I guess

$ env | grep UID $ echo $UID 576696

I'd verify the export works as expected. I would think it should as a built-in but best to be sure.

sdake · 2020-01-26T12:09:12Z

files/common/scripts/setup_env.sh

+export CONTAINER_TARGET_OUT="${CONTAINER_TARGET_OUT:-/work/out/${TARGET_OS}_${TARGET_ARCH}}"
+export CONTAINER_TARGET_OUT_LINUX="${CONTAINER_TARGET_OUT_LINUX:-/work/out/linux_amd64}"
+
+export IMG="${IMG:-gcr.io/istio-testing/build-tools:master-2019-12-15T16-17-48}"


would be nice if we had one IMG variable to modify rather than two.

ack, I think the other one can be removed

sdake · 2020-01-26T12:09:32Z

files/common/scripts/setup_env.sh

+
+export CONTAINER_CLI="${CONTAINER_CLI:-docker}"
+
+export ENV_BLACKLIST="${ENV_BLACKLIST:-^_\|PATH\|SHELL\|EDITOR\|TMUX\|USER\|HOME\|PWD\|TERM\|GO\|rvm\|SSH}"


call this ENV_REJECTLIST please.

howardjohn

ill make the changes mentioned once we are on container plan for istio. This PR is a pain to rebase so I don't intend to keep it up to date until we are ready to merge

howardjohn · 2020-01-26T17:03:12Z

files/Makefile

+# Set up environment variables
+$(shell mkdir -p out)
+$(shell $(PWD)/common/scripts/setup_env.sh envfile > out/.env)
+include out/.env


We need a way to have the environment setting consistent across the makefile and the shell script. This was the only way I could come up with this, but if there are better approaches I am all ears, I didn't feel great about this

howardjohn · 2020-01-26T17:03:45Z

files/Makefile

-ENV_VARS+=-e TAG="$(TAG)"
-endif
+RUN = ./common/scripts/run.sh
+IMG ?= gcr.io/istio-testing/build-tools:master-2019-12-15T16-17-48


The whole PR is out of date a bit, I'll rebase once we are ready to merge

howardjohn · 2020-01-26T17:04:18Z

files/common/scripts/run.sh

+    --mount "type=bind,source=${PWD},destination=/work" \
+    --mount "type=volume,source=go,destination=/go" \
+    --mount "type=volume,source=gocache,destination=/gocache" \
+    ${CONDITIONAL_HOST_MOUNTS} \


Its in the environment setup script

howardjohn · 2020-01-26T17:10:12Z

files/common/scripts/setup_env.sh

+    exit 1
+fi
+
+export UID


Oh maybe this isn't right then. UID is a builtin I guess

$ env | grep UID $ echo $UID 576696

howardjohn · 2020-01-26T17:10:51Z

files/Makefile

-ENV_VARS+=-e TAG="$(TAG)"
-endif
+RUN = ./common/scripts/run.sh
+IMG ?= gcr.io/istio-testing/build-tools:master-2019-12-15T16-17-48


I am not sure why I have this IMG, it should just be defined in the script. Ill see if I can remove this one

howardjohn · 2020-01-26T17:11:03Z

files/common/scripts/setup_env.sh

+export CONTAINER_TARGET_OUT="${CONTAINER_TARGET_OUT:-/work/out/${TARGET_OS}_${TARGET_ARCH}}"
+export CONTAINER_TARGET_OUT_LINUX="${CONTAINER_TARGET_OUT_LINUX:-/work/out/linux_amd64}"
+
+export IMG="${IMG:-gcr.io/istio-testing/build-tools:master-2019-12-15T16-17-48}"


ack, I think the other one can be removed

howardjohn · 2020-01-26T17:13:27Z

files/Makefile

-TIMEZONE=`readlink $(READLINK_FLAGS) /etc/localtime | sed -e 's/^.*zoneinfo\///'`
+# Override variables with container specific
+export TARGET_OUT=$(CONTAINER_TARGET_OUT)
+export TARGET_OUT_LINUX=$(CONTAINER_TARGET_OUT_LINUX)


The intent here was that we are defining thins like TARGET_OUT conditionally in the Makefile so we need to have both defined in the script -- no one other than Makefile uses the container_out though.

However, we can probably do the BUILD_WITH_CONTAINER check inside the script an remove the need for this

sdake · 2020-02-03T06:20:43Z

Understood re difficulty of rebasing - especially against a moving target. I think BUILD_WITH_CONTAINER=1 PR is ready to merge. Once that is done, I'd suggest moving on this fast before more changes come to the TLD makefile in the common-files repo 😁

sdake · 2020-03-03T07:19:32Z

@howardjohn what do you want to do with this PR post 1.5 release? I think the make system is in sold enough shape that a script based approach can be used. Best to do that at the beginning of the dev cycle (after 1.5 work is done of course).

Your thoughts welcome as I am listed as a reviewer.

howardjohn · 2020-03-03T16:17:47Z

I would like to merge this in a similar form as it is today but have not yet found the time to do so

…

On Mon, Mar 2, 2020 at 11:19 PM Steven Dake ***@***.***> wrote: @howardjohn <https://github.com/howardjohn> what do you want to do with this PR post 1.5 release? I think the make system is in sold enough shape that a script based approach can be used. Best to do that at the beginning of the dev cycle (after 1.5 work is done of course). Your thoughts welcome as I am listed as a reviewer. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#172?email_source=notifications&email_token=AAEYGXLROMBXASNTXW4LWZ3RFSVQJA5CNFSM4J6WNEG2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOENSL2ZA#issuecomment-593804644>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAEYGXOHSHUP2HG2BZOUSJTRFSVQJANCNFSM4J6WNEGQ> .

howardjohn · 2020-03-03T16:17:58Z

If anyone else is interested feel free to take this over

…

On Tue, Mar 3, 2020 at 8:17 AM John Howard ***@***.***> wrote: I would like to merge this in a similar form as it is today but have not yet found the time to do so On Mon, Mar 2, 2020 at 11:19 PM Steven Dake ***@***.***> wrote: > @howardjohn <https://github.com/howardjohn> what do you want to do with > this PR post 1.5 release? I think the make system is in sold enough shape > that a script based approach can be used. Best to do that at the beginning > of the dev cycle (after 1.5 work is done of course). > > Your thoughts welcome as I am listed as a reviewer. > > — > You are receiving this because you were mentioned. > Reply to this email directly, view it on GitHub > <#172?email_source=notifications&email_token=AAEYGXLROMBXASNTXW4LWZ3RFSVQJA5CNFSM4J6WNEG2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOENSL2ZA#issuecomment-593804644>, > or unsubscribe > <https://github.com/notifications/unsubscribe-auth/AAEYGXOHSHUP2HG2BZOUSJTRFSVQJANCNFSM4J6WNEGQ> > . >

This allows running the same set of logic for the script runs and makefile runs. It also allows non makefile users to source this and continue to run `go test ./...` or whatever they want, without duplicated logic.

howardjohn · 2020-03-07T00:39:50Z

I did not mean to merge this, I didn't realize it was approved. However, I do think it should be merge. If anyone disagrees we can revert or roll forward?

I broke this in istio#172 because I forgot about the BUILD_WITH_CONTAINER=0 case... This should get things moving again. I have tested this in istio/istio#21951

clarketm · 2020-03-07T01:58:30Z

I did not mean to merge this, I didn't realize it was approved. However, I do think it should be merge. If anyone disagrees we can revert or roll forward?

@howardjohn

Did the API change? Or there is probably an assumption that this is to run on Linux host? but FWIW make commands no longer work for me locally on MacOS after this merged (even when running in container).

$ BUILD_WITH_CONTAINER=1 make lint

/bin/sh: 1: set: Illegal option -o pipefail
common/Makefile.common.mk:33: recipe for target 'lint-yaml' failed
make[1]: *** [lint-yaml] Error 2
make: *** [Makefile:44: lint] Error 2

$ BUILD_WITH_CONTAINER=1 make update-common

mkdir: cannot create directory '/var/folders': Permission denied
common/Makefile.common.mk:98: recipe for target 'update-common' failed
make[1]: *** [update-common] Error 1
make: *** [Makefile:44: update-common] Error 2

$ BUILD_WITH_CONTAINER=1 make test

go: creating work dir: stat /var/folders/9j/nh3fxdf153v11vsjwtd1lf_800n1b6/T/: no such file or directory
Makefile.core.mk:23: recipe for target 'test' failed
make[1]: *** [test] Error 1
make: *** [Makefile:44: test] Error 2

This reverts commit dfe763e.

* Add docker run script * Break out environment setup into a common script This allows running the same set of logic for the script runs and makefile runs. It also allows non makefile users to source this and continue to run `go test ./...` or whatever they want, without duplicated logic. * Fix lint * clean up (cherry picked from commit dfe763e)

* Add docker run script (#172) * Add docker run script * Break out environment setup into a common script This allows running the same set of logic for the script runs and makefile runs. It also allows non makefile users to source this and continue to run `go test ./...` or whatever they want, without duplicated logic. * Fix lint * clean up (cherry picked from commit dfe763e) * done (cherry picked from commit 0b5b799084d5654b1260fbb49a1f4c4c45389434) * Add TMPDIR to blocklist For OS X

howardjohn requested a review from a team as a code owner December 23, 2019 17:27

googlebot added the cla: yes Set by the Google CLA bot to indicate the author of a PR has signed the Google CLA. label Dec 23, 2019

istio-testing added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Dec 23, 2019

howardjohn assigned sdake, ericvn and geeknoid Dec 23, 2019

howardjohn mentioned this pull request Dec 23, 2019

extract BUILD_WITH_CONTAINER docker command into a script #94

Closed

howardjohn mentioned this pull request Jan 19, 2020

Fixed the Istio docker build failure #173

Closed

ericvn approved these changes Jan 21, 2020

View reviewed changes

sdake approved these changes Jan 26, 2020

View reviewed changes

sdake added the do-not-merge Block automatic merging of a PR. label Jan 26, 2020

howardjohn commented Jan 26, 2020

View reviewed changes

sdake mentioned this pull request Feb 3, 2020

istio:istio make clean <some other target no longer works locally (MacOS only?) istio/istio#20731

Closed

istio-testing added the needs-rebase Indicates a PR needs to be rebased before being merged label Feb 12, 2020

howardjohn added 3 commits March 6, 2020 15:37

Add docker run script

d70a117

Break out environment setup into a common script

4712ef0

This allows running the same set of logic for the script runs and makefile runs. It also allows non makefile users to source this and continue to run `go test ./...` or whatever they want, without duplicated logic.

Fix lint

3f0b8b8

howardjohn force-pushed the run-script branch from 3367cb3 to 3f0b8b8 Compare March 6, 2020 23:45

istio-testing removed the needs-rebase Indicates a PR needs to be rebased before being merged label Mar 6, 2020

clean up

b58d866

howardjohn removed the do-not-merge Block automatic merging of a PR. label Mar 7, 2020

istio-testing merged commit dfe763e into istio:master Mar 7, 2020

howardjohn mentioned this pull request Mar 7, 2020

Fix build when using non container build #203

Closed

clarketm added a commit that referenced this pull request Mar 7, 2020

Revert "Add docker run script (#172)"

770f0a9

This reverts commit dfe763e.

clarketm mentioned this pull request Mar 7, 2020

Revert "Add docker run script" #204

Merged

istio-testing pushed a commit that referenced this pull request Mar 7, 2020

Revert "Add docker run script (#172)" (#204)

9827381

This reverts commit dfe763e.

fjglira pushed a commit to fjglira/istio-common-files that referenced this pull request Oct 22, 2025

Trigger a 1.6.0-alpha.0 build (istio#172)

2f0008a


		export CONTAINER_CLI="${CONTAINER_CLI:-docker}"

		export ENV_BLACKLIST="${ENV_BLACKLIST:-^_\\|PATH\\|SHELL\\|EDITOR\\|TMUX\\|USER\\|HOME\\|PWD\\|TERM\\|GO\\|rvm\\|SSH}"

Conversation

howardjohn commented Dec 23, 2019

Uh oh!

howardjohn commented Dec 23, 2019

Uh oh!

howardjohn commented Dec 23, 2019

Uh oh!

ericvn left a comment

Choose a reason for hiding this comment

Uh oh!

sdake left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

howardjohn left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sdake commented Feb 3, 2020

Uh oh!

sdake commented Mar 3, 2020

Uh oh!

howardjohn commented Mar 3, 2020 via email

Uh oh!

howardjohn commented Mar 3, 2020 via email

Uh oh!

howardjohn commented Mar 7, 2020

Uh oh!

clarketm commented Mar 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

clarketm commented Mar 7, 2020 •

edited

Loading