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

Remove img.build make target #362

Merged
merged 1 commit into from
Mar 5, 2024
Merged

Remove img.build make target #362

merged 1 commit into from
Mar 5, 2024

Conversation

bobh66
Copy link
Contributor

@bobh66 bobh66 commented Mar 4, 2024

Description of your changes

Removed the img.build make target which has no purpose in this project and is blocking the fallthrough default make target.

Fixes #361

I have:

  • Read and followed Upjet's contribution process.
  • Run make reviewable to ensure this PR is ready for review.
  • Added backport release-x.y labels to auto-backport this PR if necessary.

How has this code been tested

Running make on a clean repository now clones the build module and runs the default make targets as expected.

Copy link
Collaborator

@ulucinar ulucinar left a comment

Choose a reason for hiding this comment

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

Hi @bobh66,
Thanks for tackling with this issue. In my setup, removing the empty img.build target broke the default target as explained in the comment I left. Could you please check whether this is the case?

@@ -27,7 +27,6 @@ PLATFORMS ?= linux_amd64 linux_arm64
# 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

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.

Signed-off-by: Bob Haddleton <[email protected]>
Copy link
Collaborator

@ulucinar ulucinar left a comment

Choose a reason for hiding this comment

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

Thanks @bobh66, lgtm.

@ulucinar ulucinar merged commit e2a2297 into crossplane:main Mar 5, 2024
7 checks passed
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.

Running make on a clean repo doesn't work properly
2 participants