-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
Touch overridden files automatically #450
Conversation
Touch overridden file when it is older than file in chromium_src to trigger rebuild.
c3fe88d
to
ca9a328
Compare
|
||
if (fs.existsSync(overriddenFile)) { | ||
// If overriddenFile is older than file in chromium_src, touch it to trigger rebuild. | ||
if (fs.statSync(chromiumSrcFile).mtimeMs - fs.statSync(overriddenFile).mtimeMs > 0) { |
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.
do we really need this check? this might causes build fail after chromium upgrade even there is nothing conflicted (when chromium_src files don't need to modified after chromium upgrade).
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.
if we updated chromium then the overriden file will have an older timestamp, right? This only touches if the chromium_src file is newer
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.
what we're trying to do is to ensure that ninja detects the changes to chromium_src because we do the substitution at compile time
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.
Right, after rebasing on newer version, files in chromium_src
is older than original file.
If overridden files are updated during the rebase, it will trigger rebuild w/o this touching.
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.
ah, I see. If chromium_src timestamp gets updated after chromium upgrade, it will trigger rebuild then we don't need to touch anything to trigger it
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.
right, and we don't want to unnecessarily trigger larger rebuilds for files that haven't changed
As @bridiver 's comment(#371 (comment)), touching automatically would be much better than by using options.
Touch overridden file automatically when it is older than file in chromium_src.
Issue #352
Close #434
Submitter Checklist:
git rebase -i
to squash commits (if needed).Test Plan:
Reviewer Checklist: