Rework the Makefile so its not racy #105

mmlb · 2022-02-19T02:44:29Z

Description

Reworks/overhauls the Makefile setup so that we have proper deps and targets, while minimizing repetition.
Also merges ci.yaml and push.yaml into just one file/workflow

Why is this needed

I tried building hook locally and was confused as to how to do it and what it was going to do. Once I opened the Makefile I could not let it be.

I merged ci.yaml and push.yaml because push.yaml was potentially overwriting the 0.0 tags for the container images in quay.io if a push to a non-main branch occurred. This would cause anyone building hook locally to unintended/possibly buggy code.

How Has This Been Tested?

I've run so many make invocations its not even funny. I've built the arm and amd targets for image and the containers. I've run make dist and have the expected tarball. I've also run make run and have booted a qemu vm with the built files. Everything good so far. Also ran through github actions a bunch.

How are existing users impacted? What migration steps/scripts do we need?

Able to build hook locally, more easily/reliably. Won't get unexpected containers when building hook.

Checklist:

I have:

updated the documentation and/or roadmap (if required)
added unit or e2e tests
provided instructions on how to upgrade

mmlb · 2022-02-24T17:46:42Z

@thebsdbox PTAL, this should be ready for merging.

nshalman

This all looks like movement in the right direction, thank you for putting in the work!

Something isn't quite right if a random person clones the repo and tries to do builds.
We should get that cleaned up and perhaps improve the README a bit more.

nshalman · 2022-03-11T17:27:18Z

Makefile

-	rm ./hook-${GIT_VERSION}.tar.gz
-	rm -rf dist/ out/ tink-docker/local/ bootkit/local/
+help: ## Print this help
+	@grep --no-filename -E '^[[:alnum:]_-]+:.*?## .*$$' $(MAKEFILE_LIST) | sed 's/:.*## /·/' | sort | column -t -W 2 -s '·' -c $$(tput cols)


Everywhere I tried to use this really didn't like that special character. Replacing it with .e.g. | fixed things for me.

Hmm we use this in all the other Makefiles too (in boots, tink, tinkerbell-docs). I've been copy/pasting this from when I first got this snipped and I think the idea is to have a character that is not likely to be present in the comment string. I'm curious if you have a non UTF8 locale set?

nshalman · 2022-03-11T17:47:03Z

hack/ci-build.sh

@@ -1,4 +1,4 @@
 #!/usr/bin/env nix-shell
 #!nix-shell -i bash ../shell.nix

-make dist-existing-images
+make ORG=tinkerbell/hook dist-existing-images


When I run this, I get make: *** No rule to make target 'dist-existing-images'. Stop.

This file is actually meant to be deleted, seems I forgot to do so. Actually both files in ./hack should be deleted. I'll update the PR with the removal and update to README.md

nshalman · 2022-03-11T17:49:34Z

shell.nix

+  rev = "ce7b327a52d1b82f82ae061754545b1c54b06c66";
+  sha256 = "1rc4if8nmy9lrig0ddihdwpzg2s8y36vf20hfywb8hph5hpsg4vj";


If we use something newer here maybe we can remove the linuxkit override?

yeah I wanted to give that a go in a different PR.

Well I ended up doing it anyway because of other reasons, but linuxkit is still at v0.8 in nixpkgs as linuxkit hasn't had a release since 2020 :(.

[14:16:55]-[~/go/src/github.com/linuxkit/linuxkit]─[manny@zennix]> git log1 --no-merges v0.8..ccece6a4889e15850dfbaf6d5170939c83edb103 | wc -l 124 [14:17:02]-[~/go/src/github.com/linuxkit/linuxkit]─[manny@zennix]> git log1 --no-merges ccece6a4889e15850dfbaf6d5170939c83edb103..master | wc -l 129

might want to bump our checkout of linuxkit maybe.

nshalman · 2022-03-11T17:50:41Z

Makefile


-# END: lint-install ../hook
+run: run-$(ARCH) ## Boot system using qemu 


This fails for me with error: failed to solve: rpc error: code = Unknown desc = authorization status: 401: authorization failed so there's some sort of implicit assumption about pushing that is broken when I try to test...

🤔 I'll see if I can track this down.

argh, yeah I'm able to track this down. Its mostly having to do with how prod/dev/debug builds happen. run depends on the target that pushes to $registry. That doesn't really make any sense to me but I didn't want to change that behavior. I caused the push to fail though which imo is correct behavior since we don't have versioned tags and thus the pushes end up overwriting "released" images. I'd like to sort of leave it like this for now and come back with a follow up changing to versioned images and related stuff.

mmlb · 2022-03-14T15:20:04Z

Something isn't quite right if a random person clones the repo and tries to do builds.

Do you mean something else apart from the things you've already mentioned?

We should get that cleaned up and perhaps improve the README a bit more.

