Adblock Plus for IE build is broken for Visual Studio Express

[eric@adblockplus.org]

Environment

This is a build error. Environment is one with Visual Studio Express 2012 installed but _without_ Visual Studio Professional or higher also installed.

How to reproduce

Build the project 'shared' with the above development environment. The following compile error appears

[...]adblockplusie\src\shared\MsHTMLUtils.h(23): fatal error C1083: Cannot open include file: 'atlbase.h': No such file or directory

Version that introduced the defect

f753f2c9de1f1e52fc5468892f3dd53e35d8cac5
Issue 1109 - Support notifications

[eric@adblockplus.org]

This is the second time in three months that we have a build failure that is committed to the public repository that was not caught in advance because it was not tested in the reference development environment, which is the Express edition of VS 2012, the free one, and not one the paid editions.

[eric@adblockplus.org]

Furthermore, this defect was completely avoidable. It arises because a utility function was defined with an argument of type ATL::CComBSTR instead of std::wstring. We have been endeavoring to use the standard library string classes wherever possible.

There's exactly one good use of CComBSTR, and that's RIAA for managing system allocation of BSTR arguments to API functions. The use in the present change set is in the signature of a utility function, which is manifestly not necessary.

[eric@adblockplus.org]

The temptation will be to fix the build error by getting the include paths right in the gyp file. This would be a bad solution. The right solution would be to rewrite the offending function to use std::wstring and not an ATL type.

[oleksandr]

It doesn't look like the description is correct. There is no requirement for Visual Studio Professional to be installed. There is a requirement for a different ATL then the one in the WDK, however. In any case this and #2599 are almost identical, and I really don't think we need both. I guess it makes sense to close #2599, if we have this now. Any objections?

[eric@adblockplus.org]

Replying to oleksandr:

It doesn't look like the description is correct. There is no requirement for Visual Studio Professional to be installed.

The description does not say there is a requirement for it to be installed. All it says is that it doesn't compile in VS Express. Apparently it does compile in VS Pro, since that's what it was developed with.

There is a requirement for a different ATL then the one in the WDK, however.

The present issue is not about ATL versions. The only ATL class in the offending code that's not compiling is CComBSTR which is already used extensively in another project.

The most proximate cause is that the path to the ATL headers are not set up correctly in a project that did not previously require them. I consider it shoddy testing that this change was even committed in the first place.

In any case this and #2599 are almost identical, and I really don't think we need both. I guess it makes sense to close #2599, if we have this now. Any objections?

I completely object. They are different problems. #2599 is actually about ATL versions, because the old version just doesn't work. This one is about a build problem with paths set up incorrectly. The commonality is that code is getting committed without checking to see if it builds correctly in a reference build environment, but that's not a technical issue.

These two problems might have the same solution. But the previous one has languished for three months with no technical fixes. The one "fix" was to change the compile requirement to use non-free tools. I don't anticipate that such a solution is imminent at this point, so these should remain separate.