[PATCH] D42762: Rewrite the VS Integration Scripts

[Hans Wennborg via llvm-commits]

But if the build system invokes the compiler + linker, expects to end
up with vc140.pdb but instead ends up with foo.pdb, maybe the compiler
+ linker is not interpreting the flags passed from the build system
correctly.

On Fri, Feb 2, 2018 at 5:44 PM, Zachary Turner <zturner at google.com> wrote:
> Also, since this is a build system issue and not a compiler issue, it seems
> intuitive to me to deal with it at the build system level
> On Fri, Feb 2, 2018 at 8:42 AM Zachary Turner <zturner at google.com> wrote:
>>
>> It’s a compiler generated pdb, /Zi means “all compiler processes should
>> write to the same pdb”, whereas /Z7 means “put the debug info in the object
>> files instead”. If the user does a clean build the file will get deleted and
>> there won’t even be anything to touch. The file name comes from another flag
>> (/Fo or /Fd, can’t remember) which msbuild defaults to vc$(ToolsetName)
>> On Fri, Feb 2, 2018 at 8:36 AM Hans Wennborg <hans at chromium.org> wrote:
>>>
>>> Yes, but I'd like to understand exactly why. Where does the name
>>> vc140.pdb come from? What is supposed to go into this file? Maybe
>>> clang-cl should touch it when invoked with /Zi?
>>>
>>> On Fri, Feb 2, 2018 at 5:32 PM, Zachary Turner <zturner at google.com>
>>> wrote:
>>> > Symptom: when /Zi is selected, VS always rebuilds all source files,
>>> > even if
>>> > just 1 (possibly even none) have changed.
>>> >
>>> > Fix: Change /Zi to /Z7 in the UI
>>> >
>>> > More details here: https://bugs.llvm.org/show_bug.cgi?id=36140
>>> >
>>> > On Fri, Feb 2, 2018 at 8:12 AM Hans Wennborg <hans at chromium.org> wrote:
>>> >>
>>> >> On Fri, Feb 2, 2018 at 4:40 PM, Zachary Turner <zturner at google.com>
>>> >> wrote:
>>> >> >
>>> >> > On Fri, Feb 2, 2018 at 6:23 AM Hans Wennborg <hans at chromium.org>
>>> >> > wrote:
>>> >> >>
>>> >> >> (Your reply didn't go to Phabricator, so re-adding folks subscribed
>>> >> >> there.)
>>> >> >>
>>> >> >> On Thu, Feb 1, 2018 at 9:08 PM, Zachary Turner via llvm-commits
>>> >> >> <llvm-commits at lists.llvm.org> wrote:
>>> >> >> > I'm kind of imagining this world where we have one VS Integration
>>> >> >> > that
>>> >> >> > works
>>> >> >> > no matter what version of LLVM you have.  The nice thing about
>>> >> >> > this
>>> >> >> > is
>>> >> >> > that
>>> >> >> > it allows it to work with hermetic toolchains, older versions of
>>> >> >> > LLVM
>>> >> >> > that
>>> >> >> > may already be installed on a user's machine, local dev builds of
>>> >> >> > LLVM,
>>> >> >> > etc.
>>> >> >>
>>> >> >> I'm on board with this. It seems useful especially for the case
>>> >> >> where
>>> >> >> the developer may have multiple LLVM toolchains installed and want
>>> >> >> to
>>> >> >> point at a specific one. It would be nice if we could still trigger
>>> >> >> the installation of the toolset when installing the LLVM toolchain
>>> >> >> though.
>>> >> >>
>>> >> >> But for the integration to work regardless of LLVM version, I don't
>>> >> >> think the integration can bake in assumptions about what flags
>>> >> >> clang-cl supports and re-map them etc. The set of flags supported
>>> >> >> by
>>> >> >> clang-cl and how they're handled changes frequently.
>>> >> >>
>>> >> >> I like the idea of a VS integration that's LLVM-version independent
>>> >> >> --
>>> >> >> the current one is almost that except for the baked in version
>>> >> >> number
>>> >> >> -- but for it to work, I think it has to be really simple,
>>> >> >> basically
>>> >> >> just pointing MSVC at clang-cl.exe and nothing more.
>>> >> >
>>> >> > I’ve already mentioned at least one case whereas this is impossible
>>> >> > (/Zi
>>> >> > vs
>>> >> > /Z7), and given that there are thousands of lines of msbuild logic
>>> >> > that
>>> >> > are
>>> >> > running and processing these options before they make it to
>>> >> > clang-cl,
>>> >> > I’m
>>> >> > certain there are more that we don’t yet know about.
>>> >>
>>> >> I'd like to understand the /Zi vs /Z7 thing better. Can you ELI5 the
>>> >> problem?
>>> >>
>>> >>
>>> >> >
>>> >> > Simple is nice, I don’t disagree with that, but not at the expense
>>> >> > of
>>> >> > user
>>> >> > experience.  i still don’t think there’s any maintenance issues here
>>> >> > though.
>>> >> > I think the current version here could probably sit for 5+ years and
>>> >> > never
>>> >> > need to be touched, continuing to work both with future VS versions
>>> >> > and
>>> >> > future clang-cl versions unmodified.
>>> >> >
>>> >> >
>>> >> >>
>>> >> >>
>>> >> >> > The only maintenance burden I can think of is one where we remove
>>> >> >> > or
>>> >> >> > add
>>> >> >> > flags in clang-cl, which doesn't happen very often, if ever.
>>> >> >>
>>> >> >> It puzzles me that you think we rarely or ever change the flags
>>> >> >> clang-cl supports or how they're handled. In my experience, the
>>> >> >> flags
>>> >> >> change every release.
>>> >> >>
>>> >> >> >  Any flag that
>>> >> >> > is added to MSVC doesn't require any action from us.
>>> >> >>
>>> >> >> Depends on the flag, no?
>>> >> >
>>> >> > Not really.  Any flag that msvc adds, assuming we don’t update this
>>> >> > file,
>>> >> > gets passed through to clang-cl which is what you’re proposing i do
>>> >> > with
>>> >> > all
>>> >> > options anyway.
>>> >> >
>>> >> >
>>> >> >>
>>> >> >>
>>> >> >> > I plan to expose a UI for optimizations and warning, so I could
>>> >> >> > see a
>>> >> >> > maintenace burden when we add new -W or -f flags that are not
>>> >> >> > exposed
>>> >> >> > to
>>> >> >> > the
>>> >> >> > UI.  But those can still be specified via additional compiler
>>> >> >> > flags.
>>> >> >> > And
>>> >> >> > the maintenance burden is actually less than coupling it to the
>>> >> >> > installed
>>> >> >> > toolchain because we can do it at our leisure, rather than being
>>> >> >> > pressed
>>> >> >> > to
>>> >> >> > get it done by a release.
>>> >> >>
>>> >> >> But you're saying that the toolset should be independent of the
>>> >> >> LLVM
>>> >> >> version? If we add a -Wfoobar flag in Clang x.y.z and want to
>>> >> >> expose
>>> >> >> that in your UI, that UI then needs to be conditional on what
>>> >> >> version
>>> >> >> of Clang it's targeting. Same thing if we remove -Wquux in another
>>> >> >> Clang version. This sounds like a maintenance nightmare to me.
>>> >> >
>>> >> > If we do nothing, any added -W options are still available via
>>> >> > Additional
>>> >> > Compiler Flags.   So as with all the other custom logic in the
>>> >> > msbuild
>>> >> > files, we’re still not obligated to maintain that, and it will still
>>> >> > continue to work fine
>>> >> >
>>> >> > For options that we remove, sure, we should update the file.  One
>>> >> > way to
>>> >> > handle this would be to add a new version of clang-cl.xml every
>>> >> > release,
>>> >> > and
>>> >> > conditionally include the proper xml file.  How frequently do we
>>> >> > remove
>>> >> > warnings though?  Doing so would cause people’s builds to break
>>> >> > because
>>> >> > they’d be passing unrecognized options, so I suspect it’s almost
>>> >> > never.
>>> >> >
>>> >> > Of all the things though, this is the one that I think it’s most
>>> >> > important
>>> >> > to accept the maintenance burden of.  This is the difference between
>>> >> > “we
>>> >> > put
>>> >> > the minimum amount of work possible into getting this working so we
>>> >> > could do
>>> >> > other things” and “we care about this, we made it as easy as
>>> >> > possible to
>>> >> > use, we designed it with VS users in mind”.  As someone who used VS
>>> >> > through
>>> >> > the UI exclusively for over 15 years, there’s going to be a huge
>>> >> > difference
>>> >> > between providing this feature and not providing it.
>>> >> >
>>> >> > I *still* don’t see the maintenance burden as being high though.  We
>>> >> > can
>>> >> > release a new clang-cl.xml like every 2-3 years and it would take
>>> >> > all of
>>> >> > 30
>>> >> > minutes to put it together and get it on the marketplace.
>>> >> >
>>> >> >
>>> >> >
>>> >> >>
>>> >> >>
>>> >> >>
>>> >> >> > One thing I could maybe do to lower the maintenance burden a
>>> >> >> > little
>>> >> >> > is
>>> >> >> > to
>>> >> >> > try to have some better logic for detecting the clang version.
>>> >> >> > We
>>> >> >> > were
>>> >> >> > already using the registry before anyway to find the installed
>>> >> >> > LLVM,
>>> >> >> > maybe
>>> >> >> > there's a way for me to just figure out the version without the
>>> >> >> > additional
>>> >> >> > registry value.  I'll have to look into that though.
>>> >> >> >
>>> >> >> >
>>> >> >> >
>>> >> >> > On Thu, Feb 1, 2018 at 11:09 AM Zachary Turner
>>> >> >> > <zturner at google.com>
>>> >> >> > wrote:
>>> >> >> >>
>>> >> >> >> On Thu, Feb 1, 2018 at 10:48 AM Hans Wennborg via Phabricator
>>> >> >> >> <reviews at reviews.llvm.org> wrote:
>>> >> >> >>>
>>> >> >> >>> hans added inline comments.
>>> >> >> >>>
>>> >> >> >>>
>>> >> >> >>> ================
>>> >> >> >>> Comment at: llvm/tools/msbuild/Clang.Cpp.Common.props:41
>>> >> >> >>> +
>>> >> >> >>> +    <!-- The registry key may not be set if it's an old
>>> >> >> >>> installer,
>>> >> >> >>> try
>>> >> >> >>> the newest version that exists -->
>>> >> >> >>> +    <LLVMVersion Condition="'$(LLVMVersion)' == '' and
>>> >> >> >>>
>>> >> >> >>> Exists('$(LLVMInstallDir)\lib\clang\7.0.0')">7.0.0</LLVMVersion>
>>> >> >> >>> ----------------
>>> >> >> >>> zturner wrote:
>>> >> >> >>> > hans wrote:
>>> >> >> >>> > > As I mentioned before, separating the toolset config from
>>> >> >> >>> > > the
>>> >> >> >>> > > actual
>>> >> >> >>> > > toolchain installation makes me a little nervous.
>>> >> >> >>> > >
>>> >> >> >>> > > But if we're doing it, the version checks below should
>>> >> >> >>> > > probably
>>> >> >> >>> > > include the .1 versions too, i.e. at least 5.0.1 and 6.0.1.
>>> >> >> >>> > Unless we're going to release the full thing including the
>>> >> >> >>> > compiler,
>>> >> >> >>> > linker, etc through the marketplace I don't see an
>>> >> >> >>> > alternative.
>>> >> >> >>> > In
>>> >> >> >>> > any
>>> >> >> >>> > case, I actually think this it's preferable this way.
>>> >> >> >>> > There's
>>> >> >> >>> > nothing
>>> >> >> >>> > really about the two that benefits from having them coupled
>>> >> >> >>> > together, as far
>>> >> >> >>> > as I can see.   I'm willing to be convinced though, if we can
>>> >> >> >>> > figure
>>> >> >> >>> > out how
>>> >> >> >>> > to to do it so that we can still ship it on the marketplace.
>>> >> >> >>> "There's nothing really about the two that benefits from having
>>> >> >> >>> them
>>> >> >> >>> coupled together,"
>>> >> >> >>>
>>> >> >> >>> The toolset needs to know at least where to find the toolchain
>>> >> >> >>> and
>>> >> >> >>> how
>>> >> >> >>> to
>>> >> >> >>> invoke it. If we decouple them, there needs to be an interface
>>> >> >> >>> between
>>> >> >> >>> them
>>> >> >> >>> that can't change: in this case the LLVM path and version
>>> >> >> >>> number in
>>> >> >> >>> the
>>> >> >> >>> registry.
>>> >> >> >>
>>> >> >> >> Has that ever changed?  Doesn’t seem too onerous, using the
>>> >> >> >> registry
>>> >> >> >> is
>>> >> >> >> the windows way anyway, if anything this feels like the proper
>>> >> >> >> way.
>>> >> >> >>
>>> >> >> >>
>>> >> >> >>>
>>> >> >> >>>
>>> >> >> >>> But at the same time you're baking in all this logic in the
>>> >> >> >>> toolset
>>> >> >> >>> about
>>> >> >> >>> how to invoke the toolchain, what flags are supported, etc.
>>> >> >> >>> Those
>>> >> >> >>> things are
>>> >> >> >>> strongly dependent on the toolchain, which in this de-coupled
>>> >> >> >>> world
>>> >> >> >>> seems
>>> >> >> >>> problematic. It seems like you're actually making the coupling
>>> >> >> >>> tighter
>>> >> >> >>> in
>>> >> >> >>> that way, except you still want to ship the two parts
>>> >> >> >>> separately.
>>> >> >> >>>
>>> >> >> >>> Are there restrictions in the marketplace about how big a vsix
>>> >> >> >>> can
>>> >> >> >>> be?
>>> >> >> >>> Because if not, I think we could just package up
>>> >> >> >>> clang+headers+runtime
>>> >> >> >>> into
>>> >> >> >>> a vsix and ship the whole thing, and maybe that would be the
>>> >> >> >>> best
>>> >> >> >>> thing.
>>> >> >> >>
>>> >> >> >> An installer is very large though, and even if it’s allowed it’s
>>> >> >> >> kind
>>> >> >> >> of
>>> >> >> >> obnoxious to have to download a large amount of stuff if only
>>> >> >> >> one
>>> >> >> >> thing
>>> >> >> >> changes.  Being able to push changes to the Integration
>>> >> >> >> independently
>>> >> >> >> of an
>>> >> >> >> llvm release seems like a feature to me.
>>> >> >> >>
>>> >> >> >>  coupling it would also make it more difficult to use a custom
>>> >> >> >> built
>>> >> >> >> llvm
>>> >> >> >> toolchain, i can just change a registry setting right now and
>>> >> >> >> it’s
>>> >> >> >> good
>>> >> >> >> to
>>> >> >> >> go.  Even the builtin VS toolchains use the registry to locate
>>> >> >> >> paths,
>>> >> >> >> and we
>>> >> >> >> were already reading the registry before this anyway
>>> >> >> >>
>>> >> >> >>
>>> >> >> >>>
>>> >> >> >>>
>>> >> >> >>> Or we could just stick to the current installer version and
>>> >> >> >>> make it
>>> >> >> >>> a
>>> >> >> >>> little smarter about finding VS2017. Maybe instead of the batch
>>> >> >> >>> files
>>> >> >> >>> we
>>> >> >> >>> write an actual program that finds the installation and copies
>>> >> >> >>> the
>>> >> >> >>> files.
>>> >> >> >>
>>> >> >> >> I definitely think vsix is the way to go.  I’d hate to stick
>>> >> >> >> with
>>> >> >> >> batch
>>> >> >> >> files and not use the proper method of having an extension.
>>> >> >> >> It’s
>>> >> >> >> also
>>> >> >> >> more
>>> >> >> >> discoverable as an extension on the marketplace.
>>> >> >> >>
>>> >> >> >>
>>> >> >> >>>
>>> >> >> >>>
>>> >> >> >>>
>>> >> >> >>> ================
>>> >> >> >>> Comment at: llvm/tools/msbuild/LLVM.props:8
>>> >> >> >>> +    <!-- Friendly names added to the PlatformToolset in the
>>> >> >> >>> property
>>> >> >> >>> pages. -->
>>> >> >> >>> +    <_PlatformToolsetFriendlyNameFor_llvm
>>> >> >> >>> Condition="'$(_PlatformToolsetFriendlyNameFor_llvm)' ==
>>> >> >> >>> ''">Clang
>>> >> >> >>> for
>>> >> >> >>> Windows</_PlatformToolsetFriendlyNameFor_llvm>
>>> >> >> >>> +  </PropertyGroup>
>>> >> >> >>> ----------------
>>> >> >> >>> zturner wrote:
>>> >> >> >>> > hans wrote:
>>> >> >> >>> > > Hmm, we previously intentionally called the toolset "LLVM"
>>> >> >> >>> > > with
>>> >> >> >>> > > the
>>> >> >> >>> > > thinking that it would eventually include lld and
>>> >> >> >>> > > designated
>>> >> >> >>> > > the
>>> >> >> >>> > > complete
>>> >> >> >>> > > llvm toolchain, not just Clang. And is the "for Windows"
>>> >> >> >>> > > part
>>> >> >> >>> > > necessary?
>>> >> >> >>> > Do you think there's any value in having a toolset that does
>>> >> >> >>> > Clang+Link
>>> >> >> >>> > and a second one that does Clang+LLD?  Or do you think we
>>> >> >> >>> > should
>>> >> >> >>> > stick with
>>> >> >> >>> > only a single one?  I can change the name to LLVM though.
>>> >> >> >>> The best would be to only have one, but where the user could
>>> >> >> >>> select
>>> >> >> >>> between the two linkers, I think.
>>> >> >> >>
>>> >> >> >> Yea.  Can try that in a followup, may be tricky though
>>> >> >> >>
>>> >> >> >>>
>>> >> >> >>>
>>> >> >> >>>
>>> >> >> >>> ================
>>> >> >> >>> Comment at: llvm/tools/msbuild/Toolset.targets:38
>>> >> >> >>> +
>>> >> >> >>> +    <!-- Warn if Fiber Safe Optimizations are enabled, and
>>> >> >> >>> then
>>> >> >> >>> ignore
>>> >> >> >>> them. -->
>>> >> >> >>> +    <Warning
>>> >> >> >>> Condition="'%(ClCompile.EnableFiberSafeOptimizations)'
>>> >> >> >>> ==
>>> >> >> >>> 'true'"
>>> >> >> >>> ----------------
>>> >> >> >>> zturner wrote:
>>> >> >> >>> > hans wrote:
>>> >> >> >>> > > This seems to duplicate a lot of logic from clang-cl. It's
>>> >> >> >>> > > nice
>>> >> >> >>> > > to
>>> >> >> >>> > > provide a good UI for the user, but maintaining this seems
>>> >> >> >>> > > a
>>> >> >> >>> > > lot
>>> >> >> >>> > > of work.
>>> >> >> >>> > > Are you not concerned that this will rot?
>>> >> >> >>> > I don't think it will.  Maybe I'm being overly optimistic
>>> >> >> >>> > here,
>>> >> >> >>> > but
>>> >> >> >>> > the
>>> >> >> >>> > only case we would ever need to maintain this again is if we
>>> >> >> >>> > started
>>> >> >> >>> > supporting these options.  Fiber Safe Optimizations, for
>>> >> >> >>> > example,
>>> >> >> >>> > I'm pretty
>>> >> >> >>> > sure we will never support.  If MSVC ever removes the option,
>>> >> >> >>> > for
>>> >> >> >>> > example,
>>> >> >> >>> > we can do nothing and continue to work.
>>> >> >> >>> >
>>> >> >> >>> > We could also just silently ignore them and just pass the
>>> >> >> >>> > option
>>> >> >> >>> > through to clang-cl, but these are pretty unusual options
>>> >> >> >>> > with
>>> >> >> >>> > pretty
>>> >> >> >>> > specialized use cases, so I feel like if I had gone out of my
>>> >> >> >>> > way
>>> >> >> >>> > to
>>> >> >> >>> > enable
>>> >> >> >>> > such a strange option I would want to know if the compiler
>>> >> >> >>> > was
>>> >> >> >>> > not
>>> >> >> >>> > going to
>>> >> >> >>> > respect it.
>>> >> >> >>> I feel pretty strongly that we should handle this clang-cl
>>> >> >> >>> side. If
>>> >> >> >>> a
>>> >> >> >>> flag is not supported, either we should ignore it, or if it's
>>> >> >> >>> something the
>>> >> >> >>> user would want to know about us not supporting, we should
>>> >> >> >>> warn.
>>> >> >> >>> That's what
>>> >> >> >>> clang-cl tries to do currently, and if there are flags we don't
>>> >> >> >>> get
>>> >> >> >>> right,
>>> >> >> >>> we should fix it.
>>> >> >> >>>
>>> >> >> >>> And we do move flags from the unsupported to supported category
>>> >> >> >>> now
>>> >> >> >>> and
>>> >> >> >>> then, so the "only case we would ever need to maintain this
>>> >> >> >>> again
>>> >> >> >>> is
>>> >> >> >>> if we
>>> >> >> >>> started supporting these options" scenario is real.
>>> >> >> >>
>>> >> >> >> It’s not a matter of clang-cl doing it right or wrong, it’s that
>>> >> >> >> there
>>> >> >> >> are
>>> >> >> >> other moving parts before it even gets to clang-cl.
>>> >> >> >> Specifically,
>>> >> >> >> MSBuild.
>>> >> >> >> We’ve already seen one example of how  just letting clang-cl do
>>> >> >> >> its
>>> >> >> >> thing is
>>> >> >> >> insufficient, and nothing we can ever do in clang-cl can fix
>>> >> >> >> that.
>>> >> >> >> Given
>>> >> >> >> that it’s required sometimes, and that doing it for all options
>>> >> >> >> doesn’t
>>> >> >> >> increase our maintenance burden, i think it makes sense to do it
>>> >> >> >> everywhere
>>> >> >> >> and never have to deal with msbuild issues again.
>>> >> >> >>
>>> >> >> >>
>>> >> >> >>>
>>> >> >> >>>
>>> >> >> >>>
>>> >> >> >>> ================
>>> >> >> >>> Comment at: llvm/tools/msbuild/Toolset.targets:46
>>> >> >> >>> +             File="@(ClCompile)(0,0)"
>>> >> >> >>> +             Text="clang-cl does not support MSVC Link Time
>>> >> >> >>> Optimization.  Disable this option in compatibility settings to
>>> >> >> >>> silence this
>>> >> >> >>> warning."/>
>>> >> >> >>> +
>>> >> >> >>> ----------------
>>> >> >> >>> zturner wrote:
>>> >> >> >>> > hans wrote:
>>> >> >> >>> > > But maybe we want clang-cl to map this to -flto one day.
>>> >> >> >>> > > Now we
>>> >> >> >>> > > need
>>> >> >> >>> > > to update two places. And with the toolset/toolchain
>>> >> >> >>> > > install
>>> >> >> >>> > > split, the two
>>> >> >> >>> > > places may be installed separately :-/
>>> >> >> >>> > That's even better then.  All we have to do is change this
>>> >> >> >>> > xml,
>>> >> >> >>> > push
>>> >> >> >>> > a
>>> >> >> >>> > new build to the market place, and the VS UI will update
>>> >> >> >>> > their
>>> >> >> >>> > extension for
>>> >> >> >>> > them.
>>> >> >> >>> >
>>> >> >> >>> > Note that we could do the mapping at the MSBuild level, in
>>> >> >> >>> > this
>>> >> >> >>> > file
>>> >> >> >>> > down below where we have an `ItemGroup`.  Just add a line
>>> >> >> >>> > that
>>> >> >> >>> > says
>>> >> >> >>> > `<AdditionalOptions
>>> >> >> >>> > Condition="%(ClCompile.WholeProgramOptimization)' ==
>>> >> >> >>> > 'true'>-flto=thin %(AdditionalOptions)</AdditionalOptions>`
>>> >> >> >>> >
>>> >> >> >>> > and we can do this without touching clang.
>>> >> >> >>> But the toolset is decoupled from the toolchain in your
>>> >> >> >>> proposal.
>>> >> >> >>>
>>> >> >> >>> Not only would we need to update both clang-cl and this file,
>>> >> >> >>> but
>>> >> >> >>> this
>>> >> >> >>> file would need to handle clang-cl versions both before and
>>> >> >> >>> after.
>>> >> >> >>
>>> >> >> >> We wouldn’t have to update clang-cl.  We could map ltcg to
>>> >> >> >> -flto=thin
>>> >> >> >> in
>>> >> >> >> the extension and it would automatically work with the installed
>>> >> >> >> toolchain.
>>> >> >> >>
>>> >> >> >>
>>> >> >> >>>
>>> >> >> >>>
>>> >> >> >>>
>>> >> >> >>> ================
>>> >> >> >>> Comment at: llvm/tools/msbuild/Toolset.targets:83
>>> >> >> >>> +
>>> >> >> >>> +    <!-- Warn if XML Documentation is generated, and then
>>> >> >> >>> ignore
>>> >> >> >>> it.
>>> >> >> >>> -->
>>> >> >> >>> +    <Warning
>>> >> >> >>> Condition="'%(ClCompile.GenerateXMLDocumentationFiles)'
>>> >> >> >>> ==
>>> >> >> >>> 'true'"
>>> >> >> >>> ----------------
>>> >> >> >>> zturner wrote:
>>> >> >> >>> > hans wrote:
>>> >> >> >>> > > Keeping up with all these flags seems like a huge amount of
>>> >> >> >>> > > work.
>>> >> >> >>> > > Why
>>> >> >> >>> > > not just let clang-cl ignore it?
>>> >> >> >>> > See the large comment at the top of the file.  For some
>>> >> >> >>> > options,
>>> >> >> >>> > we
>>> >> >> >>> > could probably get by with this.  Maybe even this one, I
>>> >> >> >>> > debated
>>> >> >> >>> > on
>>> >> >> >>> > this
>>> >> >> >>> > particular one.
>>> >> >> >>> >
>>> >> >> >>> > My bar was "If the option fundamentally changes assumptions
>>> >> >> >>> > about
>>> >> >> >>> > the
>>> >> >> >>> > way code could be compiled, we should generate an error.  If
>>> >> >> >>> > it
>>> >> >> >>> > changes the
>>> >> >> >>> > behavior of the language in a way we don't support,  changes
>>> >> >> >>> > the
>>> >> >> >>> > way
>>> >> >> >>> > we
>>> >> >> >>> > generate code in a meaningful way, or causes specialized
>>> >> >> >>> > output
>>> >> >> >>> > files to be
>>> >> >> >>> > written, warn, and if it's an option we ignore then drop it"
>>> >> >> >>> >
>>> >> >> >>> > The last category there we could probably just pass through
>>> >> >> >>> > in
>>> >> >> >>> > some
>>> >> >> >>> > cases, but in that comment I also mentioned a case where
>>> >> >> >>> > setting
>>> >> >> >>> > an
>>> >> >> >>> > option
>>> >> >> >>> > that clang-cl ignores impacts MSBuild's ability to figure out
>>> >> >> >>> > dependencies
>>> >> >> >>> > and ends up causing a full rebuild every time even when
>>> >> >> >>> > nothing
>>> >> >> >>> > changed.
>>> >> >> >>> >
>>> >> >> >>> > We can scour the entire cl build tasks and try to discover if
>>> >> >> >>> > any
>>> >> >> >>> > other
>>> >> >> >>> > ones have unintended consequences, but I think it's easier to
>>> >> >> >>> > just
>>> >> >> >>> > turn them
>>> >> >> >>> > off at the MSBuild level.  And as a side benefit, the user
>>> >> >> >>> > gets
>>> >> >> >>> > shorter
>>> >> >> >>> > command lines, which is always nice.
>>> >> >> >>> >
>>> >> >> >>> > As for maintenance, this all looks like zero-maintenance code
>>> >> >> >>> > to
>>> >> >> >>> > me.
>>> >> >> >>> > Did you have an example in mind of where we'd need to update
>>> >> >> >>> > this?
>>> >> >> >>> > Whether
>>> >> >> >>> > it be a new VS version, or VS dropping support for one of
>>> >> >> >>> > these
>>> >> >> >>> > options or
>>> >> >> >>> > deprecating them, I don't think we'd have to do anything.
>>> >> >> >>> The maintenance would come from when clang-cl changes how it
>>> >> >> >>> handles
>>> >> >> >>> some
>>> >> >> >>> option, or when VS adds new options.
>>> >> >> >>
>>> >> >> >> But these are all really obscure options that we will probably
>>> >> >> >> never
>>> >> >> >> touch.  When vc adds new options we’re not obligated to update
>>> >> >> >> this
>>> >> >> >> file
>>> >> >> >> either.
>>> >> >> >>
>>> >> >> >>
>>> >> >> >>>
>>> >> >> >>>
>>> >> >> >>>
>>> >> >> >>> ================
>>> >> >> >>> Comment at: llvm/tools/msbuild/install.bat:10
>>> >> >> >>> +REM Older versions of VS would look for these files in the
>>> >> >> >>> Program
>>> >> >> >>> Files\MSBuild directory
>>> >> >> >>> +REM but with VS2017 it seems to look for these directly in the
>>> >> >> >>> Visual
>>> >> >> >>> Studio instance.
>>> >> >> >>> +REM This means we'll need to do a little extra work to
>>> >> >> >>> properly
>>> >> >> >>> detect
>>> >> >> >>> all the various
>>> >> >> >>> ----------------
>>> >> >> >>> zturner wrote:
>>> >> >> >>> > hans wrote:
>>> >> >> >>> > > Don't we want to support at least 2015 too?
>>> >> >> >>> > Mentioned in the other review, but the install.bat file
>>> >> >> >>> > shouldn't
>>> >> >> >>> > really be used anymore except for during development.  The
>>> >> >> >>> > VSIX
>>> >> >> >>> > supports
>>> >> >> >>> > both 2015 and 2017 (I tested it in both and confirmed it
>>> >> >> >>> > works)
>>> >> >> >>> Hmm, but then we should delete it, or at least take it out of
>>> >> >> >>> the
>>> >> >> >>> installer, and we need a replacement. As it is now, if we land
>>> >> >> >>> this,
>>> >> >> >>> it
>>> >> >> >>> breaks the installer for versions before 2017.
>>> >> >> >>
>>> >> >> >>
>>> >> >> >> I thought i took it out of the installer, but maybe I missed
>>> >> >> >> something.
>>> >> >> >> We still need it for dev purposes because it allows us to
>>> >> >> >> overwrite
>>> >> >> >> the
>>> >> >> >> existing installed version with new files
>>> >> >> >
>>> >> >> >
>>> >> >> > _______________________________________________
>>> >> >> > llvm-commits mailing list
>>> >> >> > llvm-commits at lists.llvm.org
>>> >> >> > http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>>> >> >> >