Skip to content
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

Specify explicitly only the needed symbols to export from the DLL. #616

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

kalibera
Copy link

Specify explicitly only the needed symbols to export from the DLL. This is safer wrt to possible naming conflicts and fixes linking on Windows using gcc 13.2, which produces weak symbols, which in turn cannot be exported from a DLL.

Also fixed R_init_rstanarm to be found by R.

@jgabry
Copy link
Member

jgabry commented Feb 22, 2024

Thanks for the PR. Gonna tag @bgoodri to take a look since he knows a lot more about this topic than I do.

@jgabry jgabry requested a review from bgoodri February 22, 2024 18:55
@bgoodri
Copy link
Contributor

bgoodri commented Feb 22, 2024

This is going to be fine. We need to test whether it continues to work on older Windows releases of R and if not, bump the minimum R version up to whatever it works with. The larger question is whether we also want to do this in rstantools after the next R release in April?

@jgabry
Copy link
Member

jgabry commented Feb 22, 2024

This is going to be fine. We need to test whether it continues to work on older Windows releases of R and if not, bump the minimum R version up to whatever it works with. The larger question is whether we also want to do this in rstantools after the next R release in April?

Tagging @andrjohns to see if he has a preference about rstantools

@kalibera
Copy link
Author

A bit orthogonal, if this works fine for you, it might be worth generating the .def file automatically - so that when someone adds a new model, the .def file doesn't have to be manually updated.

@bgoodri
Copy link
Contributor

bgoodri commented Feb 22, 2024

Yes, if we enabled LTO for many, many other packages via rstantools, then we would generate the package's .def file when the configure.win script is called (which in turn calls rstantools::rstan_config).

The PR seems to fail to load rstanarm with the message:

Error: package or namespace load failed for ‘rstanarm’ in .doLoadActions(where, attach):
 error in load action .__A__.1 for package rstanarm: (function (ns) : object 'm' not found

This error comes comes from
https://github.com/stan-dev/rstanarm/blob/master/R/zzz.R#L20
Do we not need to call Rcpp::loadModule anymore?

…is is

safer wrt to possible naming conflicts and fixes linking on Windows using
gcc 13.2, which produces weak symbols, which in turn cannot be exported from
the DLL.
@kalibera
Copy link
Author

Hmm, sorry, lets separate the two things. I've updated the pull request to just fix linking on Windows to export only selected symbols. I've removed my attempt to fix the registration from the patch, which is unrelated and will need further investigation.

@kalibera
Copy link
Author

The PR seems to fail to load rstanarm with the message:

Error: package or namespace load failed for ‘rstanarm’ in .doLoadActions(where, attach):
 error in load action .__A__.1 for package rstanarm: (function (ns) : object 'm' not found

This error comes comes from https://github.com/stan-dev/rstanarm/blob/master/R/zzz.R#L20 Do we not need to call Rcpp::loadModule anymore?

I don't know. The problem is non-deterministic on my system, sometimes it happens and sometimes not.

@bgoodri
Copy link
Contributor

bgoodri commented Feb 28, 2024

OK, this is weird. It seems that we now have the m not found problem from .onLoad even if I remove the LTO stuff from src/Makevars.win; see here. But it is not happening with the CRAN rstanarm when it is tested with r-devel.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants