-
Notifications
You must be signed in to change notification settings - Fork 5.4k
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
build: write a gcc-style depfile when using clang-cl on windows #1525
Conversation
I can't say I really understand what's happening here. So I'm mostly concerned from a long term maintenance perspective: Should this patch be undone when mozilla/sccache#246 is fixed? Or is this something that could be sent upstream to Chrome? I think you should describe the symptoms that this is fixing in the commit message - i.e. that libdeno doesn't get rebuilt properly on windows |
Does this fix #1424 ? |
It could probably go upstream; there's no disadvantages to it. |
da7e49c
to
a9e7bf2
Compare
So maybe add a comment that lets the future maintainer know when they can safely undo that patch. I guess it's when https://reviews.llvm.org/D46394 gets into stable? |
MSVC (cl.exe) can't write a depfile, but it has the sccache adds the Now what happens if you pass both flags? So dependency tracking was broken. With some effort clang-cl can however write a gcc-style depfile, so that's what I'm making it do here. |
Did you see this denoland/chromium_build@75a7f82...1e3840b#diff-215e0f644aef180fe353893e8703f3f2R186 ? |
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.
LGTM - thanks for the explanation.
Consider mentioning in the commit message what this fixes from a Deno user’s perspective.
This ensures deno gets rebuild properly when .c/.cc source files are modified. Fixes: denoland#1424
a9e7bf2
to
6ffdb93
Compare
Because clang-cl doesn't support msvc-style
/showIncludes
header dep detection, at least not in such way that it works properly with sccache.