Agree, will do so.

displague · 2022-04-05T15:56:36Z

@mmlb @nshalman #106 tests seem to have encountered the same unauthorized error while pushing images. Is that what's under discussion here?

mmlb · 2022-04-05T16:15:21Z

@mmlb @nshalman #106 tests seem to have encountered the same unauthorized error while pushing images. Is that what's under discussion here?

Yeah that should be handled by this PR, along with all sorts of other UX fixes.

nshalman · 2022-05-03T15:31:09Z

@mmlb I wonder if getting this fixed up and landed will unblock #111 and #119

mmlb · 2022-05-03T15:31:50Z

Not sure, yeah I'd like to get this merged this week. Sooner rather than later.

Easier to keep straight if everything is of the same name. Signed-off-by: Manuel Mendez <github@i.m.mmlb.dev>

This way the source file is named nicer and by having the TAG in the name we let make dependency tracking work. Signed-off-by: Manuel Mendez <github@i.m.mmlb.dev>

This way we only have one source file to manage and we don't have to worry about the debug matching the production one. Debug was not updated to the new kernel nor the ntpd bind mount addition. Signed-off-by: Manuel Mendez <github@i.m.mmlb.dev>

mmlb · 2022-05-23T22:08:07Z

Alrighty then! This is ready for re-review (finally...)! 🎉 PTAL @nshalman @ScottGarman

This new setup actually makes use of make's dependency tracking so that builds aren't randomly broken. It also correctly avoids rebuilds of things that haven't changed and thus don't need to be rebuilt. The previous Makefile setup was very buggy/racy if make was run with more than one job in parallel. It would also have issues if make was interrupted in the middle of a build. This one does not suffer from any of those issues. This new setup takes into account both debug vs release modes for the hook.yaml along with all the arches (new arches should be pretty easy to add in too). One of the bigger features of this new Makefile setup is that everything needed to build hook (except for the kernel) is built locally from source. This means we don't need to push images only to have linuxkit fetch them. This was only possible previously in dev-mode when we'd load the images from buildx into the local docker. This was limited to dev mode because docker doesn't support multi-arch manifests in the local cache. This makes it possible to tinker locally without the risk of pushing to quay.io unintentionally. This is also fully parameterized by both TAG and ORG, though ORG isn't accounted for very well. If ORG is modified in the Makefile please follow the directions and clean out the build dir `rm -rf out`. The TAG build parameter is fully supported and can be different between multiple `make` calls. We make use of this in CI now. Signed-off-by: Manuel Mendez <github@i.m.mmlb.dev>

No need to maintain 2 different ways of building when we can just let the Makefile do it. Signed-off-by: Manuel Mendez <github@i.m.mmlb.dev>

I didn't really see the reason to keep the files split, when ci.yaml is pretty easily made to work for both scenarios. We end up losing pushes pushing new container images for non-main branch builds if not created by dependabot. That was a lot to keep in context and also the correct thing to do. A push to a branch by someone with write permissions could end up pushing to quay and confusing things. So all in all we get better determenisitc behavior with this setup. Signed-off-by: Manuel Mendez <github@i.m.mmlb.dev>

Signed-off-by: Manuel Mendez <github@i.m.mmlb.dev>

Avoids fetching from nix cache/building over and over. Signed-off-by: Manuel Mendez <github@i.m.mmlb.dev>

This reworks the README with a bunch of fixes, including but not limited to: Updating with respect to the rest of the tinkerbell project Simpler sentences Grammar/spelling fixes Better nix help Better syntax highling for terminal sessions Signed-off-by: Manuel Mendez <github@i.m.mmlb.dev>

Signed-off-by: Manuel Mendez <github@i.m.mmlb.dev>

## Description Some fixes from after #105 was merged. - Fix `sed` so debug builds actually have the dbg stuff. - Make the files produced by `make dist` match the file names expected by boots and sandbox. - Add a dbg-dist target to get the files wanted for booting but for dbg builds. - I fixed up the `deploy` target for the new names, but have disabled it since publish is broken due to bad creds (we are also waiting on a cncf managed s3 bucket). - Fix the bash alias definitions for what used to be the docker service container that was renamed to hook-docker. ## Why is this needed Fixes things broken by refactor. ## How Has This Been Tested? Manual tests.

## Description - Makes CI green again by avoiding attempting to push the linuxkit images via `make deploy` which was deleted. - Removes the dbg only binding of /etc/daemon.json which breaks hook-docker since it ends up being mounted read-only. ## Why is this needed Fixes CI and dbg runs. ## How Has This Been Tested? CI fix should just work. I've been running with the daemon.json drop in a different branch and thought it was in branch for #130. ## How are existing users impacted? What migration steps/scripts do we need? CI is ✔️ not ❌ which makes us all feel warm and fuzzy. dbg builds are actually useful and work.

