Opened 4 years ago

Closed 4 years ago

Last modified 3 years ago

#2940 closed defect (fixed)

Adblock Plus for IE build is broken for Visual Studio Express

Reported by: eric@… Assignee: sergz
Priority: P1 Milestone: Adblock-Plus-for-Internet-Explorer-1.6
Module: Adblock-Plus-for-Internet-Explorer Keywords:
Cc: oleksandr, sergz Blocked By:
Blocking: Platform: Internet Explorer
Ready: no Confidential: no
Tester: Unknown Verified working: no
Review URL(s):

https://codereview.adblockplus.org/29324576/

Description

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

Change History (8)

comment:1 Changed 4 years ago by eric@…

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.

comment:2 Changed 4 years ago by eric@…

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.

Last edited 4 years ago by eric@… (previous) (diff)

comment:3 Changed 4 years ago by eric@…

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.

comment:4 follow-up: Changed 4 years ago by oleksandr

  • Blocked By 1087, 1197 added

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?

Last edited 4 years ago by oleksandr (previous) (diff)

comment:5 in reply to: ↑ 4 Changed 4 years ago by eric@…

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.

comment:6 Changed 4 years ago by oleksandr

  • Owner set to sergz
  • Review URL(s) modified (diff)

comment:7 Changed 4 years ago by sergz

  • Blocked By 1087, 1197 removed
  • Resolution set to fixed
  • Status changed from new to closed

comment:8 Changed 3 years ago by oleksandr

  • Milestone set to Adblock-Plus-for-Internet-Explorer-Next
Note: See TracTickets for help on using tickets.