chili-epfl / chilitags

Robust Fiducial Markers for Augmented Reality And Robotics
http://chili.epfl.ch/software
123 stars 57 forks source link

fix windows dll build #93

Closed hesmar closed 6 years ago

hesmar commented 7 years ago

export Chilitags class to allow usage as windows dll

severin-lemaignan commented 7 years ago

@hesmar I believe you forgot to include chilitags_exports.hpp, haven't you?

qbonnard commented 7 years ago

Thanks @hesmar ! @severin-lemaignan I suppose the generate_export_header does that... I'll check tonight if linux is happy with it. Looks like Travis needs a little update too...

hesmar commented 7 years ago

@severin-lemaignan The header is generated by cmake's generate_export_header macro. See CMake docs.

qbonnard commented 7 years ago

Oooookay, looks like it's time for a long overdue update of various versions... Which means it'll probably take a little more than tonight to merge this PR. Is there an emergency on your side @hesmar ? Also, as always,, I'm curious to know about your project involving chilitags, if you can share some information.

Cheers

qbonnard commented 7 years ago

Hmm, looks like linux ignores the generate_export_header macro... What's in chilitags_exports.hpp anyway ? if it's only a #define CHILITAGS_EXPORT decl_spec or so, I guess we can just copy paste it... Or is it more complicated ? Could you show your chilitags_exports.hpp @hesmar please ?

hesmar commented 7 years ago

It works also on Linux on my machine. What CMake version do you use? I can show the generated file tomorrow.

qbonnard commented 7 years ago

I have cmake versions 3.5.1 and 3.7.2 (the defaults on Ubuntu 16.04 (LTS) respectively 17.04)... what's yours ? What's weird is that it does not complain about the generate_export_header. I haven't found the minimum version for it (I haven't searched very long, admittedly).

hesmar commented 7 years ago

I am also using CMake 3.5.1 on Ubuntu 16.04 and the header is generated in the build directory. I tried it also with CMake 3.8.1 without problems. So, it is strange that it does not work on your machine. Maybe you can check it again? This is the content of the generated file:

#ifndef CHILITAGS_EXPORT_H
#define CHILITAGS_EXPORT_H

#ifdef CHILITAGS_STATIC_DEFINE
#  define CHILITAGS_EXPORT
#  define CHILITAGS_NO_EXPORT
#else
#  ifndef CHILITAGS_EXPORT
#    ifdef chilitags_EXPORTS
        /* We are building this library */
#      define CHILITAGS_EXPORT __attribute__((visibility("default")))
#    else
        /* We are using this library */
#      define CHILITAGS_EXPORT __attribute__((visibility("default")))
#    endif
#  endif

#  ifndef CHILITAGS_NO_EXPORT
#    define CHILITAGS_NO_EXPORT __attribute__((visibility("hidden")))
#  endif
#endif

#ifndef CHILITAGS_DEPRECATED
#  define CHILITAGS_DEPRECATED __attribute__ ((__deprecated__))
#endif

#ifndef CHILITAGS_DEPRECATED_EXPORT
#  define CHILITAGS_DEPRECATED_EXPORT CHILITAGS_EXPORT CHILITAGS_DEPRECATED
#endif

#ifndef CHILITAGS_DEPRECATED_NO_EXPORT
#  define CHILITAGS_DEPRECATED_NO_EXPORT CHILITAGS_NO_EXPORT CHILITAGS_DEPRECATED
#endif

#define DEFINE_NO_DEPRECATED 0
#if DEFINE_NO_DEPRECATED
# define CHILITAGS_NO_DEPRECATED
#endif

#endif
qbonnard commented 7 years ago

Ah right, chilitags_export.hpp is indeed created, but not where it is expected apparently (at the root of the build folder, while the include expects it in the include folder I suppose), hence my error in the build.

Now, I'm not familiar at all with all this, but it all seems a bit overkill... Wouldn't

#ifdef CHILITAGS_STATIC_DEFINE
#  define CHILITAGS_EXPORT
#else
#  ifndef CHILITAGS_EXPORT
#      define CHILITAGS_EXPORT __attribute__((visibility("default")))
#  endif
#endif

instead of #include "chilitags_export.hpp" be enough, since we don't use NO_EXPORT, DEPRECATED and NO_DEPRECATED. Or maybe just #define CHILITAGS_EXPORT __attribute__((visibility("default"))), but I don't know who could set CHILITAGS_STATIC_DEFINE. This way we would be keep a single header, which seems easier to install... Unless I'm missing something ?

Also, shouldn't CHILITAGS_EXPORT appear before

Quad
TagCornerMap

and

Chilitags3D;
Chilitags3Df;
Chilitags3Dd;

TransformMatrix;
TransformMatrixf;
TransformMatrixd;

TagPoseMap;
TagPoseMapf;
TagPoseMapd;

? Again, I'm not familiar with the subject, so it's a genuine question.

hesmar commented 7 years ago

The generated header looks different on Windows. Therefore it is easier to generate it using CMake, otherwise you have to add some compiler switches which makes the code ugly. The compiler has also to distinguish if chilitags is compiled or the target which uses it. Therfore CMake passes the chilitags_EXPORTS define to the preprocessor when compiling chilitags. In Windows, CHILITAGS_EXPORT is set to __declspec(dllexport) when compiling chilitags and to __declspec(dllexport) when using chilitags. Regarding the deprecation macros, I did not find an option to disable them, but I also do not care about them.

We do not have to export the other classes, because they are only typedefs. It is only necessary for classes or functions, where the implementation is hidden (declaration in .h, definition in .cpp).

qbonnard commented 7 years ago

Ah I see... And how much of a bad idea would it be to had some #ifdef WIN32 around the #include, and ignore the chilitags_export.hpp for linux (only defining an empty EXPORT macro) ? If it's too much of a bad idea, I guess I'll stop being annoying and only ask if you could fix the directory issue (chilitags_export.hpp not being in the same directory as chilitags.hpp, resulting in a build error).

Also, shouldn't the Chilitags3D_ class be exported too ? It's the actual class for all the Chilitags3D* typedefs

hesmar commented 7 years ago

You are right about the Chilitags3D_ class. I changed the PR. It should compile now.

qbonnard commented 6 years ago

All green, thanks for your work... and your patience @hesmar !