mmlb requested a review from thebsdbox February 19, 2022 02:44

mmlb force-pushed the makefile-overhaul branch 18 times, most recently from 9fbd645 to 5d3c8d7 Compare February 24, 2022 17:43

mmlb force-pushed the makefile-overhaul branch 2 times, most recently from a2bbaa7 to 0a7a8ae Compare March 2, 2022 19:50

nshalman suggested changes Mar 11, 2022

View reviewed changes

displague requested a review from nshalman April 5, 2022 15:54

mmlb mentioned this pull request Apr 9, 2022

question around use of cpio #112

Closed

mmlb force-pushed the makefile-overhaul branch from 82cd53c to 1492139 Compare May 16, 2022 17:52

mmlb marked this pull request as draft May 18, 2022 21:55

mmlb and others added 3 commits May 19, 2022 12:45

Rename container image directories/modules after the image name

664ca2b

Easier to keep straight if everything is of the same name. Signed-off-by: Manuel Mendez <github@i.m.mmlb.dev>

Go back to hook.yaml, instead save modifications to $TAG based name

4942d69

This way the source file is named nicer and by having the TAG in the name we let make dependency tracking work. Signed-off-by: Manuel Mendez <github@i.m.mmlb.dev>

mmlb force-pushed the makefile-overhaul branch 3 times, most recently from 61dbbde to 2d9e94d Compare May 23, 2022 22:07

mmlb marked this pull request as ready for review May 23, 2022 22:08

mmlb force-pushed the makefile-overhaul branch 2 times, most recently from c742a69 to 6964515 Compare May 23, 2022 22:22

ScottGarman previously approved these changes May 24, 2022

View reviewed changes

mmlb and others added 7 commits May 24, 2022 12:35

Build and push the container images using just the Makefile

4de1460

No need to maintain 2 different ways of building when we can just let the Makefile do it. Signed-off-by: Manuel Mendez <github@i.m.mmlb.dev>

hack: Drop build files and remove from docs

b1a4941

Signed-off-by: Manuel Mendez <github@i.m.mmlb.dev>

hack: Add Dockerfile that builds up nix store at build time

30b25bc

Avoids fetching from nix cache/building over and over. Signed-off-by: Manuel Mendez <github@i.m.mmlb.dev>

ci: Run make with parallel build support

7798824

Signed-off-by: Manuel Mendez <github@i.m.mmlb.dev>

mmlb dismissed ScottGarman’s stale review via 7798824 May 24, 2022 16:35

mmlb force-pushed the makefile-overhaul branch from 6964515 to 7798824 Compare May 24, 2022 16:35

nshalman approved these changes May 24, 2022

View reviewed changes

mmlb removed the request for review from thebsdbox May 24, 2022 17:07

mmlb added the ready-to-merge Signal to Mergify to merge the PR. label May 24, 2022

mergify bot merged commit deb3a19 into tinkerbell:main May 24, 2022

mmlb deleted the makefile-overhaul branch May 24, 2022 17:12

mmlb mentioned this pull request Jun 14, 2022

Few fixes from after #105 #130

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rework the Makefile so its not racy #105

Rework the Makefile so its not racy #105

mmlb commented Feb 19, 2022 •

edited

Loading

mmlb commented Feb 24, 2022

nshalman left a comment

nshalman Mar 11, 2022

mmlb Mar 14, 2022

nshalman Mar 11, 2022

mmlb Mar 14, 2022

nshalman Mar 11, 2022

mmlb Mar 14, 2022

mmlb May 10, 2022

nshalman Mar 11, 2022

mmlb Mar 14, 2022

mmlb Mar 14, 2022

mmlb commented Mar 14, 2022

displague commented Apr 5, 2022

mmlb commented Apr 5, 2022

nshalman commented May 3, 2022

mmlb commented May 3, 2022

mmlb commented May 23, 2022

		rev = "ce7b327a52d1b82f82ae061754545b1c54b06c66";
		sha256 = "1rc4if8nmy9lrig0ddihdwpzg2s8y36vf20hfywb8hph5hpsg4vj";


		# END: lint-install ../hook
		run: run-$(ARCH) ## Boot system using qemu

Rework the Makefile so its not racy #105

Rework the Makefile so its not racy #105

Conversation

mmlb commented Feb 19, 2022 • edited Loading

Description

Why is this needed

How Has This Been Tested?

How are existing users impacted? What migration steps/scripts do we need?

Checklist:

mmlb commented Feb 24, 2022

nshalman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mmlb commented Mar 14, 2022

displague commented Apr 5, 2022

mmlb commented Apr 5, 2022

nshalman commented May 3, 2022

mmlb commented May 3, 2022

mmlb commented May 23, 2022

mmlb commented Feb 19, 2022 •

edited

Loading