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

Bring back 'make local' #711

Merged
merged 1 commit into from
Aug 9, 2018
Merged

Bring back 'make local' #711

merged 1 commit into from
Aug 9, 2018

Conversation

skriss
Copy link
Contributor

@skriss skriss commented Jul 27, 2018

Signed-off-by: Andy Goldstein andy.goldstein@gmail.com

I pulled this out of #495, creating a PR so we don't lose track of it.

I'd like to see the goarch detection move to only happen in make local, bc this is going to break make container when running on macOS.

@ncdc
Copy link
Contributor

ncdc commented Jul 27, 2018 via email

@nrb
Copy link
Contributor

nrb commented Jul 27, 2018

LGTM.

May want to update https://github.com/heptio/ark/blob/master/docs/build-from-scratch.md#cross-compiling to reference this target.

@skriss
Copy link
Contributor Author

skriss commented Jul 27, 2018

not ready to merge

@skriss skriss changed the title [WIP] Bring back 'make local' Bring back 'make local' Aug 8, 2018
@skriss
Copy link
Contributor Author

skriss commented Aug 8, 2018

@nrb which part are you saying should be updated in the cross-compiling section?

@skriss skriss requested review from ncdc, nrb and carlisia August 8, 2018 21:14
@skriss
Copy link
Contributor Author

skriss commented Aug 8, 2018

@ncdc PTAL if you get a chance, I think this addresses the comment re: only detecting local arch when running make local (just learned about target-specific variables)

@@ -79,6 +82,15 @@ all-build: $(addprefix build-, $(CLI_PLATFORMS))

#all-push: $(addprefix push-, $(CONTAINER_PLATFORMS))

local: build-dirs
GOOS=$(GOOS) \
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can omit specifying all the variables that are unmodified and make should pass them through when executing build.sh. That would leave OUTPUT_DIR as the only one we need. Try that and see if it works?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nope :(

Copy link
Contributor

Choose a reason for hiding this comment

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

Aha, we could export whatever variables we want to be passed down to all shell invocations from all targets, if desired.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Eh, maybe in a separate PR :)

@nrb
Copy link
Contributor

nrb commented Aug 8, 2018

@skriss I was thinking this text: By default, make builds an ark binary that runs on your host operating system and architecture.

My Makefile knowledge isn't great, but isn't the point of this PR that the default target all doesn't actually do that? local will do the detection, and all will build linux-amd64 all the time.

Am I getting that right?

@skriss
Copy link
Contributor Author

skriss commented Aug 8, 2018

Ah, I see what you mean. Yeah, that sentence isn't really true in the current state. Let me see if I can do some rewording there.

@skriss
Copy link
Contributor Author

skriss commented Aug 8, 2018

OK after re-reading that section + the Makefile, I made an edit to reflect existing reality better, but I'm not mentioning make local there because I think make build is the appropriate target to use.

Signed-off-by: Andy Goldstein <andy.goldstein@gmail.com>
Signed-off-by: Steve Kriss <steve@heptio.com>
@skriss
Copy link
Contributor Author

skriss commented Aug 9, 2018

@carlisia PTAL, merge if +1.

Copy link
Contributor

@carlisia carlisia left a comment

Choose a reason for hiding this comment

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

👍

@carlisia carlisia merged commit 23d570e into vmware-tanzu:master Aug 9, 2018
@skriss skriss deleted the make-local branch August 9, 2018 19:28
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.

4 participants