issues from CRAN checks

[maelle]

issues from CRAN checks

[szhorvat]

I also will take a look what could be alternatives in R's C API.

I don't think this is a good use of our time. See the issue I linked to (both in this thread and in the past). If the maintainers of packages like rlang think that this is exceedingly difficult, and can't come up with an immediate replacement, then I really don't think we should take this on.

At the risk of sounding like a broken record, I'll say it one last time: I think the only reasonable way forward is to transition to using rlang for lazy evaluation, which is an R programming task, not C. No more repetitive comments on this from me now, promise 😉

[maelle]

https://www.stats.ox.ac.uk/pub/bdr/Intel/igraph.out

[maelle]

@krlmlr

[maelle]

r-lib/cpp11#355

[maelle]

r-lib/rlang#1706

[krlmlr]

https://www.stats.ox.ac.uk/pub/bdr/Intel/igraph.out

More context in https://www.stats.ox.ac.uk/pub/bdr/Intel/igraph.log .

Haven't managed to replicate with the Intel 2023.2 compiler from https://github.com/r-hub/containers/tree/main/containers/intel .

Same problem in duckdb.

[szhorvat]

https://www.stats.ox.ac.uk/pub/bdr/Intel/igraph.out

Here's the warning from the Intel compiler:

In file included from vendor/cigraph/src/misc/degree_sequence.cpp:27:
In file included from /usr/lib/gcc/x86_64-redhat-linux/12/../../../../include/c++/12/algorithm:61:
In file included from /usr/lib/gcc/x86_64-redhat-linux/12/../../../../include/c++/12/bits/stl_algo.h:61:
/usr/lib/gcc/x86_64-redhat-linux/12/../../../../include/c++/12/bits/stl_tempbuf.h:263:8: warning: 'get_temporary_buffer<vd_pair>' is deprecated [-Wdeprecated-declarations]
  263 |                 std::get_temporary_buffer<value_type>(_M_original_len));
      |                      ^
/usr/lib/gcc/x86_64-redhat-linux/12/../../../../include/c++/12/bits/stl_algo.h:4996:15: note: in instantiation of member function 'std::_Temporary_buffer<__gnu_cxx::__normal_iterator<vd_pair *, std::vector<vd_pair>>, vd_pair>::_Temporary_buffer' requested here
 4996 |       _TmpBuf __buf(__first, (__last - __first + 1) / 2);
      |               ^
/usr/lib/gcc/x86_64-redhat-linux/12/../../../../include/c++/12/bits/stl_algo.h:5070:23: note: in instantiation of function template specialization 'std::__stable_sort<__gnu_cxx::__normal_iterator<vd_pair *, std::vector<vd_pair>>, __gnu_cxx::__ops::_Iter_comp_iter<bool (*)(const vd_pair &, const vd_pair &)>>' requested here
 5070 |       _GLIBCXX_STD_A::__stable_sort(__first, __last,
      |                       ^
vendor/cigraph/src/misc/degree_sequence.cpp:87:18: note: in instantiation of function template specialization 'std::stable_sort<__gnu_cxx::__normal_iterator<vd_pair *, std::vector<vd_pair>>, bool (*)(const vd_pair &, const vd_pair &)>' requested here
   87 |             std::stable_sort(vertices.begin(), vertices.end(), degree_less<vd_pair>);
      |                  ^
/usr/lib/gcc/x86_64-redhat-linux/12/../../../../include/c++/12/bits/stl_tempbuf.h:99:5: note: 'get_temporary_buffer<vd_pair>' has been explicitly marked deprecated here
   99 |     _GLIBCXX17_DEPRECATED
      |     ^
/usr/lib/gcc/x86_64-redhat-linux/12/../../../../include/c++/12/x86_64-redhat-linux/bits/c++config.h:2359:34: note: expanded from macro '_GLIBCXX17_DEPRECATED'
 2359 | # define _GLIBCXX17_DEPRECATED [[__deprecated__]]
      |                                  ^

As far as I can tell this is not our problem, but an issue with the combination of Intel CC and libstdc++ on that specific system. We use the std::stable_sort() function in a standard and non-deprecated way. The internal implementation of this function uses get_temporary_buffer, which is claimed to be deprecated, but we have no control over this. This is the internal implementation of stable_sort.

We normally compile the C core in C++11 mode. To double-check, I compiled in C++17 mode both with GCC 13 (libstdc++) and Clang 18 (libc++), and I see no warning.

Feel free to link to this message when submitting to CRAN. I'm happy to take another look if someone can point out a specific issue with the usage of stable_sort, but I don't believe there is any.

[szhorvat]

r-lib/cpp11#355

I believe the SETLENGTH calls comes from code distributed by cpp11, so all we need to do is to wait for that package to address the issue and recompile igraph.

[szhorvat]

r-lib/rlang#1706

The PREXPR call comes code included in igraph, which seems to have been borrowed from lazyeval. As I understand this was superseded by tidyeval then by rlang? Instead of including this code in igraph directly, can we use rlang, and let them fix this on their side?

[maelle]

@Antonov548 do you think some stuff should be fixed on igraph's side?

[szhorvat]

Yes, the PREXPR issue can only be fixed on igraph's side because it's in C code that's included in igraph (basically a vendored version of https://github.com/hadley/lazyeval). However, as I said above, it may be possible to strip this out and replace it with functionality from rlang. I don't know enough about R, and its different lazy evaluation features, to be able to tell if this is possible. But this requires R expertise, not C expertise.

[maelle]

could you please point me to one of the uses of PREXPR?

[szhorvat]

could you please point me to one of the uses of PREXPR?

Are you deeply familiar with R's C API (I'm definitely not)? If not, this is not what you should look at.

PREXPR is in src/lazyeval.c, which as I said is a vendored version of https://github.com/hadley/lazyeval

I suggest you look at what R functions R/lazyeval.R (the R counterpart of src/lazyeval.c) provides. See which of these are used and how they are used. See if these can be replaced with whatever the current rlang provides. What I was able to figure out some weeks ago is that this definitely won't be a drop-in replacement, as the lazy evaluation strategy is said to be different. But I don't know much R and I can't handle it. Do you think you can take a look @maelle ?

In particular, see the use of lazy_dots in make.R and lazy_dots / lazy_eval in iterators.R. Can these be replaced with some rlang features?

[Antonov548]

PREXPR is in src/lazyeval.c, which as I said is a vendored version of https://github.com/hadley/lazyeval

I didn't see immediately your comment about vendoring of package. Good to know it's problem not only in R/igraph.

I also will take a look what could be alternatives in R's C API.

[Antonov548]

I also will take a look what could be alternatives in R's C API.

I don't think this is a good use of our time. See the issue I linked to (both in this thread and in the past). If the maintainers of packages like rlang think that this is exceedingly difficult, and can't come up with an immediate replacement, then I really don't think we should take this on.

At the risk of sounding like a broken record, I'll say it one last time: I think the only reasonable way forward is to transition to using rlang for lazy evaluation, which is an R programming task, not C. No more repetitive comments on this from me now, promise 😉

Make sense. I also just not sure what can be used from rlang. Let's wait then oppnion from R experts.

[maelle]

I am not sure when exactly but yes I know a bit of rlang, enough to look into this

[mpadge]

@maelle FYI The "non-API calls" are definitely an issue from cpp11, and must be resolved there. It's affecting a lot of packages now. PR to fix has been open for a month ,alas.

[maelle]

thanks @mpadge!

[maelle]

I still intend to tackle #1426... but not today.