Make compilation work on MSVC 2017
Concern Raised44f846f13d0f

Authored by wojnilowicz on Jan 26 2018, 5:24 PM.

Description

Make compilation work on MSVC 2017

Differential Revision: https://phabricator.kde.org/D10125

Details

Auditors
christiand
Committed
wojnilowiczJan 28 2018, 10:25 AM
Differential Revision
D10139: Update KMyMoney blueprint
Parents
R261:a0728e388133: Update credits
Branches
Unknown
Tags
Unknown
christiand raised a concern with this commit.Jan 30 2018, 6:17 PM
christiand added a subscriber: christiand.

The first two notes are not so important but the corresponding code should get a /**@todo */ comment.

/CMakeLists.txt
55

If this is required, it is a upstream bug or some target is not linking to the alkimia target.

130

This is a bug of FindLibOfx.cmake and should be fixed there. CMake policy is to use absolute paths only.

/kmymoney/plugins/onlinetasks/sepa/tasks/sepaonlinetransferimpl.cpp
182

Isn't this changing the behavior heavily and breaking the hole function?

This commit now has outstanding concerns.Jan 30 2018, 6:17 PM
wojnilowicz added inline comments.Jan 30 2018, 6:23 PM
/CMakeLists.txt
55

I don't know how to check this.

130

Right. I didn't know that.

/kmymoney/plugins/onlinetasks/sepa/tasks/sepaonlinetransferimpl.cpp
182

That's true. It breaks this function badly, but I don't know if it wasn't broken before. Someone with knowledge should revise that and prove that this plugin works at all.

christiand added inline comments.Jan 30 2018, 6:44 PM
/CMakeLists.txt
55

Then just add a comment, that this is a work around and has to be fixed later.

130

Oh, this is more complicated than I thought. What about using:

set(CMAKE_REQUIRED_INCLUDES "${LIBOFX_INCLUDE_DIR}")
check_struct_has_member("struct OfxFiLogin" "clientuid" "libofx/libofx.h" LIBOFX_HAVE_CLIENTUID)
unset(CMAKE_REQUIRED_INCLUDES)

This does not require compiler distinctions and is shorter. Since we have FindLibOfx.cmake under our control this code can go there (then we do not worry about setting and unsetting CMAKE_REQUIRED_INCLUDES). However, this is not required (and the lientuid var has to be marked as cached).

Btw: CMake docu for CMAKE_REQUIRED_INCLUDES in this context is here: https://cmake.org/cmake/help/latest/module/CheckStructHasMember.html

/kmymoney/plugins/onlinetasks/sepa/tasks/sepaonlinetransferimpl.cpp
182

Can you send me the MSVC error this line causes? Then I can try to handle that.

wojnilowicz added inline comments.Jan 31 2018, 2:12 PM
/CMakeLists.txt
130

As I remember correctly, I tried with CMAKE_REQUIRED_INCLUDES and failed. If it works for you on MS Windows, then go for it.
If you checked it only on Linux then please don't touch it.

mhubner added a subscriber: mhubner.Feb 7 2018, 1:02 AM
mhubner added inline comments.
/kmymoney/plugins/onlinetasks/sepa/tasks/sepaonlinetransferimpl.cpp
182

Hey Łukasz and Christian,

it's been a while, but I think I ran into that issue before. It is somehow related to which methods MSVC exports into a dll. I somewhere found the info this might be related to the order libraries are linked, but couldn't really get it to work. I didn't bother figuring out the details back then because I wasn't patient enough, but "fixed" it rather brutally by patching mymoneyaccount.h in craft as follows. That way I was able to successfully compile (although, if I remember correctly, the result looked terrible because all icons were missing).

diff --git a/kmymoney/mymoney/mymoneyaccount.h b/kmymoney/mymoney/mymoneyaccount.h
index 78739385..408261d3 100644
--- a/kmymoney/mymoney/mymoneyaccount.h
+++ b/kmymoney/mymoney/mymoneyaccount.h
@@ -36,6 +36,8 @@
 #include "kmm_mymoney_export.h"
 #include "mymoneyunittestable.h"
 
+#define EXPORT __declspec(dllexport)
+
 class MyMoneySplit;
 class payeeIdentifier;
 namespace payeeIdentifiers { class ibanBic; }
@@ -334,7 +336,7 @@ public:
    * @see MyMoneyPayeeIdentifierContainer::payeeIdentifiersByType()
    */
   template< class type >
-  QList< ::payeeIdentifierTyped<type> > payeeIdentifiersByType() const;
+  EXPORT QList< ::payeeIdentifierTyped<type> > payeeIdentifiersByType() const;
 
   /**
     * This method is used to set the descriptive text of the account
@@ -784,14 +786,14 @@ public:
 };
 
 template< class type >
-QList< payeeIdentifierTyped<type> > MyMoneyAccount::payeeIdentifiersByType() const
+EXPORT QList< payeeIdentifierTyped<type> > MyMoneyAccount::payeeIdentifiersByType() const
 {
   QList< payeeIdentifierTyped<type> > typedList;
   return typedList;
 }
 
 template<>
-QList< payeeIdentifierTyped< ::payeeIdentifiers::ibanBic> > MyMoneyAccount::payeeIdentifiersByType() const;
+EXPORT QList< payeeIdentifierTyped< ::payeeIdentifiers::ibanBic> > MyMoneyAccount::payeeIdentifiersByType() const;
 
 /**
  * Make it possible to hold @ref MyMoneyAccount objects,

This is certainly not elegant, but maybe it's a good starting point for either of you to dig deeper into it ?

Kind Regards,
Marc

tbaumgart added inline comments.
/kmymoney/plugins/onlinetasks/sepa/tasks/sepaonlinetransferimpl.cpp
182

Don't define EXPORT yourself. Use KMM_MYMONEY_EXPORT which is declared by cmake in kmm_mymoney_export.h and should have the necessary definitions for your build environment.

wojnilowicz added inline comments.Feb 14 2018, 6:23 PM
/kmymoney/plugins/onlinetasks/sepa/tasks/sepaonlinetransferimpl.cpp
182

sepaonlinetransferimpl.cpp.obj:-1: błąd: LNK2019: nierozpoznany zewnętrzny symbol "public: class QList<class payeeIdentifierTyped<class payeeIdentifiers::ibanBic> > cdecl MyMoneyAccount::payeeIdentifiersByType<class payeeIdentifiers::ibanBic>(void)const " (??$payeeIdentifiersByType@VibanBic@payeeIdentifiers@@@MyMoneyAccount@@QEBA?AV?$QList@V?$payeeIdentifierTyped@VibanBic@payeeIdentifiers@@@@@@XZ) przywo│any w funkcji "public: virtual class payeeIdentifier cdecl sepaOnlineTransferImpl::originAccountIdentifier(void)const " (?originAccountIdentifier@sepaOnlineTransferImpl@@UEBA?AVpayeeIdentifier@@XZ)

nierozpoznany zewnętrzny symbol
means
undefined external symbol