eddelbuettel / bh

R package providing Boost Header files
85 stars 33 forks source link

Consider removing boost::filesystem #55

Closed Enchufa2 closed 4 years ago

Enchufa2 commented 6 years ago

I tried to use boost::filesystem, which is listed as available in BH, but it seems that it requires linking to boost::system:

Rcpp::sourceCpp(code='
  #include <Rcpp.h>
  // Rcpp::depends("BH")
  #include <boost/filesystem.hpp>

  // [[Rcpp::export]]
  int hello() { return 0; }
')

gives undefined symbol: boost::system::generic_category().

eddelbuettel commented 6 years ago

I suspect that some headers lead to a linking requirement. That is the same for some other Boost libraries. So there may be both supported and unsupported use case.

eddelbuettel commented 6 years ago

Looks like filesystem has been part of BH for some time, but it also looks like may need compilation. From its documentation:

Using the library

Boost.Filesystem is implemented as a separately compiled library, so you must install binaries in a location that can be found by your linker. If you followed the Boost Getting Started instructions, that's already been done for you.

So maybe we should remove it...

Enchufa2 commented 6 years ago

I found this, and the following define seems to solve the issue:

Rcpp::sourceCpp(code='
  #include <Rcpp.h>
  // Rcpp::depends("BH")
  #define BOOST_ERROR_CODE_HEADER_ONLY
  #include <boost/filesystem.hpp>

  // [[Rcpp::export]]
  int hello() { return 0; }
')

If this is enough to make it work, maybe BH should set that define by default.

eddelbuettel commented 6 years ago

Dang, that is excellent. I was actually just looking at Boost filesystem a few weeks ago for this commit in RcppCNPy and ended up going to R instead because of the linking issue. Need to look some into this.

We may need a new FAQ vignette with this in it.

eddelbuettel commented 6 years ago

Ok, I checked, If I actually want to invoke filesystem functions that seem system dependent ("does path exist?") I still get error, particularly boost::filesystem::detail::status(boost::filesystem::path const&, boost::system::error_code*) being undefined. No free lunch...

Enchufa2 commented 6 years ago

That's a pity, but sounds reasonable though. It appears that only some path utilities (which are nothing but string manipulations) can be used without linking. I'd vote for removing it then.

eddelbuettel commented 6 years ago

Agreed and retitled. I think on the next update I will remove it.

nbenn commented 5 years ago

This might be a dumb question, but why exactly is there talk about removing a component such as filesystem? Personally I hope, this does not happen as I just included BH provided boost::filesystem into a project of mine.

Isn't it sufficient to add something like PKG_LIBS = -lboost_filesystem to Makevars for linking? To be honest, I was surprised how easy this was (maybe, I'm lucky that boost libs are available from one of my system library directories), but @eddelbuettel suggested this both on SO and is doing so in a package.

eddelbuettel commented 5 years ago

Isn't it sufficient to add something like PKG_LIBS = -lboost_filesystem

Sure. Only that libboost_filesystem is not part of this package, so you are now mixing your local installation with the package. Nothing "wrong" with that but you can't distribute that code.

And of course if you have a full Boost locally you don't need the BH package....

nbenn commented 5 years ago

I understand that, but aren't you doing the same with rcppbdt?

eddelbuettel commented 5 years ago

No I don't as it is header-only as well.

nbenn commented 5 years ago

I'm sorry, I did not read Makevars of rcppbdt correctly: PKG_LIBS is commented out.

## Needed for the 'non-streams' variant charToPOSIXctNS
#PKG_LIBS = -lboost_date_time

So to do this properly, I need to add a configure script to look up boost include & lib dirs on the target machine and maybe add boost as SystemRequirements, right?

Do you off the top of your head know of a package that does something like this?

eddelbuettel commented 5 years ago

Correct. (And I myself had forgotten I once had a tester function needing linking. The intent for all this Boost use was always portability -- no linking.)

Because it is a real problem. I have two projects I am involved in blocked from CRAN access because they require linking with boost:

So I in short I would recommend to you to avoid linking if CRAN is your goal.

nbenn commented 5 years ago

Thanks a lot for sharing your insight. One last question: I can easily swap boost::filesystem for C++17 std::filesystem. Is that also a problem with CRAN?

eddelbuettel commented 5 years ago

Sadly, yes. Some packages currently use C++14 but as far as I know nothing beyond. Once the Rtools set gets updated (to, I think, g++ 8) you can assume it on Windows.

nbenn commented 5 years ago

Ok, thanks for the help!

eddelbuettel commented 4 years ago

Preparing BH 1.72.0 now based on last week's Boost 1.72 release. I removed filesystem from the list of explicitly added packages ... but some of its files are still coming in via bcp and cross-library dependencies. Such is life ...