Well, we were already passing INCLUDE through so they could already include system headers .  I could add a parameter to the function like `allow_system_library_use` but I’m not sure if the value of that is high enough to warrant it.  I don’t have a strong opinion though <br><div class="gmail_quote"><div dir="ltr">On Fri, Nov 30, 2018 at 7:28 AM Nico Weber via Phabricator <<a href="mailto:reviews@reviews.llvm.org">reviews@reviews.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">thakis added a comment.<br>
<br>
In D54963#1310190 <<a href="https://reviews.llvm.org/D54963#1310190" rel="noreferrer" target="_blank">https://reviews.llvm.org/D54963#1310190</a>>, @zturner wrote:<br>
<br>
> I don't think that ever actually happened, or at least I never observed it.  The real behavior change is that we're now passing through "LIB", "VCINSTALLDIR", "VCToolsInstallDir", "VSINSTALLDIR", "WindowsSdkDir", and "WindowsSdkVersion", whereas before we were not propagating them into the child environment at all.  clang-cl reads these, so it could have an observable impact on clang-cl (but any observable impact should be beneficial, since before it would actually just fail if a test tried to include a system header or link against a system library)<br>
<br>
<br>
At least for clang tests, tests reading system libraries are usually a bug -- tests are supposed to be hermetic, cross-platform, etc. It sounds like now it's easier to accidentally use a system header?<br>
<br>
<br>
Repository:<br>
  rL LLVM<br>
<br>
CHANGES SINCE LAST ACTION<br>
  <a href="https://reviews.llvm.org/D54963/new/" rel="noreferrer" target="_blank">https://reviews.llvm.org/D54963/new/</a><br>
<br>
<a href="https://reviews.llvm.org/D54963" rel="noreferrer" target="_blank">https://reviews.llvm.org/D54963</a><br>
<br>
<br>
<br>
</blockquote></div>