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.

Sign up for GitHub

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

Jump to bottom

Update common files. #16989

Merged
merged 1 commit into from
Sep 11, 2019
Merged

Update common files. #16989

merged 1 commit into from
Sep 11, 2019

Conversation

geeknoid
Copy link
Contributor

No description provided.

@geeknoid geeknoid requested review from howardjohn, linsun, rshriram and a team as code owners September 10, 2019 23:53
@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 Sep 10, 2019
@istio-testing istio-testing added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Sep 10, 2019
howardjohn
howardjohn previously requested changes Sep 11, 2019
View reviewed changes
Copy link
Member

@howardjohn howardjohn left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Why is this adding a bunch of binary proto_descriptor stuff? and changing random mixer stuff like statsd.yaml

is this because you reran go:generate?

howardjohn
howardjohn reviewed Sep 11, 2019
View reviewed changes
Copy link
Member

@howardjohn howardjohn left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I don't think we can merge without a fix for .PHONY. There are tons of targets that fail now

Getting makefile completion to work would be ideal as well, if possible. This repo has a lot of targets so it helps new developers (and experienced ones) navigate them all

@geeknoid
Copy link
Contributor Author

@howardjohn Yes, I ran "go generate ./...". This resulted in slightly different proto descriptors due to different import paths. This in turn results in different yaml, since the yamls encoded the deacriptors as base 64.

@geeknoid
Copy link
Contributor Author

The non-generic solution for .PHONY is to add the relevant definitions to Makefile.overrides.mk. That makefile is evaluated along with the top-level Makefile before launching the container. I think this can be done as a follow-on.

Which are the significant targets which wouldn't work with this change? It's limited by the set of top-level files/dirs at the root of istio/istio, right?

@howardjohn
Copy link
Member

Which are the significant targets which wouldn't work with this change? It's limited by the set of top-level files/dirs at the root of istio/istio, right?

I think so.. maybe we can just dynamically set phony based on all too level files. Or just do it statically with the current set of files

@geeknoid
Copy link
Contributor Author

@howardjohn OK, I added a bunch of phony targets to Makefile.overrides.mk.

@geeknoid
Copy link
Contributor Author

PTAL

@howardjohn
Copy link
Member 

$ make istioctl
make: Nothing to be done for 'istioctl'.

Seems like the phony in override doesn't work?

@geeknoid geeknoid requested a review from howardjohn September 11, 2019 16:37
@geeknoid
Copy link
Contributor Author

geeknoid commented Sep 11, 2019 via email

@geeknoid geeknoid requested a review from a team as a code owner September 11, 2019 16:51
@geeknoid
Copy link
Contributor Author

geeknoid commented Sep 11, 2019 via email

@howardjohn
Copy link
Member

howardjohn commented Sep 11, 2019 •
edited
Loading

Wow I don't get what makefile is doing...

I tried to replace what you did with

PHONYS := $(shell ls)
.PHONY: $(PHONYS)
$(PHONYS):
	@${MAKE} $@

and i get the

make[1]: Nothing to be done for 'Makefile.overrides.mk'.
make[1]: Nothing to be done for 'Makefile'.

stuff again

If I do


PHONYS := $(shell ls)
.PHONY: $(PHONYS)
$(PHONYS):
	echo make -f Makefile.core.mk $@

I get

$ make istioctl
echo make -f Makefile.core.mk Makefile.overrides.mk
make -f Makefile.core.mk Makefile.overrides.mk
echo make -f Makefile.core.mk Makefile
make -f Makefile.core.mk Makefile
echo make -f Makefile.core.mk istioctl
make -f Makefile.core.mk istioctl

So it seems somehow make is doing some weird magic, maybe because it matches multiple wildcards?

@howardjohn
Copy link
Member

Interesting, I think the problem is that we get a target for Makefile.override.mk. So if we replace with

PHONYS := $(shell ls | grep -v Makefile)
.PHONY: $(PHONYS)
$(PHONYS):
	@$(MAKE) $@

It works?

Not sure if we need even more crazy makefile stuff though

@sdake
Copy link
Member

sdake commented Sep 11, 2019

Interesting, I think the problem is that we get a target for Makefile.override.mk. So if we replace with

PHONYS := $(shell ls | grep -v Makefile)
.PHONY: $(PHONYS)
$(PHONYS):
	@$(MAKE) $@

It works?

Not sure if we need even more crazy makefile stuff though

this is actually a pretty cool solution - with appropriate comments.

@geeknoid
Copy link
Contributor Author

@howardjohn Is this hack for Makefile or Makefile.overrides.mk?

@howardjohn
Copy link
Member

howardjohn commented Sep 11, 2019 via email

@geeknoid
Copy link
Contributor Author

OK, I've merge John's code in Makefile.overrides.mk. If we find we need this kind of cleverness in other repos, we can move it to Makefile in the common repo instead. But for now, let's keep it local to Makefile.overrides.mk in case it starts to show some problems...

@istio-testing
Copy link
Collaborator

@geeknoid: The following test failed, say /retest to rerun them all:

Test name Commit Details Rerun command
pilot-multicluster-e2e_istio ce021bf link /test pilot-multicluster-e2e_istio

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@geeknoid geeknoid merged commit 03387cf into istio:master Sep 11, 2019
@shamsher31 shamsher31 mentioned this pull request Aug 21, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@howardjohn howardjohn

@linsun linsun

@rshriram rshriram

Assignees

@sdake sdake

Labels
cla: yes Set by the Google CLA bot to indicate the author of a PR has signed the Google CLA. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@geeknoid @howardjohn @sdake @istio-testing @googlebot