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

Remove img.build make target #362

Merged
merged 1 commit into from
Mar 5, 2024
Merged

Remove img.build make target #362

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 0 additions & 8 deletions Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,14 +21,6 @@ PLATFORMS ?= linux_amd64 linux_arm64
# to run a target until the include commands succeeded.
-include build/makelib/common.mk

# ====================================================================================
# Setup Images

# even though this repo doesn't build images (note the no-op img.build target below),
# some of the init is needed for the cross build container, e.g. setting BUILD_REGISTRY
-include build/makelib/image.mk
img.build:
Copy link
Collaborator

Choose a reason for hiding this comment

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

When I gave the following a try:

  • Clone the repo
  • Remove the img.build target from the Makefile
  • Run make
    I get the following error:
❯ make
Submodule 'build' (https://github.com/upbound/build) registered for path 'build'
Cloning into '/private/tmp/upjet/build'...
Submodule path 'build': checked out 'bd5297bd16c113cbc5ed1905b1d96aa1cb3078ec'
Initial setup complete. Running make again . . .
11:02:25 [ .. ] verify go modules dependencies have expected content
all modules verified
11:02:27 [ OK ] go modules dependencies verified
11:02:28 [ .. ] go build linux_arm64
11:02:28 [ OK ] go build linux_arm64
make[4]: *** No rule to make target `img.build', needed by `build.artifacts.platform'.  Stop.
make[3]: *** [do.build.artifacts.linux_arm64] Error 2
make[2]: *** [build.all] Error 2
make[1]: *** [build] Error 2
make: *** [fallthrough] Error 2

My understanding is that this empty target was added to prevent this error.

Copy link
Collaborator

@ulucinar ulucinar Mar 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Looks like removing the include together with the empty target makes the default target behave as we would like it to:

diff --git a/Makefile b/Makefile
index cda2468..d1e3ffa 100644
--- a/Makefile
+++ b/Makefile
@@ -21,14 +21,6 @@ PLATFORMS ?= linux_amd64 linux_arm64
 # to run a target until the include commands succeeded.
 -include build/makelib/common.mk

-# ====================================================================================
-# Setup Images
-
-# even though this repo doesn't build images (note the no-op img.build target below),
-# some of the init is needed for the cross build container, e.g. setting BUILD_REGISTRY
--include build/makelib/image.mk
-img.build:
-
 # ====================================================================================
 # Setup Go

I will also leave a comment in the parent issue. I think we can do this as we don't need this target and this is not a template repository like the https://github.com/crossplane/upjet-provider-template.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Just left a comment discussing the root cause here. Moving the empty img.build target after to the fallthrough target also addresses the issue:

diff --git a/Makefile b/Makefile
index cda2468..f3732fa 100644
--- a/Makefile
+++ b/Makefile
@@ -21,14 +21,6 @@ PLATFORMS ?= linux_amd64 linux_arm64
 # to run a target until the include commands succeeded.
 -include build/makelib/common.mk
 
-# ====================================================================================
-# Setup Images
-
-# even though this repo doesn't build images (note the no-op img.build target below),
-# some of the init is needed for the cross build container, e.g. setting BUILD_REGISTRY
--include build/makelib/image.mk
-img.build:
-
 # ====================================================================================
 # Setup Go
 
@@ -60,6 +52,14 @@ fallthrough: submodules
        @echo Initial setup complete. Running make again . . .
        @make
 
+# ====================================================================================
+# Setup Images
+
+# even though this repo doesn't build images (note the no-op img.build target below),
+# some of the init is needed for the cross build container, e.g. setting BUILD_REGISTRY
+-include build/makelib/image.mk
+img.build:
+
 # Update the submodules, such as the common build scripts.
 submodules:
        @git submodule sync

but because we don't need the build/makelib/image.mk targets, we can safely remove the include as suggested above.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @ulucinar - interesting that it works when the change is applied after already running make in the repo but it doesn't work on a clean repo. I'll remove the include.


# ====================================================================================
# Setup Go

Expand Down
Loading