-
Notifications
You must be signed in to change notification settings - Fork 277
CBMC_VERSION: Use generated include files instead of command-line defines #2393
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -229,23 +229,18 @@ OBJ += $(patsubst %.cc, %$(OBJEXT), $(filter %.cc, $(SRC))) | |
|
||
# get version from git | ||
GIT_INFO = $(shell git describe --tags --always --dirty || echo "n/a") | ||
GIT_INFO_FILE = .release_info | ||
RELEASE_INFO = \#define CBMC_VERSION "$(CBMC_VERSION) ($(GIT_INFO))" | ||
GIT_INFO_FILE = version.h | ||
|
||
CBMC_VERSION_FILES += $(filter %_parse_options$(OBJEXT), $(OBJ)) | ||
$(GIT_INFO_FILE): | ||
echo '$(RELEASE_INFO)' > $@ | ||
|
||
ifeq ($(BUILD_ENV_),MSVC) | ||
$(CBMC_VERSION_FILES): CP_CXXFLAGS += /DCBMC_VERSION='"""$(CBMC_VERSION) ($(GIT_INFO))"""' | ||
else | ||
$(CBMC_VERSION_FILES): CP_CXXFLAGS += -DCBMC_VERSION="\"$(CBMC_VERSION) ($(GIT_INFO))\"" | ||
endif | ||
$(filter %_parse_options$(OBJEXT), $(OBJ)): $(GIT_INFO_FILE) | ||
|
||
# mark the actually generated file as a phony target to enforce a rebuild - but | ||
# only of the version information has changed! | ||
KNOWN_RELEASE_INFO = $(shell cat $(GIT_INFO_FILE) 2>/dev/null) | ||
ifneq ($(GIT_INFO), $(KNOWN_RELEASE_INFO)) | ||
$(CBMC_VERSION_FILES): $(GIT_INFO_FILE) | ||
|
||
$(GIT_INFO_FILE): | ||
echo $(GIT_INFO) > $@ | ||
|
||
ifneq ($(RELEASE_INFO), $(KNOWN_RELEASE_INFO)) | ||
.PHONY: $(GIT_INFO_FILE) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh, I see - this is some sneaky logic to avoid updating the timestamp on the GIT_INFO_FILE if the version info hasn't changed - nice, but could you add a comment to explain that ? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'll add that comment, but the build failures on Travis and AppVeyor make me wonder whether this actually works as it should (it definitively does so locally). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I had messed up quoting. Thus it did not actually work as supposed... I'll push an update addressing this once #2415 is merged. |
||
endif | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -38,11 +38,6 @@ LIBS = | |
|
||
CLEANFILES = goto-cc$(EXEEXT) goto-cl$(EXEEXT) | ||
|
||
CBMC_VERSION_FILES = as_mode$(OBJEXT) \ | ||
compile$(OBJEXT) \ | ||
gcc_mode$(OBJEXT) \ | ||
goto_cc_mode$(OBJEXT) | ||
|
||
include ../config.inc | ||
include ../common | ||
|
||
|
@@ -51,6 +46,8 @@ all: goto-cl$(EXEEXT) | |
endif | ||
all: goto-cc$(EXEEXT) | ||
|
||
as_mode$(OBJEXT) compile$(OBJEXT) gcc_mode$(OBJEXT) goto_cc_mode$(OBJEXT): $(GIT_INFO_FILE) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ditto my comment about whether the dependency would now be auto-generated from the |
||
|
||
ifneq ($(wildcard ../jsil/Makefile),) | ||
OBJ += ../jsil/jsil$(LIBEXT) | ||
CP_CXXFLAGS += -DHAVE_JSIL | ||
|
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.
Given that the version info is now pulled in by a standard
#include
- do you actually need to add explicit rules for the dependency any more? Wouldn't that be picked up by.d
files just like any other include ?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.
I had thought the same, but actually this isn't true: the
.d
file will only be built as a side-effect of compilation, and thus only helps for re-builds - it won't do the trick in a clean build tree!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.
Oh right, I see. I wonder if we can arrange for the generation target to be run in the first build without having to add these dependencies explicitly like this...