Revert #1753 and add additional tests to expose previously untested behavior #1935
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
bradking
suggested changes
Mar 24, 2021
| "rule generate-depfile\n" | ||
| " command = echo \"$out: inimp\" > $depfile ; touch $out\n" | ||
| "build out: generate-depfile\n" | ||
| " depfile = out.d\n")); |
There was a problem hiding this comment.
I think this missing test_dependency = inimp.
The human-reference command= line use $test_dependency instead of inimp. Also, for consistency with DiscoveredDepDuringBuildChanged, let's put the touch $out ; first.
There was a problem hiding this comment.
I thought I'd updated all these to use the test_dependency binding. Must have missed this one in the cherry-pick.
src/build_test.cc
Outdated
| "rule touch-out-implicit-dep\n" | ||
| " command = touch $out ; sleep 1 ; touch $test_dependency\n" | ||
| "rule generate-depfile\n" | ||
| " command = touch $out; echo \"$out: $test_dependency\" > $depfile\n" |
There was a problem hiding this comment.
Whitespace consistency nit: touch $out ; echo ....
jdrouhard
force-pushed
the
revert_input_mtime
branch
from
9783ca0
to
96baedb
Compare
This reverts commit 67fbbee. There were a few missing test cases exposed by CMake's test suite that need slight adjustments. Better to revert the original attempt, add the test cases, and then re-land the change with the fixes. Fixes ninja-build#1932
These expose some behavior related to implicit deps unknown to ninja and timestamps with generating them as part of a rule. See discussions in ninja-build#1932 and ninja-build#1933.
jdrouhard
force-pushed
the
revert_input_mtime
branch
from
96baedb
to
ef2ae30
Compare
bradking
approved these changes
Mar 24, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This reverts #1753 with the intention of re-landing the change with the fixes from #1933.
Additional tests are included that recreate some CMake test suite tests (see #1932). These regression tests are useful since they expose some edge-casey behavior.
Fixes #1932.