ros2 / rosidl_typesupport

Packages which provide the typesupport for ROS messages and services
Apache License 2.0
13 stars 34 forks source link

Disable access to environment variables for UWP and Android #31

Closed esteve closed 3 years ago

esteve commented 5 years ago

Neither Android or UWP apps can access environment variables, this PR just disables that logic if building for either platform.

Connects to ros2/rcl#280

wjwwood commented 5 years ago

I'm pretty sure this will silently prevent some things from working when built with multiple rmw implementations, so perhaps that should be prevented from happening at the cmake level?

If it works for your use case, then I'd be ok with these conditional disables.

Theosakamg commented 5 years ago

@esteve Android can access to environment variable (in a same process) https://stackoverflow.com/questions/318239/how-do-i-set-environment-variables-from-java/22315463#22315463 since API 21 : https://developer.android.com/reference/android/system/Os.html#setenv(java.lang.String,%20java.lang.String,%20boolean)

How to switch of RMW without RMW_IMPLEMENTATION ? Actually compatible with Android :

esteve commented 5 years ago

@wjwwood

I'm pretty sure this will silently prevent some things from working when built with multiple rmw implementations, so perhaps that should be prevented from happening at the cmake level?

That's true, but at the same time Poco::LoadLibrary and class_loader don't work on UWP anyway because loading arbitrary libraries at runtime is restricted:

https://msdn.microsoft.com/en-us/library/ms684175(v=VS.85).aspx

Moreover, given the space constraints of devices that Android and UWP run on, I'd argue that apps built on top of ROS2 will only ship with one RMW implementation, picked at compile time.

If it works for your use case, then I'd be ok with these conditional disables.

It does, otherwise MSVC just refuses to compile this because _dupenv_s is not compatible with WinRT/UWP:

https://docs.microsoft.com/en-us/cpp/c-runtime-library/reference/dupenv-s-wdupenv-s?view=vs-2017

clalancette commented 3 years ago

@esteve This is pretty old now, and needs to be rebased. Are you still interested in moving this forward?

clalancette commented 3 years ago

I'm going to close this out for now, but please feel free to reopen if you'd like to move this forward.

esteve commented 3 years ago

@clalancette yeah sorry, it's ok to close it, I don't think I'll have time to work on this soon, but it's still an issue when crosscompiling for Android and UWP. I'll submit an updated PR when I have time. Thanks.