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
Makefile: fix "no such file" error on first invocation #14257
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.
LGTM. I was just thinking the other day that it was odd that we were using the LDFLAGS variable here when that's also used (with different semantics) by make's default implicit rules (which we fortunately don't use).
One nit: Maybe call it GO_LDFLAGS
(or LINKFLAGS
?) to avoid confusion with https://en.wikipedia.org/wiki/Gold_(linker)
e0ca596
to
6ae4b58
Compare
TFTRs! I went with |
Makefile
Outdated
@@ -83,16 +83,21 @@ TAR_XFORM_FLAG = $(shell $(TAR) --version | grep -q GNU && echo "--xform=s" || e | |||
|
|||
GIT_DIR := $(shell git rev-parse --git-dir 2> /dev/null) | |||
|
|||
# We intentionally use LINKFLAGS instead of the more traditional LDFLAGS |
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.
can this be up near the other flags? this one may conceivably be overridden on the command line or in the environment. for now, we can put it in the command-line only section.
Currently, if you run Make on a fresh Git clone without a .buildinfo directory, you'll see the following output printed $ make cat: .buildinfo/tag: No such file or directory cat: .buildinfo/rev: No such file or directory cat: .buildinfo/tag: No such file or directory cat: .buildinfo/rev: No such file or directory cat: .buildinfo/rev: No such file or directory after which Make, remarkably, manages to proceed with a successful build. (You can see this in an existing clone with `rm -rf .buildinfo && make`.) As best as I can tell, this is because Make treats LDFLAGS specially and expands it before executing any target recipe, even if that recipe does not reference LDFLAGS. Go figure. Our current Makefile assumes the LDFLAGS variable won't be expanded until it's referenced in the `install` recipe, at which point .buildinfo/tag and .buildinfo/rev are guaranteed to exist, because they're prerequisites of `install`. But due to the above oddity, LDFLAGS is expanded during execution the recipes to create .buildinfo/*, and since LDFLAGS expands to `$(shell cat .buildinfo/...)`, we see the above error message--but only on the first invocation, when .buildinfo/* does not yet exist. Note that the build proceeds successfully because LDFLAGS is *re-expanded* when executing the `install` target, where .buildinfo/* exists with the correct values. This commit works around the issue by using a variable which behaves normally (namely, LINKFLAGS), in place of LDFLAGS.
6ae4b58
to
fb5a23d
Compare
Currently, if you run Make on a fresh Git clone without a .buildinfo directory, you'll see the following output printed
after which Make, remarkably, manages to proceed with a successful build. (You can see this in an existing clone with
rm -rf .buildinfo && make
.) As best as I can tell, this is because Make treatsLDFLAGS
specially and expands it before executing any target recipe, even if that recipe does not referenceLDFLAGS
. Go figure.Our current Makefile assumes the
LDFLAGS
variable won't be expanded until it's referenced in theinstall
recipe, at which point .buildinfo/tag and .buildinfo/rev are guaranteed to exist, because they're prerequisites ofinstall
. But due to the above oddity, LDFLAGS is expanded during execution the recipes to create .buildinfo/*, and sinceLDFLAGS
expands to$(shell cat .buildinfo/...)
, we see the above error message--but only on the first invocation, when .buildinfo/* does not yet exist. Note that the build proceeds successfully becauseLDFLAGS
is re-expanded when executing theinstall
target, where .buildinfo/* exists with the correct values.This commit works around the issue by using a variable which behaves normally (namely,
GOLDFLAGS
), in place ofLDFLAGS
.NOTE: I'm perfectly happy to pursue another approach here (or at least drop an explanatory comment in the Makefile if this approach seems reasonable). For example, we could instead set LDFLAGS as a shell variable inside of the
install
recipe (rather than as a target-specific variable). The downside with that approach is that we'd lose the output of the expanded command: instead ofgo build... -ldflags '-X ...build.tag=12345'
, we'd seego build... -ldflags "$ldflags"
.This change is