Opened 5 years ago

Closed 4 years ago

Last modified 3 years ago

#2599 closed defect (fixed)

Adblock Plus for IE build is broken for x64

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

http://codereview.adblockplus.org/4923675673362432/

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. The error goes away if Pro is installed, but Pro is not our designated, completely-no-cost build environment.

How to reproduce

Build the x64 solution in the above environment. The following link error appears:

error LNK2001: unresolved external symbol CComStdCallThunkHelper

First version to exhibit the defect

66c71b83b1057808261031ad628557e92846b9bb
Issue 1672 - Subscribe and listen COM events using ATL::IDispEventImpl and BEGIN_SINK_MAP

Commentary

The reason for the error is an ATL version mismatch. Our designated ATL for building is one from an older version of the Windows Driver Development Kit (WinDDK). This kit does not seem to be available as such any more; the replacement is Windows Driver Kit (WDK). Visual Studio Pro (and above) 2012 come with a _different_ version installed, apparently a slightly later one. Code introduced in the change set referenced above added code that works with the Pro version but not with the WinDDK version.

The problem had been masked because our build system had the Pro version directories ahead of the WinDDK ones, so they took priority. This review changes the priority order, avoiding future masking problems.

http://codereview.adblockplus.org/4923675673362432/

The offending directory is "$(VCInstallDir)atlmfc". If that directory is (temporarily) renamed, it acts as if Pro isn't installed (insofar as this defect is concerned); it does this without changing the build.

Change History (6)

comment:1 Changed 5 years ago by eric@…

The question remains about how to fix the problem with the build. There are a few options.

1) If we can find a distribution of ATL that matches the one in Visual Studio 2012 Pro, that should work. It's not known whether there was ever such a distribution.

2) We can use the ATL distribution in Visual Studio Community 2013. That, however, requires us to use the 2013 tool set, which is not at present compatible with the version of v8 in libadblockplus.

https://issues.adblockplus.org/ticket/1197

There was an engine upgrade, but to a version required by Maxthon, not one that's adequate for VS 2013. Does Maxthon still require that specific version?

This option requires upgrading v8 for all platforms, forking our v8 repository to fix the compile problem (it's simple; just some duplicate definitions for C99 math functions), or forking libadblockplus for products that require a particular, older version.

3) We can revert the change that caused the problem entirely. It was made to simplify code, not add a feature or correct a defect. This is difficult, in part because the offending change is pretty far back in the version history at this point.

4) We can rewrite the change that caused the problem to avoid use of the ATL libraries. This seems feasible, since the main change in the offending change set was to use an ATL implementation class for the interface "IDispatch", which pulls in a bunch of ATL infrastructure. We could go back to using a manually written dispatch function, calling out the separate implementation functions. Splitting out these functions from the old dispatch routine was also part of the change set, but there's no inherent problem with those changes.

comment:2 Changed 5 years ago by oleksandr

  • Blocked By 1197 added

We have decided that it would be best just to upgrade to Visual Studio 2013, where we can use the Community Edition, which has ATL. Since the only problem that is blocking the move is libadblockplus incopatibility - I am adding #1197 as blocker.

comment:3 Changed 4 years ago by oleksandr

  • Blocked By 1087 added

comment:4 Changed 4 years ago by sergz

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

comment:5 Changed 3 years ago by oleksandr

  • Milestone set to Adblock-Plus-for-Internet-Explorer-Next

comment:6 Changed 3 years ago by rraceanu

  • Tester changed from Unknown to Rraceanu
Note: See TracTickets for help on using tickets.