search

eddelbuettel / rquantlib

R interface to the QuantLib library
120 stars 50 forks source link

Duplicate symbol error when using RQuantLib in a package #182

Closed oelhammouchi closed 7 months ago

oelhammouchi commented 8 months ago

First of all, thank you so much for your work on this amazing package! I'm currently trying to use in an internal package for the company I work at, but I'm stumbling on the following problem.

My goal is to have the interfaces for C++ functions taking in or returning QuantLib date types be automatically generated (much like they are for Armadillo classes with Rcpp). However, the master header file RQuantLib.h does not include the wrapper functionality implemented in rquantlib_impl.h by default. When I try to force it by passing -DRQuantLib_Plugin in my Makevars, the linker complains about "duplicate symbols".

I think the issue is caused by the fact that the templates for as and wrap are not marked inline and thus are put separately in all the object files which use the header (I will try to clone the repo to verify this once I get home this evening). In the mean time, I'm doing

#include <rquantlib_impl.h>
#include <rquantlib_internal.h>

but that doesn't seem to me to follow best practices. Have you encountered this difficulty before?

eddelbuettel commented 8 months ago

Howdy, and glad to see this being used. It could well be that I overlooked something, but it could also be that you are doing something not exactly right? It has been a while but I the additional header was really just for small use case demos via Rcpp::sourceCpp() and Rcpp::cppFunction(). And I don't think we mean for 'plugins' to be used in a package.

We can and should definitely support general conversion of Date objects so let's experiment. I also used the in other places. One approach you could try for now is to not use =DRQuantLib_Plugin but to bring the headers in otherwise (maybe via a copy). Also see what repos rcppquantuccia (the initial attempt) and qlcal-r do.

eddelbuettel commented 8 months ago

Another thing that may help (locally) is to not include all headers in all source files. Plus, headers ending in *_impl.h are typically "impementation detail" headers not meant for you to include. So somewhere between you and me our wires are crossed.

I understand you cannot expose company code but it may be useful if you could set up a 'mock' mimimal RQuantLib client package doing something trivial (take an instrument, prints its value say) to 'mock' what you real payload does just so that we can work out setup kinks.

oelhammouchi commented 8 months ago

Wow, thanks a lot for the quick response! I suspected that the -DRQuantLib_Plugin wasn't really meant for package development, I just found it mentioned in a talk you gave while Googling if there was an easier way to interface with QuantLib::Date and friends. Having to worry about the conversions everywhere in our current code is somewhat tedious.

Ok, so I tested my theory using a mock client package as you suggested. I also forked the repo and inlined the wrappers in rquantlib_impl.h and amended RQuantLib.h to always include it (i.e. not just when -DRQuantLib_Plugin is set). The test package compiles without problem and automagically a QuantLib::Date from C++ to R when using Rcpp attributes.

eddelbuettel commented 8 months ago

Excellent news! I have the feeling I also have that in use somewhere and yes, that is how it should be used. Now, while all this is fresh in your head, any suggestions as to

oelhammouchi commented 8 months ago

Oh, I should also add that my work machine is MacOS and my personal setup (which the last comment was based on) is Debian.

oelhammouchi commented 8 months ago

Excellent news! I have the feeling I also have that in use somewhere and yes, that is how it should be used. Now, while all this is fresh in your head, any suggestions as to

* If/where/how I should (re-)document that "plugins" are not for package development?  (We used to stress that more when plugins were written for Rcpp more frequently)

