Skip to content

Fix failure when running BUILD_WITH_CONTAINER=1 make istioctl.completion#20339

Merged
istio-testing merged 1 commit intoistio:masterfrom
ericvn:fixCompletion
Jan 21, 2020
Merged

Fix failure when running BUILD_WITH_CONTAINER=1 make istioctl.completion#20339
istio-testing merged 1 commit intoistio:masterfrom
ericvn:fixCompletion

Conversation

@ericvn
Copy link
Copy Markdown
Contributor

@ericvn ericvn commented Jan 20, 2020 •
edited by istio-policy-bot
Loading

Fixes #20338

@istio-testing istio-testing added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Jan 20, 2020
@googlebot googlebot added the cla: yes Set by the Google CLA bot to indicate the author of a PR has signed the Google CLA. label Jan 20, 2020
@ericvn
Copy link
Copy Markdown
Contributor Author

ericvn commented Jan 20, 2020 •
edited
Loading

rm -rf out (does a better make clean since the later doesn't remove the linux related directory)
BUILD_WITH_CONTAINER=1 make istioctl.completion
yields a successful run.

...
/work/out/linux_amd64/istioctl collateral --bash && \
mv istioctl.bash /work/out/darwin_amd64/release/istioctl.bash
/work/out/linux_amd64/istioctl collateral --zsh && \
mv _istioctl /work/out/darwin_amd64/release/_istioctl

A second BUILD_WITH_CONTAINER=1 make istioctl.completion without the clean is also successful.

howardjohn
howardjohn reviewed Jan 20, 2020
View reviewed changes
Makefile.core.mk
export ISTIO_OUT:=$(TARGET_OUT)
export ISTIO_OUT_LINUX:=$(TARGET_OUT_LINUX)

# LOCAL_OUT should include architecture where we are currently running versus the desired.
Copy link
Copy Markdown
Member

@howardjohn howardjohn Jan 20, 2020

Choose a reason for hiding this comment

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

I am a bit confused when LOCAL_OUT should be used and when ISTIO_OUT should be?

Copy link
Copy Markdown
Member

@howardjohn howardjohn Jan 20, 2020

Choose a reason for hiding this comment

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

Oh is local-out is for running things, we never explicitly output things there?

Copy link
Copy Markdown
Contributor Author

@ericvn ericvn Jan 21, 2020

Choose a reason for hiding this comment

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

Correct. LOCAL_OUT is where you want run built artifacts from (ie, those artifacts should be of the OS/ARCH of the container if being run in the container or the local environment if not running in the container). I would have preferred to use HOST for specifying where we are running, LOCAL for the container, but we already were using LOCAL (copying from TARGET) so hard to come up with a good name.

@ericvn
Copy link
Copy Markdown
Contributor Author

ericvn commented Jan 21, 2020

/test unit-tests_istio

@istio-testing istio-testing merged commit a0fce32 into istio:master Jan 21, 2020
@ericvn ericvn deleted the fixCompletion branch January 21, 2020 02:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@howardjohn howardjohn howardjohn approved these changes

@costinm costinm Awaiting requested review from costinm

@linsun linsun Awaiting requested review from linsun

@rshriram rshriram Awaiting requested review from rshriram

Assignees

No one assigned

Labels

area/test and release cla: yes Set by the Google CLA bot to indicate the author of a PR has signed the Google CLA. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

istio/istio: BUILD_WITH_CONTAINER=1 make istioctl.completion fails on MacOS

5 participants

@ericvn @howardjohn @googlebot @istio-testing @istio-policy-bot