search

grame-cncm / libmusicxml

A C/C++ library to support the MusicXML format.
Mozilla Public License 2.0
152 stars 33 forks source link

Visibility issue in lilypond/utilities.h #32

Closed school510587 closed 5 years ago

school510587 commented 5 years ago

I failed to build xml2ly.exe, because the linker could not find std::ostream& operator<< (std::ostream&, const indenter&). This is an issue of visibility. Could you add EXP before its declaration in utilities.h? Also, std::ostream& operator<< (std::ostream&, const timing&) is not declared EXP. If this operator is sure to be part of the API, please add EXP, too. Thanks.

dfober commented 5 years ago

could you give more details? the target platform, your compiler. I’m just a bit surprised because compilation is automatically checked with travis on linux and appveyor on windows.

Le 30 déc. 2018 à 20:23, school510587 notifications@github.com a écrit :

I failed to build xml2ly.exe, because the linker could not find std::ostream& operator<< (std::ostream&, const indenter&). This is an issue of visibility. Could you add EXP before its declaration in utilities.h? Also, std::ostream& operator<< (std::ostream&, const timing&) is not declared EXP. If this operator is sure to be part of the API, please add EXP, too. Thanks.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/grame-cncm/libmusicxml/issues/32, or mute the thread https://github.com/notifications/unsubscribe-auth/AAaTwzAV7XO_EZ3WOLYECcwPKsQ-xdf8ks5u-RKYgaJpZM4ZlC5h.

school510587 commented 5 years ago

I tried two situations to compile your commit 52654. 1) On Windows, MinGW G++ i686-8.1.0-posix-dwarf-rt_v6-rev0 This resulted in a linker error described above. 2) On Ubuntu, G++ version 7.3.0 (Ubuntu 7.3.0-27ubuntu1~18.04) Compilation completed. However, some warnings were shown, e.g., xmlpart2guido.cpp has an unused variable wedgeStart.

school510587 commented 5 years ago

In the 2nd case, I can reproduce the linker error by -DCMAKE_CXX_FLAGS:str="-fvisibility=hidden" when calling cmake. My suggestion is to check flags -fvisibility=hidden and -fvisibility-inlines-hidden, but I don't know their equivalent in MSVC. Thanks.

jacques-menu commented 5 years ago

Hello school510587,

I’ve added EXP to all ‘operator<<‘ occurrences in src/lilypond/*.h, and pushed that to the lilypond branch (that’s where I contribute my work, Dom does the merging with the dev branch) some days ago.

JM

Le 3 janv. 2019 à 18:43, school510587 notifications@github.com a écrit :

In the 2nd case, I can reproduce the linker error by -DCMAKE_CXX_FLAGS:str="-fvisibility=hidden" when calling cmake. My suggestion is to check flags -fvisibility=hidden and -fvisibility-inlines-hidden, but I don't know their equivalent in MSVC. Thanks. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/grame-cncm/libmusicxml/issues/32#issuecomment-451220630, or mute the thread https://github.com/notifications/unsubscribe-auth/AGKb-r6rMxiXUxBKLpcXnj_ja1jr_ELlks5u_kE0gaJpZM4ZlC5h.

school510587 commented 5 years ago

Hi @dfober and @jacques-menu,

I checked out lilypond branch, and compilation completed with -fvisibility=hidden. Before closing this issue, could you add such lines into CMakeLists.txt:

check_cxx_compiler_flag (-fvisibility=hidden FVISIBILITY_HIDDEN) if(${FVISIBILITY_HIDDEN}) set(CMAKE_CXX_FLAGS "-fvisibility=hidden ${CMAKE_CXX_FLAGS}") endif()

Note that it is not inside any other if-else-endif block, because it does not depend on the OS. I remember @dfober asked me to trace the latest version from dev branch. So, if you don't feel trouble, please merge commit 83bb34 and the above code into dev branch. Thanks!

dfober commented 5 years ago

Sorry, I'm really busy up to the end of the month and I prefer to carefully check on all platforms (and especially with MSVC). I'll do that in February.

dfober commented 5 years ago

Found 5 minutes to look at the issue... There is no need to change visibility at global level. The problem should be fixed with the export (EXP) declarations mentionned by Jacques. It works on the dev branch with msys makefiles. Could you check on your side?

school510587 commented 5 years ago

Hi @dfober,

Thanks for quick reply. I have completed compilation of commit 131ceb locally. Sorry, I generated MinGW makefiles by cmake build -G "MinGW Makefiles" and ran it directly by mingw32-make, because I was not familiar to MSYS environment. It seems MinGW g++ treats all declarations without explicit visibility specification as visibility=hidden. So, I think that setting -fvisibility=hidden will cause compilation failure if other EXP declaration is forgotten at text time. However, it's also ok if you think changing visibility in CMakeLists.txt is not necessary.

dfober commented 5 years ago

Exporting explicitly the functions that need to be exported is also a way to ensure a correct interface declaration. In addition, the -fvisibility=hidden is compiler dependant and will not work with MSVC. Thus if the current solution works, we'll leave the CMakelist.txt file as is.

school510587 commented 5 years ago

Closed because 131ce fixed this problem.