Issue 10952045: Developer installation from within Visual Studio (Closed)
DescriptionSUPERSEDED: http://codereview.adblockplus.org/11058028/
Patch set 1:
This isn't fully working yet, but it's ready for initial review. The one thing that's not working is that 'regedit' is not writing to the registry when invoked from within the Makefile. The same .REG file (for debugging copied out to a .txt file during the build), however, works fine when invoked directly from the command line, which then causes the 64-bit plugin to work. I've got no idea why this might be.
Also, I discovered that you cannot use HKCU to register a BHO, contrary to some previous information I had bookmarked. (HKCU to register the CLSID works fine.) That means that there's a need for a UAC elevation. Luckily, for a developer install, you only need to do it once.
The amount of ugly hacking to get this to work was far more than I expected. Passing Visual Studio variables to the Makefile command line needs to be done manually, contrary to what's in the VS dialog both. There are problems in dealing with inconsistent conventions for string quoting, needed for spaces in file names. There's a need to break out of the 32-bit ghetto of NMAKE to write to the 64-bit registry.
Patch set 2:
The BHO key sits within HKLM. The only thing in the build it depends upon is the CLSID of the BHO, which is basically fixed. Registering this key semi-permanently does no harm; if the CLSID ID isn't found, there's no IE error. Therefore, I moved add/remove for this key into a pair of ".reg" files. The developer can merge these into the registry from Explorer by double-clicking. Also, this is exactly the same value for this key that get registered by the ordinary installer, so there's no conflict there.
I added checking for this key to the project build, so that there's a notice to the developer if it's not present. Fortuitously, Visual Studio even marks it with a yellow warning flag. Therefore, at a tiny cost to the developer, we can avoid the entire issue of UAC elevation from within VS for now, maybe forever. We can always revisit it later if it turns out to be a problem.
There's a build target "query"; use "nmake -f Makefile.DevInstall query" to invoke it from the command line. It looks for every relevant registry entry that could lead to IE not loading the BHO. I used it to check that everything was in place. It's much faster than looking manually with regedit. I'm sure that someone will find use for it later.
Patch Set 1 #
Total comments: 16
Patch Set 2 : Took UAC out of the build loop #
Total comments: 8
Patch Set 3 : Fixes addressing comments #
MessagesTotal messages: 10
Looking forward to seeing the elevation prompt implemented as we discussed it. http://codereview.adblockplus.org/10952045/diff/1/AdblockPlusDeveloperInstall... File AdblockPlusDeveloperInstall.vcxproj (right): http://codereview.adblockplus.org/10952045/diff/1/AdblockPlusDeveloperInstall... AdblockPlusDeveloperInstall.vcxproj:106: <NMakeMacros>$(NMakeMacros) OutDir="$(OutDir)\" DLL="$(OutDir)AdblockPlusx64.dll" DLL32="$(OutDir32)AdblockPlus.dll"</NMakeMacros> We should be registering the DLL that was built, not both of them. So if we build the x64 DLL then we should register it. If we build the x86 DLL then we register that one here. http://codereview.adblockplus.org/10952045/diff/1/Makefile.DevInstall File Makefile.DevInstall (right): http://codereview.adblockplus.org/10952045/diff/1/Makefile.DevInstall#newcode2 Makefile.DevInstall:2: # NMAKE is a 32-bit application, which makes it problematic to make 64-bit registry entries with the standard command-line tools such as "reg.exe". Quite frankly, I don't think that we want to use nmake in this project - maintaining it is a pain. You can use a custom build step in the MSVS project to run any commands you want (http://msdn.microsoft.com/en-us/library/hefydhhy%28v=vs.80%29.aspx). And we will soon migrate the build system to gyp which will make changing the project files less awkward. http://codereview.adblockplus.org/10952045/diff/1/Makefile.DevInstall#newcode62 Makefile.DevInstall:62: << I would rather use reg.exe to add these entries as well.
http://codereview.adblockplus.org/10952045/diff/1/AdblockPlusDeveloperInstall... File AdblockPlusDeveloperInstall.vcxproj (right): http://codereview.adblockplus.org/10952045/diff/1/AdblockPlusDeveloperInstall... AdblockPlusDeveloperInstall.vcxproj:106: <NMakeMacros>$(NMakeMacros) OutDir="$(OutDir)\" DLL="$(OutDir)AdblockPlusx64.dll" DLL32="$(OutDir32)AdblockPlus.dll"</NMakeMacros> On a 64-bit system, we have to register both DLL's. That's because 64-bit Windows comes with two versions of IE (both 32 and 64 bit) and each needs its own DLL. This mirrors the behavior of the installer, incidentally, which does the same thing. http://codereview.adblockplus.org/10952045/diff/1/Makefile.DevInstall File Makefile.DevInstall (right): http://codereview.adblockplus.org/10952045/diff/1/Makefile.DevInstall#newcode2 Makefile.DevInstall:2: # NMAKE is a 32-bit application, which makes it problematic to make 64-bit registry entries with the standard command-line tools such as "reg.exe". The whole point of this project is to provide a developer install for a Visual Studio user. It's not needed for the build itself, but rather is a convenience for the developer so that they don't have to run install/uninstall with each build-test cycle. 'gyp' isn't a generic front end for VS, so the project is pretty much independent of that concern. The issue is that we need to get access within Visual Studio both to the "build" and "clean" actions on a project. (You need to clean a developer install in order to test a full installation, since the HKCU registry entries override and will cause misbehavior unless they're removed.) The makefile project is the only way to do that, at least, the only way I know of that works with the project types available in VS Express without plugins. A makefile project doesn't need to use nmake, admittedly, but it's convenient for it. Rebuild is essentially clean-then-build. If we were to put the commands straight into the command lines, we'd be dealing with a maintenance problem to keep everything in sync. That means using some kind of external command. So a far as external commands, the reason I went with nmake is for parameter passing. We need to get some paths out of the VS environment, and that basically means putting them on the command line. Nmake has a syntax for named arguments, and that's much better than using batch files. I agree with you overall. This whole thing is an ugly hack, but the ugliness is a result of the limitations of the environment: Visual Studio Express and its default toolset. If I were doing my version of the _right_ way, I'd write a tool in C++ that used system calls to lock down every variation that caused problems. I suspect that's more effort than it's worth, however. http://codereview.adblockplus.org/10952045/diff/1/Makefile.DevInstall#newcode62 Makefile.DevInstall:62: << On 2013/06/18 15:18:33, Wladimir Palant wrote: > I would rather use reg.exe to add these entries as well. I would too, but see the comment above. "reg.exe" just fails if you use it here. (Tried that.) It's configured not to ask for UAC escalation, which is required to write to HKLM from an ordinary medium integrity process.
http://codereview.adblockplus.org/10952045/diff/1/AdblockPlusDeveloperInstall... File AdblockPlusDeveloperInstall.vcxproj (right): http://codereview.adblockplus.org/10952045/diff/1/AdblockPlusDeveloperInstall... AdblockPlusDeveloperInstall.vcxproj:106: <NMakeMacros>$(NMakeMacros) OutDir="$(OutDir)\" DLL="$(OutDir)AdblockPlusx64.dll" DLL32="$(OutDir32)AdblockPlus.dll"</NMakeMacros> On 2013/06/18 16:00:19, Eric wrote: > On a 64-bit system, we have to register both DLL's. This is not the installer - it's our development environment. Developers usually don't compile all DLLs, as they only run one version of Internet Explorer for testing. So this code should register whichever DLL was compiled. http://codereview.adblockplus.org/10952045/diff/1/Makefile.DevInstall File Makefile.DevInstall (right): http://codereview.adblockplus.org/10952045/diff/1/Makefile.DevInstall#newcode2 Makefile.DevInstall:2: # NMAKE is a 32-bit application, which makes it problematic to make 64-bit registry entries with the standard command-line tools such as "reg.exe". On 2013/06/18 16:00:19, Eric wrote: > The whole point of this project is to provide a developer install for a Visual > Studio user. It's not needed for the build itself, but rather is a convenience > for the developer so that they don't have to run install/uninstall with each > build-test cycle. So far no contradictions - that's why you made it a separate project, it can be run separately. > 'gyp' isn't a generic front end for VS, so the project is > pretty much independent of that concern. Yes, we will merely use gyp to generate the project files because editing them (with eight configurations) manually is messy. > The issue is that we need to get access within Visual Studio both to the "build" > and "clean" actions on a project. You can use the AfterClean target for that: http://msdn.microsoft.com/en-us/library/ms366724.aspx > So a far as external commands, the reason I went with nmake is for parameter > passing. While I didn't quite get the issue, I doubt that there is something you can do with nmake here that cannot be done with a regular project file. http://codereview.adblockplus.org/10952045/diff/1/Makefile.DevInstall#newcode62 Makefile.DevInstall:62: << Yes, reg.exe runs with the privileges that you run it with. In other words, you need to start it elevated. I was somehow under the impression that you can do it with the runas command line tool - you cannot. However, http://technet.microsoft.com/en-us/magazine/2007.06.utilityspotlight.aspx links to an elevate.vbs script. While I won't suggest using that script, we can easily copy the approach in our own elevate.js that can be part of the repository: var WMI = GetObject("winmgmts:{impersonationLevel=impersonate}!\\\\.\\root\\cimv2"); var version = WMI.Get("Win32_OperatingSystem=@").Version; var shell = WScript.CreateObject("Shell.Application"); var app = WScript.Arguments.Item(0); var cmdLine = ""; for (var i = 1; i < WScript.Arguments.length; i++) cmdLine += ' "' + WScript.Arguments.Item(i) + '"'; var verb = (parseInt(version) >= 6 ? "runas" : "open"); shell.ShellExecute(app, cmdLine.substr(1), "", verb); This should work correctly on XP as well - will use the "open" verb instead of "runas" then. This script can be run during build like that: wscript //nologo elevate.js reg.exe ...
http://codereview.adblockplus.org/10952045/diff/1/AdblockPlusDeveloperInstall... File AdblockPlusDeveloperInstall.vcxproj (right): http://codereview.adblockplus.org/10952045/diff/1/AdblockPlusDeveloperInstall... AdblockPlusDeveloperInstall.vcxproj:106: <NMakeMacros>$(NMakeMacros) OutDir="$(OutDir)\" DLL="$(OutDir)AdblockPlusx64.dll" DLL32="$(OutDir32)AdblockPlus.dll"</NMakeMacros> On 2013/06/19 07:14:15, Wladimir Palant wrote: > This is not the installer - it's our development environment. Developers usually > don't compile all DLLs, as they only run one version of Internet Explorer for > testing. Point taken. http://codereview.adblockplus.org/10952045/diff/1/Makefile.DevInstall File Makefile.DevInstall (right): http://codereview.adblockplus.org/10952045/diff/1/Makefile.DevInstall#newcode2 Makefile.DevInstall:2: # NMAKE is a 32-bit application, which makes it problematic to make 64-bit registry entries with the standard command-line tools such as "reg.exe". On 2013/06/19 07:14:15, Wladimir Palant wrote: > While I didn't quite get the issue, I doubt that there is something you can do > with nmake here that cannot be done with a regular project file. Probably. If we want to rewrite it later, I won't care. This whole thing was intended to be a one-day hack and it's been way more than that, fighting random Microsoft nonsense. I've been more worried about the result (getting the right registry keys) than control structure (using nmake). Once we have the right results, writing a different control structure should be fairly straightforward. If you want to use 'gyp' to generate control structure, it's fine with me; I don't have a strong opinion about it. The parameter passing issue has to do with string quoting and spaces in path names. My original idea was to write out a .reg file and import it with a single command. That doesn't (easily) work because spaces need backslash escapes inside the file. 'reg.exe' does not require them to be quoted on the command line, so I went with multiple invocations of that tool. http://codereview.adblockplus.org/10952045/diff/1/Makefile.DevInstall#newcode62 Makefile.DevInstall:62: << On 2013/06/19 07:14:15, Wladimir Palant wrote: > Yes, reg.exe runs with the privileges that you run it with. In other words, you > need to start it elevated. I came to the same conclusion shortly after yesterday's meeting. The whole reason regedit.exe was there in the first place was that I thought it would elevate UAC and do the write without further ado. It asks for UAC elevation but then doesn't actually write; I still have no good reason why it would fail like that. At that point I threw up my hands and pushed a review out. > I was somehow under the impression that you can do it with the runas command > line tool - you cannot. The issue with "runas" is that it's for running as a different user (read: Administrator), not running under the same user with UAC elevation. MS should have added such arguments to "runas" when Vista came out, but no. > While I won't suggest using that script, we can easily > copy the approach in our own elevate.js that can be part of the > repository I figured out while I was out for errands yesterday afternoon a way to avoid UAC elevation entirely, so we can defer the issue entirely. I'll amend the review shortly. Basically, the one thing we need in HKLM doesn't depend on anything in the build except the CLSID of the BHO, which is basically constant anyway. So we can provide a way of getting that registry entry in place outside the build, and that means we can avoid dealing with UAC elevation inside of it. Having said that, we might have need of such an elevate script at some point, for this or another reason. I had the beginnings of the same thought, but I wanted to avoid use of Script Host, if only because it's another MS-specific thing to burden future developers with.
See edited description for patch set 2.
http://codereview.adblockplus.org/10952045/diff/1/Makefile.DevInstall File Makefile.DevInstall (right): http://codereview.adblockplus.org/10952045/diff/1/Makefile.DevInstall#newcode2 Makefile.DevInstall:2: # NMAKE is a 32-bit application, which makes it problematic to make 64-bit registry entries with the standard command-line tools such as "reg.exe". On 2013/06/19 15:22:54, Eric wrote: > I've been more worried about the result (getting the right registry keys) than > control structure (using nmake). Once we have the right results, writing a > different control structure should be fairly straightforward. Could you do that before checking this in? I would *really* prefer this to be sorted out beforehand. MSVS projects calling nmake is the stuff of my nightmares. http://codereview.adblockplus.org/10952045/diff/1/Makefile.DevInstall#newcode62 Makefile.DevInstall:62: << On 2013/06/19 15:22:54, Eric wrote: > Basically, the one thing we need in HKLM doesn't depend on anything in the build > except the CLSID of the BHO, which is basically constant anyway. So we can > provide a way of getting that registry entry in place outside the build, and > that means we can avoid dealing with UAC elevation inside of it. In other words, you want to ask developers to run that registry file manually? I would still rather have that happen automatically if necessary, that's the whole point of having a separate build step for the development environment. http://codereview.adblockplus.org/10952045/diff/8002/AdblockPlusDeveloperInst... File AdblockPlusDeveloperInstall.vcxproj (right): http://codereview.adblockplus.org/10952045/diff/8002/AdblockPlusDeveloperInst... AdblockPlusDeveloperInstall.vcxproj:160: <NMakeReBuildCommandLine>nmake -nologo -f Makefile.DevInstall rebuild 32$(NMakeMacros)</NMakeReBuildCommandLine> Space at the wrong place? http://codereview.adblockplus.org/10952045/diff/8002/Makefile.DevInstall File Makefile.DevInstall (right): http://codereview.adblockplus.org/10952045/diff/8002/Makefile.DevInstall#newc... Makefile.DevInstall:57: # On 64-bit Windows, invoking it this way ends up with the 32-bit version of 'reg'. How about renaming the current REG variable into REG64 and defining REG32 as %WINDIR%\SysWOW64\reg.exe to be used here? Then you can save that lengthy explanation and it's immediately obvious what's going on. http://codereview.adblockplus.org/10952045/diff/8002/Makefile.DevInstall#newc... Makefile.DevInstall:74: !if [$(REG) query "HKEY_LOCAL_MACHINE\SOFTWARE\Microsoft\Windows\CurrentVersion\Explorer\Browser Helper Objects\{FFCB3198-32F3-4E8B-9539-4324694ED664}" 2>&1 >nul] What about HKEY_LOCAL_MACHINE\SOFTWARE\Wow6432Node\Microsoft\Windows\CurrentVersion\ Explorer\Browser Helper Objects\{FFCB3198-32F3-4E8B-9539-4324694ED664} - doesn't that key have to be set as well? I would expect check_BHO32 and check_BHO64, with the 32 bit and 64 bit reg.exe respectively. http://codereview.adblockplus.org/10952045/diff/8002/Makefile.DevInstall#newc... Makefile.DevInstall:87: # N.B. We don't delete the CLSID in HKLM for clean. That wouldn't be our key, but the installer's. IMHO, since this is about the development environment we have to assume that it's our key - developers usually don't run the installer. And even if they do, they will just run it again - it's obvious that the development environment will interfere with the installer.
I realized I haven't put the removal of the "regsvr32" calls and the other post-build events into the review. Does anybody want to see them? http://codereview.adblockplus.org/10952045/diff/1/Makefile.DevInstall File Makefile.DevInstall (right): http://codereview.adblockplus.org/10952045/diff/1/Makefile.DevInstall#newcode2 Makefile.DevInstall:2: # NMAKE is a 32-bit application, which makes it problematic to make 64-bit registry entries with the standard command-line tools such as "reg.exe". On 2013/06/20 07:10:38, Wladimir Palant wrote: > Could you do that before checking this in? I would *really* prefer this to be > sorted out beforehand. MSVS projects calling nmake is the stuff of my > nightmares. Well, I could. But it's going to make me really cranky. I'm already fed up with dealing with undocumented and badly documented Microsoft nonsense. Changing to another project type and another packaging for the command line tools (a bunch of batch files) is days long, not hours. It's basically starting from scratch on half the project. And it's going to be harder to understand, because whatever its faults, NMAKE at least has a certain amount of modularity built in. The simple alternatives do not. The other ways I know of dealing with this using standard tools are inferior in terms of comprehension and maintenance. The time to revisit this is when you convert to 'gyp'. There's necessarily going to be some amount of overlap when that happens. Might as well address it later. http://codereview.adblockplus.org/10952045/diff/1/Makefile.DevInstall#newcode62 Makefile.DevInstall:62: << On 2013/06/20 07:10:38, Wladimir Palant wrote: > In other words, you want to ask developers to run that registry file manually? I > would still rather have that happen automatically if necessary, that's the whole > point of having a separate build step for the development environment. Yes, manually. It's a simple double-click in Explorer. And there's a warning that comes up if the key isn't there, with the name of the file to click on. If UAC elevation simply worked, it would be better. But it's not simply working. It's going to take a lot of screwing around to get it working. It doesn't seem worth the time right now. The separate build step is necessary to register the CLSID for the DLL, whose location depends on check-out directory and build configuration. The BHO key, on the other hand, is essentially constant. http://codereview.adblockplus.org/10952045/diff/8002/AdblockPlusDeveloperInst... File AdblockPlusDeveloperInstall.vcxproj (right): http://codereview.adblockplus.org/10952045/diff/8002/AdblockPlusDeveloperInst... AdblockPlusDeveloperInstall.vcxproj:160: <NMakeReBuildCommandLine>nmake -nologo -f Makefile.DevInstall rebuild 32$(NMakeMacros)</NMakeReBuildCommandLine> On 2013/06/20 07:10:38, Wladimir Palant wrote: > Space at the wrong place? Yes. http://codereview.adblockplus.org/10952045/diff/8002/Makefile.DevInstall File Makefile.DevInstall (right): http://codereview.adblockplus.org/10952045/diff/8002/Makefile.DevInstall#newc... Makefile.DevInstall:57: # On 64-bit Windows, invoking it this way ends up with the 32-bit version of 'reg'. On 2013/06/20 07:10:38, Wladimir Palant wrote: > How about renaming the current REG variable into REG64 and defining REG32 as > %WINDIR%\SysWOW64\reg.exe to be used here? Then you can save that lengthy > explanation and it's immediately obvious what's going on. I want to disagree with one thing. Nothing is ever immediately obvious about Windows and multiple architectures. I've been steeped in it for a few weeks and I still get hung up occasionally. Renaming to save a comment is not a good tradeoff in this matter. That would be the right thing if we were only ever to run on 64-bit Windows. If we're running on 32-bit Windows, the SysWOW64 directory doesn't exist. If you want a rename, then REG is really REG_NATIVE, that is, 64-bit reg.exe on 64-bit Windows and 32-bit reg.exe on 32-bit Windows. "reg" could be REG32, because invoking it that way from NMAKE, a 32-bit program, always gets the 32-bit version. If you invoke "reg" from the ordinary command line on 64-bit Windows, then you get 64-bit "reg.exe", because the ordinary command line is a 64-bit program. http://codereview.adblockplus.org/10952045/diff/8002/Makefile.DevInstall#newc... Makefile.DevInstall:74: !if [$(REG) query "HKEY_LOCAL_MACHINE\SOFTWARE\Microsoft\Windows\CurrentVersion\Explorer\Browser Helper Objects\{FFCB3198-32F3-4E8B-9539-4324694ED664}" 2>&1 >nul] On 2013/06/20 07:10:38, Wladimir Palant wrote: > What about > HKEY_LOCAL_MACHINE\SOFTWARE\Wow6432Node\Microsoft\Windows\CurrentVersion\ > Explorer\Browser Helper Objects\{FFCB3198-32F3-4E8B-9539-4324694ED664} - doesn't > that key have to be set as well? I would expect check_BHO32 and check_BHO64, > with the 32 bit and 64 bit reg.exe respectively. You're correct. I also need to add it to the query target below. http://codereview.adblockplus.org/10952045/diff/8002/Makefile.DevInstall#newc... Makefile.DevInstall:87: # N.B. We don't delete the CLSID in HKLM for clean. That wouldn't be our key, but the installer's. On 2013/06/20 07:10:38, Wladimir Palant wrote: > IMHO, since this is about the development environment we have to assume that > it's our key - developers usually don't run the installer. And even if they do, > they will just run it again - it's obvious that the development environment will > interfere with the installer. There are three reasons not to delete installer keys. 1) There's no need to. If the HKCU keys for CLSID are registered, they override those in HKLM. (The BHO keys are identical.) 2) It's not a good idea to alter any asset registered by the installer. At best you trigger a Windows Installer "repair" action. 3) We don't have support for writing to HKLM right now. That includes deleting keys.
Superseded by http://codereview.adblockplus.org/11058028/ and closed. 
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
