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

Consider removing boost::filesystem #55

Closed
Enchufa2 opened this issue Mar 25, 2018 · 17 comments · Fixed by #65
Closed

Consider removing boost::filesystem #55

Enchufa2 opened this issue Mar 25, 2018 · 17 comments · Fixed by #65

Comments

@Enchufa2
Copy link

@Enchufa2 Enchufa2 commented Mar 25, 2018

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

This comment has been minimized.

Copy link
Owner

@eddelbuettel eddelbuettel commented Mar 25, 2018

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

This comment has been minimized.

Copy link
Owner

@eddelbuettel eddelbuettel commented Mar 25, 2018

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

This comment has been minimized.

Copy link
Author

@Enchufa2 Enchufa2 commented Mar 25, 2018

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

This comment has been minimized.

Copy link
Owner

@eddelbuettel eddelbuettel commented Mar 25, 2018

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

This comment has been minimized.

Copy link
Owner

@eddelbuettel eddelbuettel commented Mar 25, 2018

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

This comment has been minimized.

Copy link
Author

@Enchufa2 Enchufa2 commented Mar 26, 2018

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 eddelbuettel changed the title undefined symbol for boost::filesystem Consider removing boost::filesystem Mar 26, 2018
@eddelbuettel

This comment has been minimized.

Copy link
Owner

@eddelbuettel eddelbuettel commented Mar 26, 2018

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

@nbenn

This comment has been minimized.

Copy link

@nbenn nbenn commented Jul 16, 2019

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

This comment has been minimized.

Copy link
Owner

@eddelbuettel eddelbuettel commented Jul 16, 2019

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

This comment has been minimized.

Copy link

@nbenn nbenn commented Jul 16, 2019

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

@eddelbuettel

This comment has been minimized.

Copy link
Owner

@eddelbuettel eddelbuettel commented Jul 16, 2019

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

@nbenn

This comment has been minimized.

Copy link

@nbenn nbenn commented Jul 16, 2019

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

This comment has been minimized.

Copy link
Owner

@eddelbuettel eddelbuettel commented Jul 16, 2019

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

This comment has been minimized.

Copy link

@nbenn nbenn commented Jul 16, 2019

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

This comment has been minimized.

Copy link
Owner

@eddelbuettel eddelbuettel commented Jul 16, 2019

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

This comment has been minimized.

Copy link

@nbenn nbenn commented Jul 16, 2019

Ok, thanks for the help!

@eddelbuettel

This comment has been minimized.

Copy link
Owner

@eddelbuettel eddelbuettel commented Dec 15, 2019

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 ...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.