* Should we make your mock client package an example package?  I found over the years with some packages such as RcppArmadillo and nloptr, it helps to have 'demo packages'.  (I haven't had time to look at your mock pkg yet, busy day...)

* Ditto for inline and the header.  I am a little lost right now as I checked earlier with some other packages and I seem to recall that we did not need to / did not inline the `template` statements.  But it is quite possible that I overlooked another segment and that there are 'plain' free functions that need it.  If you have a link to a diff I'll take a look.

All right, I'll look into this ASAP!

oelhammouchi commented 8 months ago

@eddelbuettel Sorry for the delay, I had a very busy weekend.

eddelbuettel commented 8 months ago

The changes look good. It's a possible oversight on my end. I'll try to look at the example package some more this week.

eddelbuettel commented 8 months ago

Took me a moment to get over there. It looks good overall (I have some quibbles with the README addition but it too has its heart in the right place).

The bigger issue with (generally nice, we needed something like this years ago) example package! It goes back to the old issue that folks sometimes "just" want date calcs and exspecially calendaring. And for that building against QuantLib is expensive as building QuantLib is expensive. And ... that is what I built qlcal/qlcal-r. It too exports an as/wrap header (which may have the same bloober that I forgot to inject a few inline).

Would that be simpler for you to use? I currently have it spec'ed to do 'just' calendaring. We could either put missing date calcs (for schedules) back in, or do something similar. But I really think we are better off with lighter header-only libraries.

Lastly, the diff is good. Let me play with your example package to 'proof' that we need it. I think this could well become a nice little PR for RQuantLib.

eddelbuettel commented 8 months ago

The diff is good! Please send a PR to RQuantLib. Add, if you like, an entry to ChangeLog - format is Date, four spaces, name, four spaces, email header followed by the 'one tab,*' descriptions.

oelhammouchi commented 8 months ago

The diff is good! Please send a PR to RQuantLib. Add, if you like, an entry to ChangeLog - format is Date, four spaces, name, four spaces, email header followed by the 'one tab,*' descriptions.

PR has been submitted! I forgot to take out the changes to README.md at first, so I had to close and reopen. Sorry :sweat_smile:

oelhammouchi commented 8 months ago

Took me a moment to get over there. It looks good overall (I have some quibbles with the README addition but it too has its heart in the right place).

The bigger issue with (generally nice, we needed something like this years ago) example package! It goes back to the old issue that folks sometimes "just" want date calcs and exspecially calendaring. And for that building against QuantLib is expensive as building QuantLib is expensive. And ... that is what I built qlcal/qlcal-r. It too exports an as/wrap header (which may have the same bloober that I forgot to inject a few inline).

Would that be simpler for you to use? I currently have it spec'ed to do 'just' calendaring. We could either put missing date calcs (for schedules) back in, or do something similar. But I really think we are better off with lighter header-only libraries.

Lastly, the diff is good. Let me play with your example package to 'proof' that we need it. I think this could well become a nice little PR for RQuantLib.

Ah, I see your point. For our use case we need the whole of QuantLib because we use the more heavy-duty stuff as well, but you're right that this may not necessarily reflect the typical user. I can take a look at qlcal/qlcal-r with regards to the above issue if you like?

For the example package, would it perhaps be better to show some QuantLib functionality (e.g. pricing a bond) which then incidentally uses the wrapping functionality? Anyhow, looking forward to your input on this.

eddelbuettel commented 7 months ago

Yeah to justify the higher building cost it may make sense to show a price -- option or bond or ...

eddelbuettel commented 7 months ago

so I had to close and reopen.

Actually you can always follow-up with commits within the same PR. git and GitHub are featureful and it all takes a moment of practice so no worries!

oelhammouchi commented 7 months ago

so I had to close and reopen.

Actually you can always follow-up with commits within the same PR. git and GitHub are featureful and it all takes a moment of practice so no worries!

Yeah, I figured this out shortly after my oopsie :sweat_smile:. I have a fair bit of experience in programming and git VCS, but this is my first foray into open source on GH. Thanks for bearing with me!

oelhammouchi commented 7 months ago

Yeah to justify the higher building cost it may make sense to show a price -- option or bond or ...

I'll look into it!

eddelbuettel commented 7 months ago

Also see the follow-uo discussion in #184 the PR actually damaged our ability to build on macOS :-/