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

Add RcppParallel linker flags for StanHeaders 2.26 Compatibility #5

Merged
merged 2 commits into from Feb 9, 2023
Merged

Add RcppParallel linker flags for StanHeaders 2.26 Compatibility #5

merged 2 commits into from Feb 9, 2023

Conversation

andrjohns
Copy link
Contributor

This PR updates your package to additionally link against RcppParallel, which will be needed for the next version of StanHeaders

@mattfidler mattfidler merged commit 008b69d into nlmixr2:main Feb 9, 2023
@mattfidler
Copy link
Contributor

Thanks

@bgoodri
Copy link

bgoodri commented Mar 10, 2023

Same deal as with nlmixr2est.

@mattfidler
Copy link
Contributor

Same as rxode2parse

@mattfidler
Copy link
Contributor

@bgoodri @andrjohns

Cran wanted cpp17 instead of cpp14. With the flags you supplied and cpp17 I have the following issue:

Dear maintainer,

Please see the problems shown on
https://cran.r-project.org/web/checks/check_results_rxode2ll.html.

Please correct before 2023-03-31 to safely retain your package on CRAN.

The CRAN Team

Does this need cpp14?

@mattfidler
Copy link
Contributor

I can revert everything but I sm sure that doesn't help you.

@bgoodri
Copy link

bgoodri commented Mar 17, 2023

Don't revert; we'll figure it out. Does nlmixr2 compile with the C++14 standard (but continuing to link to TBB)?

@andrjohns
Copy link
Contributor Author

I believe you just need to replace -DBOOST_NO_AUTO_PTR with -D_HAS_AUTO_PTR_ETC=0 in your Makevars files

@mattfidler
Copy link
Contributor

Thanks @andrjohns

I will try it out and resubmit to CRAN.

@bgoodri it linked without linking to the tbb threads library in the past, so I'm unsure. I use the backward AD engine only if that helps answer your question.

I separated out the stan pieces from nlmixr2est proper because armadillo and eigen were having problems. This should be fine.

mattfidler added a commit that referenced this pull request Mar 17, 2023
@bgoodri
Copy link

bgoodri commented Mar 17, 2023

With the current StanHeaders / rstan on CRAN, linking to TBB was somewhat optional and it would work either way if TBB was not utilized. With the next StanHeaders / rstan, linking to TBB is mandatory and there will be a linker error otherwise. So, the change to the linker flags is necessary for the next StanHeaders to have a smooth(er) path to CRAN.

The stuff about the C++ standard and -D_HAS_AUTO_PTR_ETC=0 is separate from TBB considerations and is mostly due to CRAN wanting everyone to use the C++17 standard for the next R release.

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.

None yet

3 participants