Fix incremental build by preventing nonchanging writes #15817
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.
I'm not the right person to review this, but as long as it works, and Andy's scenario works -- (1) touch one JIT source file, (2) build, (3) only that one source file gets built -- then I'm happy. Of course, this needs to work for all platforms, including cross platforms like ARM.
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 don't feel qualified to review this; it looks OK given my very limited knowledge of the build process.
I'm probably also not well enough versed in python to sign off on this. And it looks more complex than I would have imagined. Not sure who to recommend... |
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.
All looks pretty good to me.
@@ -64,7 +64,7 @@ def genProviderInterface(manifest, intermediate): | |||
for pattern, replacement in replacements: | |||
header_text = re.sub(pattern, replacement, header_text) | |||
|
|||
with open_for_update(path.join(provider_dirname, mcheader_filename)) as mcheader_file: | |||
with open(path.join(provider_dirname, mcheader_filename), 'w') as mcheader_file: |
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.
Not sure of the context of this line, but considering the older version has the line "update" in it, should we be opening the file as append?
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.
open_for_update
is a function I wrote to only write to disk if the file would change. It is not relevant here because the file on disk will always end up being different. Both are functionally equivalent. I am changing it for clarity
In #15611 I took incomplete measures to prevent non-changing writes from source code generation
This PR fixes this by directing mc.exe to write to a temp directory, the copying changes files over to the final directory. This PR also fixes a bug with conflicting writes to clrxplatevents.h triggering an update when one should not have been needed.
This PR includes a rewrite of the UpdateDirectory function in utilities.py because the old one was written centered around a utility incapable of performing deep file comparison.
Closes: #15808