add missing export variable

[johnsonshih]

Add CLASS_LOADER_PUBLIC to ClassLoader::has_unmananged_instance_been_created_ or it won't build on Windows

[mikaelarguedas]

@johnsonshih do you have an example where this is needed? or a reproducible example of this not building ?

I'm not fully familiar with visibility macros for Windows but it seems surprising to me that private attributes need to be declared public.

The code on the ros2 branch (that has the same set of visibility macros) seems to build fine on Windows 10 (VS2015 and VS2017).

[johnsonshih]

The build will fail when building shared_ptr_test.cpp with unresolved external symbol class_loader::ClassLoader::has_unmananged_instance_been_created_ if we don't add the declaration. The name of CLASS_LOADER_PUBLIC macro is misleading, it's not to declare a entity public but specify the storage-class attribute when it's built on Windows (or visibility on non-Windows environment). The storage-class attribute (dllexport or dllimport)is used to tell the compiler if the current built component need to provide the entity instance. If the current built dll provide the entity, need to set to dllexport or nothing. If the current built dll consumes the entity, the attribute need to set to dllimport. has_unmananged_instance_been_created_ is a static value, and is provided by Class_Loader.dll, so we need to use CLASS_LOADER_PUBLIC to specify the attribute so compiler know how to handle this value.

[mikaelarguedas]

@johnsonshih I would expect it to fall back to SHARED if BUILD_SHARED_LIBS is not specified. Did you experience a different behavior on Windows?

[johnsonshih]

On Windows, class_loader doesn't honor BUILD_SHARED_LIBS and is always built as DLL (it's specified in around line#69, defined CLASS_LOADER_BUILDING_DLL to build as DLL).
I added this line to match the settings so it won't cause problem if BUILD_SHARED_LIBS is Off.

if(WIN32)

  # Causes the visibility macros to use dllexport rather than dllimport

  # which is appropriate when building the dll but not consuming it.

  target_compile_definitions(${PROJECT_NAME} PRIVATE "CLASS_LOADER_BUILDING_DLL")

endif()

[mikaelarguedas]

thanks @johnsonshih for the additional details.
Would it make sense to set the definition CLASS_LOADER_BUILDING_DLL only if BUILD_SHARED_LIBS is ON rather than ignoring BUILD_SHARED_LIBS and forcing it to always be shared on Windows ?

[johnsonshih]

I'm not sure if that's a good idea. The concern is the static data in class_loader, class_loader is a low level component used by a lot of components. If class_loader is a static library, there will be multiple instances of class_loader static data within one process (that should be only one per process) that might cause unexpected side effect. I will suggest keep it as is.

[mikaelarguedas]

thanks @johnsonshih for the additional context and the patience.

The patch looks good to me.

Notes to self/maintainers:

• It would be good to test that the current logic works on Linux whhen building statically but it's safer to change the behavior only for windows for now 👍
• Some of these changes may benefit from being ported to the ros2 branch as well

@nuclearsandwich FYI

[kejxu]

hey @mikaelarguedas and maintainers, just a friendly ping, could this change be merged into the mainline?

[mikaelarguedas]

Thanks James for the reminder Friendly ping @nuclearsandwich that is currently maintaining this package

…

On Fri, Jan 18, 2019, 13:37 James Xu ***@***.*** wrote: hey @mikaelarguedas <https://github.com/mikaelarguedas> and maintainers, just a friendly ping, could this change be merged into the mainline? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#102 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AGVh8DFV569EdOXiWH0h4P1rFAqXznSVks5vETNLgaJpZM4VppUE> .

[nuclearsandwich]

Just waiting on the big pre-release job to finish running on this job before merging.

[kejxu]

hi @nuclearsandwich, just a friendly ping so this pull request won't be lost in the mail box

[nuclearsandwich]

hi @nuclearsandwich, just a friendly ping so this pull request won't be lost in the mail box

Thanks @kejxu. As an update, the prerelease job is not behaving stably. Which is why these PRs are still in flight. I don't think any one of them is the cause, but I am still trying to get the pre-release workspace to build, let alone run tests.

[kejxu]

get the pre-release workspace to build, let alone run tests

My apologies for the spam, I thought the pre-release check was referring to the CI checks. Please take your time with the build =)

[nuclearsandwich]

My apologies for the spam, I thought the pre-release check was referring to the CI checks. Please take your time with the build =)

No worries. You had no way to know that work was still in progress as I hadn't written any update. CI can tell us that class_loader builds and passes all its own tests but until a release is actually made we won't know whether the changes break any downstream packages on the farm (at which point it is not quite too late). So the ros_buildfarm library supports a prerelease script which you can generate from the prerelease website which will build your packages from a target branch, then build and test the packages dependencies from source on top of that.

It functions very similarly to ci.ros2.org in that regard but the builds are run on your local machine.

[seanyen]

@nuclearsandwich ping! Just in case this got lost in your backlogs. 😃