-
Notifications
You must be signed in to change notification settings - Fork 8.1k
add missing rule in Makefile that's breaking builds #3860
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't fully understand this fix, but based on your comments, I believe that this should help.
/lgtm
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: douglas-reid The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these OWNERS Files:
You can indicate your approval by writing |
Codecov Report
@@ Coverage Diff @@
## master #3860 +/- ##
=======================================
- Coverage 77% 76% -<1%
=======================================
Files 295 295
Lines 26463 26863 +400
=======================================
+ Hits 20227 20403 +176
- Misses 4989 5159 +170
- Partials 1247 1301 +54
Continue to review full report at Codecov.
|
/test all [submit-queue is verifying that this PR is safe to merge] |
To be more explicit about the issue, in the Makefile each of the main go executables has two targets. The first target is in the form of the full path to where the executable is created (e.g., "$(ISTIO_OUT)/istioctl") while the second is basically an "phony" alias (e.g., "istioctl") that allows devs to conveniently build a particular executable (e.g., "make istioctl"). The docker and debian targets have dependencies on the full-path target (this helps avoid unnecessary recompiles during rebuilds since make always wants to rebuild "phony" targets), and the one for istioctl was dropped in the earlier PR (hence make's complaint that it doesn't know how to build istioctl). For the debian image the dependency is set up in tools/deb/istio.mk using the list in ISTIO_DEB_DEPS (and the foreach loop that comes a few lines later) to declare a dependency on each of the go binaries listed in ISTIO_DEB_DEPS (using the full-path target) and also set up a SIDECAR_FILES variable that's used to tell fpm to add each of these listed go binaries to the debian image. This change adds back the missing target for $(ISTIO_OUT)/istioctl. You might be used to seeing something more of the form: $(ISTIO_OUT)/istioctl: dependencies recipe istioctl: $(ISTIO_OUT)/istioctl though a different form that has both targets borrow the same recipe is: istioctl $(ISTIO_OUT)/istioctl: dependencies recipe |
My attempt to fix #3858
I made two attempts to avoid the breakage prior to submit of #3820:
#3820 (comment)
#3820 (comment)
In this change I also remove some space that emacs warns about being suspect (but is probably okay since it's continued from the prior line).
I confirmed that with this change an attempt at "make deb" gets to the point of trying to run fpm (which fails on my system since I don't have it installed, but the point is is compiled istio and got to this step okay).