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

Move C_VISIBILITY out of CPPFLAGS #523

Open
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
4 participants
@jeroen
Contributor

jeroen commented Dec 1, 2018

as requested by CRAN.

Move C_VISIBILITY out of CPPFLAGS
as requested by CRAN.
@jennybc

This comment has been minimized.

Member

jennybc commented Dec 12, 2018

Thanks!

How do I reconcile your resolution, which now has:

PKG_CXXFLAGS = $(C_VISIBILITY)

with what Prof Ripley says:

Visibility flags are compiler flags not preprocessor flags, so
$(C_VISIBILITY) should rather be in PKG_CFLAGS . (And C_VISIBILITY is
not appropriate for the C++ compiler ...).

?

I'd like to keep the flag, in whatever forms are most appropriate because, for example, another package could conceivably bundle RapidXML.

What's the rationale for PKG_CXXFLAGS = $(C_VISIBILITY)?

@wch

This comment has been minimized.

Member

wch commented Dec 14, 2018

It looks like R 3.5.2 adds a CXX_VISIBILITY flag for the C++ compiler.

https://cran.r-project.org/doc/manuals/r-devel/NEWS.html

I think that means it should be PKG_CXXFLAGS = $(CXX_VISIBILITY). Does that sound right? We need to do the same thing for httpuv.

@jeroen

This comment has been minimized.

Contributor

jeroen commented Dec 14, 2018

Ah okay now BDR's comment makes sense. However CXX_VISIBILITY is not available for old R versions so there is no easy way to use this....

@wch

This comment has been minimized.

Member

wch commented Dec 14, 2018

Will it cause problems to try to use CXX_VISIBILITY in an older R version? I'm using R 3.5.1, and I have PKG_CXXFLAGS = $(CXX_VISIBILITY), and httpuv compiles and runs just fine.

@jeroen

This comment has been minimized.

Contributor

jeroen commented Dec 14, 2018

It won't cause problems because it doesn't do anything. So you're also not protected form symbol collision problems.

@jcheng5

This comment has been minimized.

jcheng5 commented Dec 14, 2018

FWIW, @wch and I noticed that R on Mac (using Simon's distribution) C_VISIBILITY is not used (despite clang supporting -fvisibility).

$ grep C_VISIBILITY /Library/Frameworks/R.framework/Resources/etc/Makeconf
C_VISIBILITY =
@jeroen

This comment has been minimized.

Contributor

jeroen commented Dec 14, 2018

So far we have only seen symbol collision issues with rstudio-server on CentOS and Fedora systems. Never on Ubuntu or MacOS or Windows. Here are the relevant issues:

In both cases the packages were causing random crashes in rstudio-server that were fixed by making internal symbols in these packages invisible.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment