ericdunipace / RcppCGAL

Package to link to CGAL (Computational Geometry Algorithms Library) header files. Downloads CGAL version 5.6.1 on install.
10 stars 3 forks source link

CRAN issue #11

Closed stla closed 9 months ago

stla commented 1 year ago

Dear Eric,

You probably received the email from CRAN regarding RcppCGAL. I asked some help on StackOverlow and Dirk Eddelbuettel proposes a potential solution (personally I don't understand it).

m-jahn commented 1 year ago

Hi Eric, my R package WeightedTreemaps depends on your package RcppCGAL and to run autmatic checks on Github, it would be nice to get it up and running on CRAN. You probably haven't noticed, it was removed just two days ago.

Let me know if help is needed to fix the issue (though personally, I am not very fluent in C++).

ericdunipace commented 1 year ago

Sorry about that guys. I was on a busy rotation but will look at in the next few days

m-jahn commented 1 year ago

Thank you, that's great to hear!

stla commented 1 year ago

@m-jahn You can say to R to use the Github version, for your Github actions. Just add this line in DESCRIPTION:

Remotes: ericdunipace/RcppCGAL

But gcc has been upgraded and there's the same problem as CRAN, at least with the Ubuntu OS.

m-jahn commented 1 year ago

@stla Thanks for the hint!

tylermorganwall commented 12 months ago

@ericdunipace, part of the issue is exit() is some places (in particular, in https://github.com/CGAL/cgal/blob/master/Triangulation_3/include/CGAL/Triangulation_segment_traverser_3.h) is an internal class function and simply find/replacing all calls to exit() in the package may break the package in these cases. As an example, changing:

std::tuple<Locate_type, int, int> exit() const
    {
      return { prev_lt(), prev_li(), prev_lj() };
    }

to:

std::tuple<Locate_type, int, int> Rcpp::stop("Error") const
    {
      return { prev_lt(), prev_li(), prev_lj() };
    }

will means any calls to this code will fail. The issue isn't that your search/replace is missing exit() somewhere, but rather that the search/replace you've put together is defining a function with a namespace operator in front of it and a string constant in the place of the arguments. Which is why you end up with these two errors:

At global scope:
/data/gannet/ripley/R/test-dev/RcppCGAL/include/CGAL/Triangulation_segment_traverser_3.h:371:49: 
error: expected identifier before string constant
   371 |     std::tuple<Locate_type, int, int>Rcpp::stop("Exit Error") const
       |                                                 ^~~~~~~~~~~~
/data/gannet/ripley/R/test-dev/RcppCGAL/include/CGAL/Triangulation_segment_traverser_3.h:371:49: 
error: expected ',' or '...' before string constant
/data/gannet/ripley/R/test-dev/RcppCGAL/include/CGAL/Triangulation_segment_traverser_3.h:371:38: 
error: invalid use of '::'
   371 |     std::tuple<Locate_type, int, int>Rcpp::stop("Exit Error") const
       |      

The compiler and R's CHECK should hopefully be smart enough to realize that these calls to exit are class functions and not calls to the global exit(), so if you just skip them in your search/replace I believe you should be fine. If not, just have a pre-pass before your main search/replace where you re-name this exit() function to something else (e.g. exit_cranfix()).

...
tx[search]  <- gsub(pattern = "std::tuple<Locate_type, int, int> exit\\(\\) const", replacement="std::tuple<Locate_type, int, int> exit_cran() const", x = tx[search])
...

You would just have to note to people using the headers that this change has been made (I don't believe CGAL calls the empty exit() function anywhere, based on my search).

tylermorganwall commented 12 months ago

I fetched the most recent docker container with gcc 13.2,

docker pull cran/debian

and built the package, and it seemed that subbing in the changes I made in my fork of RcppCGAL fixed the compilation error when trying to install @stla's cgalMeshes package. I didn't actually complete the compilation because my docker container ran out of memory installing, but it didn't fail where it did previously with the current github version of RcppCGAL. I'll open a merge request.

stla commented 11 months ago

Dear Eric,

I tested @tylermorganwall 's pull request and I confirm it works!

@m-jahn ,

You can use @tylermorganwall 's fork by including

Remotes: tylermorganwall/RcppCGAL

in your DESCRIPTION file.

Thank you @tylermorganwall !

m-jahn commented 11 months ago

great, maybe @ericdunipace will merge it soon and get the package up and running again on CRAN.. Remotes are as far as I know not permitted as dependency for other CRAN packages.

stla commented 11 months ago

Yes, remotes are not permitted on CRAN, but it's nice for Github actions.

ericdunipace commented 9 months ago

Finally re-accepted by CRAN