This is an archive of the discontinued LLVM Phabricator instance.

vs integration: fix default path to clang-cl
ClosedPublic

Authored by hans on Aug 6 2018, 7:37 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

hans created this revision.Aug 6 2018, 7:37 AM

I’m pretty sure I tested this code path, was it wrong? Also I don’t think
we should be explicitly adding the \ after the $(LLVMInstallDir), if you
look through the way every other path in a VS project works, they assume
the \ is in the registry path, so if it’s possible I think we should follow
the same pattern.

hans added a comment.Aug 6 2018, 8:07 AM

I’m pretty sure I tested this code path, was it wrong?

It didn't work when I tried it.

Also I don’t think
we should be explicitly adding the \ after the $(LLVMInstallDir), if you
look through the way every other path in a VS project works, they assume
the \ is in the registry path, so if it’s possible I think we should follow
the same pattern.

I don't think we should change the path in the registry (for example that would make the vsix not work with old installs), but we can make LLVMInstallDir add a backslash on top of what it gets from the registry.

hans updated this revision to Diff 159307.Aug 6 2018, 8:08 AM

Make LLVMInstallDir include a trailing backslash.

I’m OOO this week so I can’t easily check this stuff. Sorry for all the
questions. The registry key that we create, is it the (Default) value or is
it a string value named LLVM?

hans added a comment.Aug 6 2018, 8:17 AM

I’m OOO this week so I can’t easily check this stuff. Sorry for all the
questions. The registry key that we create, is it the (Default) value or is
it a string value named LLVM?

If I understand it correctly, it's a string value named LLVM.

In that case I think the first version was correct. LLVM@LLVM means “the
value LLVM under the key LLVM”. LLVM\LLVM means “the default value under
the key LLVM which itself is under the key LLVM”

hans added a comment.Aug 6 2018, 8:26 AM

In that case I think the first version was correct. LLVM@LLVM means “the
value LLVM under the key LLVM”. LLVM\LLVM means “the default value under
the key LLVM which itself is under the key LLVM”

Arrr, no I got it backwards. It looks like this:

If you go to vs settings you can go to project -> build & run -> output
level and set it to Diagnostic. Then Ctrl+F for LLVMInstallDir and you
should see the value. Try this with @ and with \ and see if they’re
different.

Btw, unfortunately i had a few more changes to the csproj file that I may
have forgotten to upstream before i left. IIRC, they were changing the
author name to LLVMExtensions , deleting the part in the csproj about
Brutal Strong name signer, setting the key file in project settings, and
fixing the path to sn.exe.

Edit: just saw your update. That is such weird installer behavior. What
goes in the LLVM key above that? Maybe the installer behavior changed and
we need to support both?

hans added a comment.Aug 6 2018, 9:16 AM

Btw, unfortunately i had a few more changes to the csproj file that I may
have forgotten to upstream before i left. IIRC, they were changing the
author name to LLVMExtensions , deleting the part in the csproj about
Brutal Strong name signer, setting the key file in project settings, and
fixing the path to sn.exe.

Ouch, it's not so great that the published extension was built from source that's not checked in. But I guess publishing an updated version can wait until you're back.

Edit: just saw your update. That is such weird installer behavior. What
goes in the LLVM key above that? Maybe the installer behavior changed and
we need to support both?

I tried an old installer (3.8.0) and it sets the same registry keys.

I don't know if it's that weird. I think cmake/cpack just figures the package vendor is LLVM and the product is also called LLVM, and that's how it ended up there.

This revision was not accepted when it landed; it landed in state Needs Review.Aug 7 2018, 2:02 AM
This revision was automatically updated to reflect the committed changes.