makefile: make runc non-PHONY target #4873
Conversation
|
We can also do this for the test binaries and/or |
Making runc a .PHONY target means that we rebuild it each time, even if the source code does not change. This can cause issues if you are trying attach uprobes a binary that then gets replaced when you try to run integration tests with it. The "static" target needed to be adjusted a little, but now it's just a special case of the standard "runc" target. Also, the "runc-bin" and "static-bin" rules were really not necessary in the first place... Signed-off-by: Aleksa Sarai <[email protected]>
| runc: runc-bin | ||
| GO_SRC := \ | ||
| go.mod go.sum \ | ||
| $(shell find . -type f -name '*.go' -or -name '*.c') |
There was a problem hiding this comment.
And //go:embed files
Line 25 in d845c4a
There was a problem hiding this comment.
Let me cook up some awk-foo to make this work without hardcoding them...
There was a problem hiding this comment.
@cyphar I'm not convinced we want to stop relying on the compiler for this on every runc compilation.
I'm not very familiar with uprobes, how are you attaching them?
If you really need the binary to not change, I think something like: cp runc runc-uprobes; make; <overwrite runc-uprobes only if md5 is different> is a simpler alternative that doesn't put the price of this limitation (discover files we need to trigger a recompile) on everyone else not using uprobes.
It would be great if go provided, like gcc, a feature to list the dependencies in a makefile-style fashion. Or if we can say to go build: only overwrite if there are changes. These might be good issues for go, though? I haven't searched if they already rejected something like this or what.
There was a problem hiding this comment.
I'm not very familiar with uprobes, how are you attaching them?
bpftrace -e 'uprobe:./runc:symbol { ... }'
I was wrong though -- the Go compiler doesn't replace the binary if it would be identical (the inode number is unchanged after make) so this patch is unnecessary. I'm not sure why it didn't work when I was testing it before...
There was a problem hiding this comment.
This patch also does not rebuild, if the target has changed.
akhil@akhil-ThinkPad-L14:~/g/s/g/o/runc (makefile-deps)$ make runc
go build -trimpath "-buildmode=pie" -tags "seccomp urfave_cli_no_docs" -ldflags "-X main.gitCommit=0d6527ad " -o runc .
akhil@akhil-ThinkPad-L14:~/g/s/g/o/runc (makefile-deps)$ make static
make: Nothing to be done for 'static'.
Did not create a static runc binary, but ideally thats the expected behaviour.
| .PHONY: runc-bin | ||
| runc-bin: | ||
| $(GO_BUILD) -o runc . | ||
| .DEFAULT: runc |
There was a problem hiding this comment.
This DEFAULT will be a no-op since there is another target defined before.
So just calling make will trigger the static build, rather than the current existing one.
|
@akhilerm I would've preferred to have a The "dumb" solution would be to just rebuild |
I mean, they should get a pretty obvious 404 from doing so and update their packaging, right? 😇 |
There was a problem hiding this comment.
One other issue I see is when GO= is explicitly specified, runc won't be rebuilt. This will break one of my usual scenarios, in a hard-to-notice way.
Similar for GOEXPERIMENT= although I rarely use it.
Are there alternative solutions for the issue described? I'd really like go to worry about dependencies, not us.
| # "static" just maps to runc with a different "go build" invocation. This lets | ||
| # us avoid rebuilding if the sources haven't changed -- even with .PHONY. | ||
| .PHONY: static | ||
| static: export GO_BUILD=$(GO_BUILD_STATIC) |
There was a problem hiding this comment.
nit: is export really needed here? Looks like not, since we don't use sub-make, but maybe I'm missing something.
|
This is not actually necessary -- I just tested this and the Go compiler doesn't actually replace the binary if there are no changes, and so uprobes do actually work. I'm not sure why it was failing when I tried this before but it's actually fine. |
Making runc a .PHONY target means that we rebuild it each time, even if
the source code does not change. This can cause issues if you are trying
attach uprobes a binary that then gets replaced when you try to run
integration tests with it.
The "static" target needed to be adjusted a little, but now it's just a
special case of the standard "runc" target.
Also, the "runc-bin" and "static-bin" rules were really not necessary in
the first place...
Signed-off-by: Aleksa Sarai [email protected]