Issue 29324576: Issue 2599, 2940 - Fix the presence of correct ATL version (Closed)
Patch Set 1 #
Total comments: 3
Patch Set 2 : add tested environments #
Total comments: 7
Patch Set 3 : address comments #
Total comments: 5
Patch Set 4 : address comments #
Total comments: 5
Patch Set 5 : address comment, precise python version #MessagesTotal messages: 22
LGTM
NOT LGTM. Still doesn't compile. I downloaded the patch to test it. A very cursory examination of my installation of VS 2013 Community shows that there's no directly named "atlmfc". The environment variable asked for designates VS 2013. I just cannot believe that this solution was tested in a free-tool environment, with Visual Studio 2012 Express installed and some later free version installed for ATL. If that were the case, such an obvious error would have been caught. ----------- Also, sorry for the delay in getting this posted. I had an unrelated build problem somewhere in jshydra within libadblockplus. Apparently there's an executable that's not being tracked (js.exe) and had gotten out of date (with respect to the syntax being used). Killing the repository entirely fixed the problem, but that took hours to track down, and I only figured it out today. In particular, "rebuild all" did not correct the problem. https://codereview.adblockplus.org/29324576/diff/29324577/README.md File README.md (right): https://codereview.adblockplus.org/29324576/diff/29324577/README.md#newcode24 README.md:24: `C:\Program Files (x86)\Microsoft Visual Studio 12.0\VC`). This comment does not explain things adequately. -- It assumes that users know the difference between VS versions and toolset versions. This is manifestly not obvious, and needs some explanation. -- Are you requiring VS 2013 or just recommending it? -- What has been actually tested? VS 2013 Community, VS 2015 Community, other editions? Given all the difficulty actually making this work, it would be a good idea to designate a reference environment that is known to work. https://codereview.adblockplus.org/29324576/diff/29324577/adblockplus.gyp File adblockplus.gyp (right): https://codereview.adblockplus.org/29324576/diff/29324577/adblockplus.gyp#new... adblockplus.gyp:58: '$(VCInstallDir_120)/atlmfc/include', Nope. Visual Studio 2013 Community does not have any directory "atlmfc".
On 2015/08/26 18:25:36, Eric wrote:
> NOT LGTM.
>
> Still doesn't compile. I downloaded the patch to test it.
>
> A very cursory examination of my installation of VS 2013 Community shows that
> there's no directly named "atlmfc". The environment variable asked for
> designates VS 2013. I just cannot believe that this solution was tested in a
> free-tool environment, with Visual Studio 2012 Express installed and some
later
> free version installed for ATL. If that were the case, such an obvious error
> would have been caught.
Actually I have specially tested it in a free-tool environment, there were
merely MSVS 2012 Express and MSVS 2013 Community. Just wonder what else is not
there, could you send the list of files in your Visual C++ directory, just would
like to compare. Here is the list of my files:
c:\Program Files (x86)\Microsoft Visual Studio 12.0\VC>dir
Volume in drive C has no label.
Volume Serial Number is xxxx-xxxx
Directory of c:\Program Files (x86)\Microsoft Visual Studio 12.0\VC
01.06.2015 04:28 <DIR> .
01.06.2015 04:28 <DIR> ..
01.06.2015 04:20 <DIR> atlmfc
01.06.2015 04:33 <DIR> bin
01.06.2015 04:19 <DIR> crt
01.06.2015 04:25 <DIR> include
01.06.2015 04:25 <DIR> lib
01.06.2015 04:28 <DIR> redist
01.06.2015 04:28 <DIR> Snippets
01.06.2015 04:47 <DIR> UnitTest
01.06.2015 04:33 <DIR> VCAddClass
01.06.2015 04:25 <DIR> VCContextItems
22.07.2013 01:33 160 vcEmptyTestProject.vsz
01.06.2015 04:25 <DIR> VCNewItems
01.06.2015 04:28 <DIR> vcpackages
01.06.2015 04:17 <DIR> VCProjectDefaults
01.06.2015 04:25 <DIR> vcprojectitems
01.06.2015 04:33 <DIR> vcprojects
01.06.2015 04:28 <DIR> VCResourceTemplates
13.08.2013 18:18 1 813 vcvarsall.bat
01.06.2015 04:47 <DIR> VCWizards
2 File(s) 1 973 bytes
19 Dir(s) 24 773 169 152 bytes free
>
> -----------
>
> Also, sorry for the delay in getting this posted. I had an unrelated build
> problem somewhere in jshydra within libadblockplus. Apparently there's an
> executable that's not being tracked (js.exe) and had gotten out of date (with
> respect to the syntax being used). Killing the repository entirely fixed the
> problem, but that took hours to track down, and I only figured it out today.
>
> In particular, "rebuild all" did not correct the problem.
>
> https://codereview.adblockplus.org/29324576/diff/29324577/README.md
> File README.md (right):
>
> https://codereview.adblockplus.org/29324576/diff/29324577/README.md#newcode24
> README.md:24: `C:\Program Files (x86)\Microsoft Visual Studio 12.0\VC`).
It's a well known trouble, I've just created an issue for that
(https://issues.adblockplus.org/ticket/2956) because you are already at least
the third person who is saying it to me.
https://codereview.adblockplus.org/29324576/diff/29324577/README.md
File README.md (right):
https://codereview.adblockplus.org/29324576/diff/29324577/README.md#newcode24
README.md:24: `C:\Program Files (x86)\Microsoft Visual Studio 12.0\VC`).
On 2015/08/26 18:25:35, Eric wrote:
> This comment does not explain things adequately.
>
> -- It assumes that users know the difference between VS versions and toolset
> versions. This is manifestly not obvious, and needs some explanation.
Do you mean that user does not know about absence of ATL in the Express edition
in MSVS2012? I don't think we should explain the difference between MSVS
editions. Let leave for Microsoft.
>
> -- Are you requiring VS 2013 or just recommending it?
There is neither recommendation of MSVS 2013 nor requirement of it. We are
merely kindly added the support of MSVS2013 and explaining the peculiarity how
to get it working.
>
> -- What has been actually tested? VS 2013 Community, VS 2015 Community, other
> editions?
MSVS 2012 Express edition with MSVS 2013 Community edition.
>
> Given all the difficulty actually making this work, it would be a good idea to
> designate a reference environment that is known to work.
Added.
> Just wonder what else is not > there, could you send the list of files in your Visual C++ directory, just would > like to compare. Digging into it, I have VS 2013 Express installed, not Community. I installed it last summer, before VS 2013 Community was available. And frankly, I wasn't completely aware what the difference was, nor did the documentation in the first patch set point out the difference. We need to add a note that neither 2012 nor 2013 Express have the requisite libraries. Not an obvious fact. > Do you mean that user does not know about absence of ATL in the Express edition > in MSVS2012? I don't think we should explain the difference between MSVS > editions. Let leave for Microsoft. That's rather dismissive. Microsoft does not do a particularly good job of making of explaining its own editions, their version numbers, and their features. When the vendor does a bad job, it's up to others (that is, us) to make things clear. What you're saying is that someone who doesn't already know this spend an hour or two figuring it out, rather than simply letting them read something for two minutes written by us. This was a defect in the old documentation, too, which had less-than-satisfactory instructions about how to get ATL out of the DDK. This is the opportunity to do better. ----- Separately, I would rather not add a new recommendation to install an old version of Visual Studio. Does this work with the ATL found in VS 2015 Community? It has an "atlmfc" directory; I just checked. If it does, the name of the environment variable should change.
It's been six days since my last comment on this, which asked a very specific question: does the ATL version in Visual Studio 2015 Community work? Please respond to the question, either to answer it or to say you're going to ignore it. I have not been able to get Visual Studio 2013 Community installed. The installer keeps bailing out with an error. I find it hardly surprising that trying something non-standard doesn't work with Visual Studio, though.
On 2015/09/02 16:12:00, Eric wrote: > It's been six days since my last comment on this, which asked a very specific > question: does the ATL version in Visual Studio 2015 Community work? > > Please respond to the question, either to answer it or to say you're going to > ignore it. > > I have not been able to get Visual Studio 2013 Community installed. The > installer keeps bailing out with an error. I find it hardly surprising that > trying something non-standard doesn't work with Visual Studio, though. I have also troubles, after installing of MSVS2015 Community edition MSVS2013 Express becomes broken, no any kind of resetting helps. If you have them all already installed and working, why have you not checked it yet? In total, I think, we can verify whether it works with MSVS2015 later and make the necessary changes in case. This patch allows to build and develop the project using only freely available tools as well as to build the release, which is basically the main aim of it.
On 2015/09/04 12:19:14, sergei wrote: > I have also troubles, after installing of MSVS2015 Community edition MSVS2013 > Express becomes broken, no any kind of resetting helps. If you have them all > already installed and working, why have you not checked it yet? I do NOT have them all working. I have yet to be able to install VS 2013 Community. I've done enough work on this that I am fairly sure we do not want to recommend VS 2013 at this point. The issue is completely arcane, and while I might yet be able to fix it for myself, I absolutely do not want to put anybody else through this. Here's the very short version. The problem seems to be Windows Update KB2918614, which dates from 2014 Aug 12. It borked something in the cryptography which verifies MSI signatures. There were later fixes on top of this one that evidently did not completely fix the problem. As a result, some packages that ought to verify just fine do not. VS 2013 Community is evidently one of these packages. I think .NET 4.6 may be one as well, which I finally got to install after reverting multiple KB# updates (and then reinstalling them). I did not write down anything at the time, thinking that this would be a one-time thing. And evidently this problem has various system-specifics about it, because not everyone exhibits the symptoms and all the fixes seem work for some and not others. Even if I can get it to work (yet to be proven), we would need to say in the documentation that if you have to install these free tools that you should be prepared for them to cost you a lot of time and effort to get them to work and to provide instructions. There's nothing authoritative from Microsoft about this, but rather a bunch of things people have tried, some of which finally worked. Not a good situation. > we can verify whether it works with MSVS2015 later Apparently not. Figuring out whether it works with VS 2015 Community seems of prime importance at this point. We should test with ATL from VS 2015 Community. If that works, it should be added to the list of tested environments. The name of the variable in gyp should change to reflect that it's not necessarily a particular version tool set. And if we leave VS 2013 versions listed as supported, we need a caveat that their installation may fail badly.
I've created an issue for it, https://issues.adblockplus.org/ticket/3150. In addition, I think MSVS2013 is more relevant now than MSVS2015 because we are updating also libadblockplus (as well as gyp and v8) to work with MSVS2013 and nobody has even mentioned MSVS2015 yet. So, it's likely the developer will have MSVS2013 installed and we will unlikely skip MSVS2013 and jump to MSVS2015 for adblockplusie.
On 2015/10/01 17:08:32, sergei wrote: > I've created an issue for it, https://issues.adblockplus.org/ticket/3150. I just commented and closed the ticket. The ATL in VS 2015 cannot compile with the 11.0 toolset that's shipped with VS 2012. The code uses C++11 language features that are not supported by the 2012 toolset. So that can't possibly work. Figuring this out was about the only non-painful part of this whole process. It took me just a few minutes to see what was happening. > In addition, I think MSVS2013 is more relevant now than MSVS2015 because we are > updating also libadblockplus (as well as gyp and v8) to work with MSVS2013 and > nobody has even mentioned MSVS2015 yet. Well, I've mentioned it before. The real point here is that we are presently locked out the ordinary VS upgrade path, and that's a Bad Thing. We've waited long enough to fix the larger issue that we are now two major versions behind Microsoft's release schedule, and we're stuck there. > So, it's likely the developer will have > MSVS2013 installed and we will unlikely skip MSVS2013 and jump to MSVS2015 for > adblockplusie. Without a paid version of 2012, they'll have to have 2013 installed. It's now a requirement. But as soon as we upgrade to the 2013 toolset, it behooves us to see if the 2015 toolset works. It should, because the problems with gyp were generic C++11 name issues and had nothing specific to do with 2013 vs. 2015. There are a few things in the 2015 toolset I want to use immediately. I do not consider that staying one version behind rather than two versions behind is something that's been decided yet.
After days last week trying to overcome my installation problems, I finally got everything installed: -- VS 2012 Express (did not alter) -- VS 2013 Community -- VS 2015 Community The way I did this was idiosyncratic, certainly not a general solution, and involve some low level hacking that I don't even fully recall, much less feel any need to document. I've verified the following configuration -- toolset from 2012 -- ATL from 2013 -- IDE from 2015 What a mess, but it works. The present change set basically works, but still need helps. Primarily, the documentation is inadequate and needs to be greatly expanded. I've commented about how to do that. There's some legacy junk in the gyp file left over from using the ATL in the DDK (and as I recall, prior to that the SDK). That should be cleaned up at this point, because the present change set adds new junk in analogy to the old junk. We can just get rid of it all at this point. It's all salient to ATL in our build setup. https://codereview.adblockplus.org/29324576/diff/29325089/README.md File README.md (right): https://codereview.adblockplus.org/29324576/diff/29325089/README.md#newcode19 README.md:19: You need Microsoft Visual C++ 2012 and Python 2.7. Make sure that `python.exe` This line of text has become confusing given the facts. First and easiest, split out the Python requirements into their own paragraph. Given how much of a mess it is to set up a proper development environment, it behooves us to explain it in detail and with as little confusion as possible. This means one paragraph on each of the following topics. -- which VC++ toolset we require -- which ATL versions will work -- which version of Visual Studio as an IDE will work Please don't quibble about the need to be verbose here. Just because you already understand it doesn't mean someone coming in cold will. The comment policy in the code is "someone familiar with the code base". That policy can't work in this case, because you can't be familiar with the code base before you can get to compile, and you can't do that before you get a development environment up and running. Microsoft does not have a product named "Microsoft Visual C++ 2012". When you search for that, all you get is the redistributable library packages. It is more accurate to say that you need the "Visual C++ 2012 toolset". To my knowledge, the only standard way to get this toolset installed is to install some version of Visual Studio 2012. We know that any version of VS 2012 satisfies the toolset requirement. For ATL, we can list three sources we know work. -- (free) VS 2013 Community -- (paid) VS 2012 Professional and Ultimate I presume that the ATL in paid versions of VS 2013 does work, but we shouldn't list it until after we've tested it. We should also mention here that the ATL in VS 2015 Community does _not_ work. For Visual Studio as an IDE, it seems that any version 2012, 2013, 2015 works. I have tested the following configuration. -- toolset from VS 2012 Express -- ATL from VS 2013 Community -- IDE from VS 2015 Community https://codereview.adblockplus.org/29324576/diff/29325089/README.md#newcode38 README.md:38: both from the same project files. I would add a separate paragraph about the environment variable for VS 2013 used to find ATL here. You don't need the environment variable set when you run 'gyp'. 'gyp' passes through the name of the variable for resolution at build time. The environment variable needs to be set when you invoke the IDE. Stating this fact after talking about 'createsolution' makes this point clear. There's a missing piece in our knowledge about this, I realize. I don't know if you can leave this environment variable undefined if you're using a paid version of VS 2012. I can't test that, because I don't have such a paid version. I presume it gets picked up in the default path. Regardless, this detail is not mentioned in the current text. So this paragraph should say the following: -- either (1a) use VS 2013 Community and (1b) set the environment variable -- or (2) use a paid version of VS 2012 https://codereview.adblockplus.org/29324576/diff/29325089/adblockplus.gyp File adblockplus.gyp (left): https://codereview.adblockplus.org/29324576/diff/29325089/adblockplus.gyp#old... adblockplus.gyp:156: '$(WINDDKDIR)/inc/atl71', The DDK directory illustrates why the SDK paths can go. We were only using the DDK as a (now-obsolete) source of ATL. The SDK, as I recall, was the even-older source, and then Microsoft removed ATL from the SDK and moved it to the DDK. https://codereview.adblockplus.org/29324576/diff/29325089/adblockplus.gyp File adblockplus.gyp (right): https://codereview.adblockplus.org/29324576/diff/29325089/adblockplus.gyp#new... adblockplus.gyp:57: '$(WindowsSDK_IncludePath)', We don't need any of the "WindowsSDK_*" environment variables anywhere in the gyp file. They should all go. It's just path-junk, and it's prudent to remove it so that it can't introduce spurious conflicts. I've tested that you can remove all these lines and get a clean compile. That's only for my own configuration, though. It needs to be tested for a paid VS 2012 configuration. https://codereview.adblockplus.org/29324576/diff/29325089/adblockplus.gyp#new... adblockplus.gyp:106: 'DelayLoadDLLs': ['Shell32.dll'], This line seems to be duplicated.
Updated.
https://codereview.adblockplus.org/29324576/diff/29325089/adblockplus.gyp File adblockplus.gyp (right): https://codereview.adblockplus.org/29324576/diff/29325089/adblockplus.gyp#new... adblockplus.gyp:57: '$(WindowsSDK_IncludePath)', On 2015/10/08 14:36:36, Eric wrote: > We don't need any of the "WindowsSDK_*" environment variables anywhere in the > gyp file. They should all go. It's just path-junk, and it's prudent to remove it > so that it can't introduce spurious conflicts. > > I've tested that you can remove all these lines and get a clean compile. That's > only for my own configuration, though. It needs to be tested for a paid VS 2012 > configuration. Done. https://codereview.adblockplus.org/29324576/diff/29325089/adblockplus.gyp#new... adblockplus.gyp:106: 'DelayLoadDLLs': ['Shell32.dll'], On 2015/10/08 14:36:36, Eric wrote: > This line seems to be duplicated. Done.
I didn't test the build with this patch set, but I don't see anything obviously wrong with it. I'll do a final test with the next patch set. Documentation in this patch is much better than the original. Needs a few tweaks. https://codereview.adblockplus.org/29324576/diff/29329319/README.md File README.md (right): https://codereview.adblockplus.org/29324576/diff/29329319/README.md#newcode15 README.md:15: This section is good. It's adequately descriptive. https://codereview.adblockplus.org/29324576/diff/29329319/README.md#newcode27 README.md:27: `ATLDir` environment variable to the directory of the corresponding ATL (e.g, Nit: As a convention, environment variables are all uppercase. https://codereview.adblockplus.org/29324576/diff/29329319/README.md#newcode40 README.md:40: efforts. Actually, there's zero extra work to use, say, VS 2015 Community. Of the any things I've had trouble with, this is not one of them. The project file generated by gyp identifies the toolset, and Visual Studio uses it seamlessly. There is, however, one caveat. VS 2013 and later will offer the option to "upgrade" the compiler, which will cause the build to fail if selected. In VS 2015, it's in a context menu named "Upgrade VC++ Compilers and Libraries". So don't do that. https://codereview.adblockplus.org/29324576/diff/29329319/README.md#newcode57 README.md:57: Desktop" with "Microsoft Visual Studio Community 2013" as the source of ATL. This item is still a bit confusing. I'd explicitly state that "2012 Express" is where the toolset come from. I would also state explicitly that several different versions of VS-as-IDE work. The way it reads right now seems to contradict the clearer exposition above. https://codereview.adblockplus.org/29324576/diff/29329319/README.md#newcode59 README.md:59: Visual Studio Professional 2012" as the source of ATL. Is there anybody who would use Express if Professional were installed? In other words, I think we can omit this configuration.
Can we get this one completed and pushed? It's been more than two weeks now without any movement, it's almost done, and it's a P1 issue.
On 2015/12/01 20:34:32, Eric wrote: > Can we get this one completed and pushed? It's been more than two weeks now > without any movement, it's almost done, and it's a P1 issue. Please find the new changes. I have not tested it yet, because it's quite long and complicated process, but the changes in gyp-file are merely the renaming of the environment variable.
The README looks fine now. I'll run a test and make sure everything builds. https://codereview.adblockplus.org/29324576/diff/29331721/README.md File README.md (right): https://codereview.adblockplus.org/29324576/diff/29331721/README.md#newcode27 README.md:27: `ADBLOCKPLUS_ATL` environment variable to the directory of the corresponding That's a much better environment variable name. Won't collide with anything else. https://codereview.adblockplus.org/29324576/diff/29331721/README.md#newcode41 README.md:41: caveat is to disable "Upgrade C++ Compilers and Libraries". Strictly speaking, you can't disable the upgrade (or if there is I can't find it). What you do is simply avoid upgrading them. It's a minor point, and it's accurate enough for the time being. https://codereview.adblockplus.org/29324576/diff/29331721/README.md#newcode59 README.md:59: described in "ATL versions" section. Good. It matters here and not the ones below.
Cleaned and rebuilt the plugin, the engine, the shared library, and their unit tests. Everything compiles fine. All the unit tests pass. Sanity check on BHO in IE looks OK. LGTM
https://codereview.adblockplus.org/29324576/diff/29331721/README.md File README.md (right): https://codereview.adblockplus.org/29324576/diff/29331721/README.md#newcode11 README.md:11: You need to have installed python 2+ (tested with python 2.7). It should be Since we are going with this verbose documentation we should also state that we don't support python 3.
https://codereview.adblockplus.org/29324576/diff/29331721/README.md File README.md (right): https://codereview.adblockplus.org/29324576/diff/29331721/README.md#newcode11 README.md:11: You need to have installed python 2+ (tested with python 2.7). It should be On 2015/12/02 20:24:28, Oleksandr wrote: > Since we are going with this verbose documentation we should also state that we > don't support python 3. It seems we are say exactly python 2.7 in other "readme" files, I've changed it to that variant.
LGTM again.
LGTM on new patch. Just so there's no question at all. 
|
