[PATCH] D42762: Rewrite the VS Integration Scripts

[Nico Weber via llvm-commits]

I'm not saying a corrupt file, but an empty valid pdb file.

Trying to be compatible with cl means being compatible with its observable
effects as far as reasonable.

On Fri, Feb 2, 2018 at 12:27 PM, Zachary Turner <zturner at google.com> wrote:

> I would also argue that clang-cl only tries to be compatible **with cl**.
> Not with every possible build system that might support cl.  The compiler
> should not be be trying to support the build system, it's the other way
> around.
>
> On Fri, Feb 2, 2018 at 9:24 AM Zachary Turner <zturner at google.com> wrote:
>
>> I disagree with this.  We should not be putting emitting a corrupt file
>> under any circumstances.  That's even worse for compatibility than doing
>> nothing.
>>
>> On Fri, Feb 2, 2018 at 9:24 AM Zachary Turner <zturner at google.com> wrote:
>>
>>> I tested and CMake is buggy with respect to how it handles /Zi (perhaps
>>> it's an intentional tradeoff, I don't know).  Steps to reproduce this test:
>>>
>>> 1) Build obj2yaml from LLVM using CMake and cl
>>> 2) delete tools\obj2yaml\CMakeFiles\obj2yaml.dir\vc140.pdb
>>> 3) delete tools\obj2yaml\CMakeFiles\obj2yaml.dir\coff2yaml.pdb
>>> 4) delete bin\obj2yaml.pdb
>>> 5) ninja obj2yaml
>>>
>>> You'll get these warnings:
>>> LINK : program database D:\src\llvmbuild\cl\Debug\x64\bin\obj2yaml.pdb
>>> missing; performing full link
>>> obj2yaml.cpp.obj : warning LNK4099: PDB 'vc140.pdb' was not found with
>>> 'obj2yaml.cpp.obj' or at 'D:\src\llvmbuild\cl\Debug\x64\bin\vc140.pdb';
>>> linking object as if no debug info
>>> dwarf2yaml.cpp.obj : warning LNK4099: PDB 'vc140.pdb' was not found with
>>> 'dwarf2yaml.cpp.obj' or at 'D:\src\llvmbuild\cl\Debug\x64\bin\vc140.pdb';
>>> linking object as if no debug info
>>> elf2yaml.cpp.obj : warning LNK4099: PDB 'vc140.pdb' was not found with
>>> 'elf2yaml.cpp.obj' or at 'D:\src\llvmbuild\cl\Debug\x64\bin\vc140.pdb';
>>> linking object as if no debug info
>>> macho2yaml.cpp.obj : warning LNK4099: PDB 'vc140.pdb' was not found with
>>> 'macho2yaml.cpp.obj' or at 'D:\src\llvmbuild\cl\Debug\x64\bin\vc140.pdb';
>>> linking object as if no debug info
>>> wasm2yaml.cpp.obj : warning LNK4099: PDB 'vc140.pdb' was not found with
>>> 'wasm2yaml.cpp.obj' or at 'D:\src\llvmbuild\cl\Debug\x64\bin\vc140.pdb';
>>> linking object as if no debug info
>>> Error.cpp.obj : warning LNK4099: PDB 'vc140.pdb' was not found with
>>> 'Error.cpp.obj' or at 'D:\src\llvmbuild\cl\Debug\x64\bin\vc140.pdb';
>>> linking object as if no debug info
>>>
>>> What it should be doing is saying that every cpp file, in addition to
>>> $(FileName).obj, also produces vc140.pdb.  MSBuild gets this correct
>>>
>>> On Fri, Feb 2, 2018 at 9:05 AM Zachary Turner <zturner at google.com>
>>> wrote:
>>>
>>>> It’s an additional pdb, which the linker then uses as an input. The
>>>> linker still writes the final pdb. The msbuild task that actually invokes
>>>> cl.exe is implemented in a dll, so we can only observe its effects.
>>>>
>>>> I’m not sure how cmake is implemented, but this is really a build
>>>> system issue of inputs, outputs, and dependencies. If cmake does have
>>>> issues, patching cmake seems like the appropriate solution.
>>>> On Fri, Feb 2, 2018 at 8:54 AM Hans Wennborg <hans at chromium.org> wrote:
>>>>
>>>>> I'd still like to understand how msbuild.exe is running the build to
>>>>> expect vc140.pdb to be written, and why we're outputing the .pdb file
>>>>> in the wrong place. Or is vc140.pdb some additional pdb that's
>>>>> supposed to get output on the side?
>>>>>
>>>>> I don't like the idea of the vs integration rewriting flags. What
>>>>> about users not using msbuild but cmake? Should cmake start rewriting
>>>>> flags for clang-cl too?
>>>>>
>>>>> On Fri, Feb 2, 2018 at 5:49 PM, Zachary Turner <zturner at google.com>
>>>>> wrote:
>>>>> > That’s why we unset the option in msbuild, so it no longer expects
>>>>> that. The
>>>>> > current solution makes it behave exactly as if /Z7 had been chosen
>>>>> in the
>>>>> > UI. Having the compiler write a type server pdb seems like an
>>>>> enormous
>>>>> > amount of work, and writing a 0 byte file seems odd.
>>>>> >
>>>>> > On Fri, Feb 2, 2018 at 8:46 AM Hans Wennborg <hans at chromium.org>
>>>>> wrote:
>>>>> >>
>>>>> >> 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
>>>>> >> >>> >> >> >
>>>>>
>>>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20180202/77d7e41f/attachment.html>