-
Notifications
You must be signed in to change notification settings - Fork 274
Unify make and cmake behaviours #5993
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
Conversation
regression/cbmc/Makefile
Outdated
@$(MAKE) -f $(THIS_FILE) test-paths-lifo | ||
@$(MAKE) -f $(THIS_FILE) test-cprover-smt2 |
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.
How about making the test
target depend on test-paths-lifo
and test-cprover-smt2
?
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.
Yes, either that, or make default
target depend on test-paths-lifo
and test-cprover-smt2
?
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.
One issue to consider here is: if we add these additional targets as dependencies for test
, we wouldn't be able to run "just test
" ...
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.
Good call. The right approach probably is to rename the existing test
target and introduce a new (PHONY
) test
target.
Codecov Report
@@ Coverage Diff @@
## develop #5993 +/- ##
========================================
Coverage 77.79% 77.79%
========================================
Files 1567 1567
Lines 179707 179707
========================================
Hits 139797 139797
Misses 39910 39910 Continue to review full report at Codecov.
|
I'm wondering whether tests would pass if this were rebased onto current develop, which includes #5985. |
ce343c9
to
b787c13
Compare
regression/Makefile
Outdated
done; | ||
|
||
# Pattern to execute a single test suite directory | ||
.PHONY: $(DIRS) | ||
$(DIRS): | ||
@echo "Running $@..." ; | ||
$(MAKE) -C "$@" || exit 1; | ||
$(MAKE) -C "$@" test || exit 1; |
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.
Wouldn't test-parallel
(below) also need this change?
This fixes the inconsistencies between cmake and make behaviours.
Change from calling the separate make targets inside a default to instead using make dependency style.
7340252
to
bc54138
Compare
This fixes the inconsistencies between cmake and make
behaviours.
Fixes #5986