-
Notifications
You must be signed in to change notification settings - Fork 97
ESD-2176 windows dll update #1069
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
f8adc18 to
4a354bd
Compare
| #include <cassert> | ||
| #include <array> | ||
| #include <cassert> | ||
| #include <cstddef> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added this in since down below we used size_t which isn't guaranteed to be imported by any of the above header files (based on the documentations)
| gcc6-release: | ||
| name: GCC6 - Release | ||
| ubuntu-lts: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've applied the DRY principle here and removed the gcc6-release and clang6-* jobs and replaced them with "strategy matrix". This matrix will create 7 jobs (would have been 8 but I've excluded 1 since its already being build by the Code Coverage jobs above). I've also applied the same idea with Mingw and VS 2019.
I also made sure that the updated build now follow the Configure/Build/Test/Install workflow.
Also as part of this change, I've also fixed up the fact that gcc6-release job wasn't actually building for GCC 6, it was actually using the system version which was GCC 7.
fc3af79 to
d3ac6a9
Compare
d3ac6a9 to
4df1147
Compare
c/src/CMakeLists.txt
Outdated
| set_target_properties(sbp PROPERTIES | ||
| POSITION_INDEPENDENT_CODE ON) | ||
| generate_export_header(sbp | ||
| DEPRECATED_MACRO_NAME CMAKE_SBP_DEPRECATED |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these *DEPRECATED_MACRO_NAME entries needed to be added because the generate_export_header otherwise defines SBP_DEPRECATED which conflicts with the SBP_DEPRECATED definition in common.h.
Also the INCLUDE_GUARD_NAME is to make sure that it complies with the Google C++ Style Guide
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't it be simpler to just add SBP_EXPORT to common.h along with all the other compiler macros? That's the way libswiftnav handles this same problem. Then all the compiler specific stuff can be in one place and we don't have to deal with duplicate symbols
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
generate_export_header manages things across all compilers nicely for you without any extra work and or knowledge, the naming clash is unfortunate for SBP_DEPRECATED, but not a major issue. Also I don't know if the feature even works on libswiftnav since SWIFT_DECLSPEC is only being used by two methods and no CI stage to build shared libraries on Windows OS.
The use of this is also advised by the cmake maintainer as he said that people have tried to write this on their own for all platforms, but often have issues so it is better to stick with this approach.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I understand, but we have to deal with this issue anyway since generate_export_header is very limited in scope - we also have to have macros for packed, format, etc. From a maintainability perspective I think it's better to have all of this sort of stuff in the same place rather than having 2 different mechanism for defining compiler attribute macros.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Made the changes as requested, will also see if I can add a test to catch the installation issue you posted below.
| ) | ||
| set_target_properties(sbp | ||
| PROPERTIES | ||
| VISIBILITY_INLINES_HIDDEN true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these options make it so that all symbols on Linux OS are private, in the same way they are on Windows Visual studio. This is done to:
- minimize the shared library size by not exposing all symbols
- making it easier to pick up error on Linux/Mac machines that would appear on Windows
…ft-nav/libsbp into rodrigor/ESD-2176-windows-dll-update
| @@ -1,4 +1,4 @@ | |||
| cmake_minimum_required(VERSION 3.2) | |||
| cmake_minimum_required(VERSION 3.12) | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needed to up this number because here you can see that this feature only worked for C++ until until 3.12 when C was also introduce.
8701149 to
2040be1
Compare
2040be1 to
6f0ac79
Compare
c/include/libsbp/edc.h
Outdated
|
|
||
| #include <libsbp/common.h> | ||
|
|
||
| #include "sbp_export.h" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just had a little go and this doesn't work when libsbp is installed to the filesystem:
> cat test.c
#include <libsbp/sbp.h>
int main(void) { return 0; }
> cc test.c -o test -Iinclude -Llib -lsbp
In file included from include/libsbp/sbp.h:17,
from test.c:1:
include/libsbp/legacy/api.h:18:10: fatal error: sbp_export.h: No such file or directory
18 | #include "sbp_export.h"
| ^~~~~~~~~~~~~~
compilation terminated.
> ls include/libsbp
acquisition.h file_io_macros.h logging.h orientation.h settings_macros.h user_macros.h
acquisition_macros.h flash.h logging_macros.h orientation_macros.h solution_meta.h v4
bootload.h flash_macros.h mag.h piksi.h solution_meta_macros.h vehicle.h
bootload_macros.h gnss.h mag_macros.h piksi_macros.h ssr.h vehicle_macros.h
common.h gnss_macros.h navigation.h sbas.h ssr_macros.h version.h
cpp imu.h navigation_macros.h sbas_macros.h system.h
edc.h imu_macros.h ndb.h sbp_export.h system_macros.h
ext_events.h legacy ndb_macros.h sbp.h tracking.h
ext_events_macros.h linux.h observation.h sbp_msg_type.h tracking_macros.h
file_io.h linux_macros.h observation_macros.h settings.h user.h
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've since removed the use of sbp_export.h and placed everything in common.h. I've also added a new stage to all the builds which will consume the installed SBP library to catch any issues in the future that might arise from this.
woodfell
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Changes
I've done a number of changes with this PR, the primary one being that we now we are able to generate Windows share library files (*.dll) which can be consumed and contains all publicly available functions.
What was wrong to begin with and why?
Visual Studio unlike GCC and Clang have different philosophies when building shared libraries. GCC and Clang generate symbol information for all functions that are defined in their source files whereas Visual Studio by default does not expose any symbol unless told otherwise through Visual Studio's variable/function/class annotation
__declspec(dllexport). So the major portion of the work done here is to annotate each public libsbp header file to expose the function. The way I've approach this was to follow what was advised here by Craig Scott. Note that I could have gone with forcing Visual Studio to expose all symbols like GCC and Clang, but that isn't the preferred way (you can see how we've also applied this to libsettings). The two major benefits with doing it this way is:We've never had an issue with building libsbp static libraries for any of our platforms because unlike shared libraries, static libraries will expose all its symbols and let the linker strip off any symbols it does not need during the linking phase.
When to add
SBP_EXPORT?Well for one, any basic C/C++ global function should have
SBP_EXPORTadded at the front. I say "basic" because if a function is declared withstatic, we don't need to addSBP_EXPORTand the reason for that is that once the header file is included in a source file, thestatictells the compiler that the function will be accessible only to the translation unit.Additional Changes
While making these changes, I've also had a go at trying to clean up the CI a bit as well as place some build stages which will validate that dll files are now properly working. Building the libsbp unit tests using the shared libraries libraries does validate that its working. I've also gone ahead and added in an additional stage to install the project to make sure that it doesn't break in the future.
Limitations
Trying to run unit test on Windows with shared libraries doesn't work, if I were to install the binaries (including the unit tests) and than running them, it works. It appears that in Windows (unlike in Linux and Macs), the binaries RPATH isn't correctly managed by cmake. As such you might notice that I've update the CI to only build the test binaries for shared libraries but not run them (they are run when building static libraries).
Testing
On Windows I installed the libsbp library and created a seperate cmake project to consume it. I've written a simple C/C++ program that uses the shared library and it now works.