Thank you very much for the very detailed response!
Using your Makevars I got Gecode to compile and the package to build on
windows. In my case I had to adjust a part of the configure script
responsible for choosing the thread-model used, as the script did not
directly check for the host-os but rather available libraries (which
lead to a problem in my case as the pthreads library itself was
available, however, it is not the correct choice for windows).
After that I ran the CRAN checks, interestingly not yielding the "array
subscript -1 is below array bounds" warning (which should, however,
indeed just be a false positive). Regarding the "mismatched-new-delete"
warning I am also relatively sure that this is actually a false
positive. Taking a look into the stack-trace, the new and delete
operators are actually overloaded with functions that internally use
malloc and free, hence, there is no mismatch:
void*
BoolExpr::Node::operatornew(size_tsize) {
returnheap.ralloc(size);
}
void
BoolExpr::Node::operatordelete(void*p, size_t) {
heap.rfree(p);
}
BoolExpr::BoolExpr(void) : n(newNode) {}
The methods on heap then refer to an allocator where malloc and free are
used. What would be the course of action with warnings like this, can I
just suppress them with something like|:|
|#pragma GCC diagnostic ignored "-Wmismatched-new-delete"|
- and would this still be tolerated by CRAN?
Best,
Nils
Am 30.04.2025 um 20:57 schrieb Ivan Krylov:
> В Tue, 29 Apr 2025 14:43:40 +0200
> Nils Lüschow<nils.luesc...@uni-duesseldorf.de> пишет:
>
>> While we generally got this feature to work when Gecode is
>> pre-installed on the system, we would like to spare the users this
>> step and include it with the package
> It's probably a good idea to try to achieve both. If a system copy of
> the package is available, link with it; otherwise build the bundled
> copy. Unfortunately, this kind of user convenience involves a lot of
> testing to make sure that both configurations work as intended.
> Bundling dependencies also creates headaches for distro packagers.
>
>> However, we run into a problem when it comes to compiling Gecode
>> during the packages build-process, which I describe in this SO post
> One problem is that when using $(MAKE) to compile gecode, various
> variables used by gecode's Makefile are overridden through the
> MAKEFLAGS variable. For example, $(CXX) ends up being defined as
> $(CXX20) $(CXX20STD) without the variables CXX20 and CXX20STD being
> available to the downstream Make process. What's worse is CXXFLAGS are
> also overridden, losing the include paths in the process.
>
> In order to build gecode from Makevars, you'll have to disable these
> overrides by emptying MAKEFLAGS:
>
> build_gecode:
> cd $(GECODE_DIR) && \
> ./configure CC="$(CC)" CXX="$(CXX20) $(CXX20STD)" \
> --disable-examples --enable-static --disable-shared
> $(MAKE) -C $(GECODE_DIR) MAKEFLAGS=
>
> (Alternatively, you can ./configure and build the library from the
> ./configure script of your package.)
>
> Other changes are also needed to make the package build in a portable
> manner:
>
> - Recipe lines in Makefiles must start with tabs, not groups of
> four spaces.
> - Don't use GNU Make append syntax (+=) without declaring
> SystemRequirements: GNU Make.
> - Don't set compiler-specific flags such as -mtune=native without at
> least testing that the compiler understands them.
> - With the current directory structure, GECODE_DIR must be just
> gecode, not $(CURDIR)/gecode-release-6.3.0.
> - Since the C++ source code already uses #include <gecode/...>, the
> include path must be -I$(GECODE_DIR), not -I$(GECODE_DIR)/gecode.
> - Don't hard-code compiler names. Instead of ./configure CC=gcc ...,
> use ./configure CC="$(CC)" CXX="$(CXX20) $(CXX20STD)".
> - Build gecode as a static library and link it into the package
> shared library. Otherwise you'll need to arrange for the gecode
> shared libraries to be installed together with the R package and
> properly loaded before your package shared library.
> - Provide a 'clean' target so that R CMD build doesn't include the
> binaries from the gecode subdirectory into the source package.
>
> With that, we get the following Makevars:
>
> all: $(SHLIB)
>
> GECODE_DIR = gecode
> PKG_CPPFLAGS = -I$(GECODE_DIR)
> PKG_LIBS = -L$(GECODE_DIR) -lgecodesearch -lgecodeminimodel \
> -lgecodeint -lgecodekernel -lgecodesupport
> CXX_STD = CXX20
>
> $(SHLIB): gecode_built
> # package code relies on header files only available after gecode is
> # configured and probably built
> $(OBJECTS): gecode_built
>
> gecode_built:
> cd $(GECODE_DIR) && \
> ./configure CC="$(CC)" CXX="$(CXX20) $(CXX20STD)" \
> --disable-examples --enable-static --disable-shared \
> --disable-flatzinc --disable-qt
> $(MAKE) -C $(GECODE_DIR) MAKEFLAGS=
> touch $@
>
> clean:
> -cd $(GECODE_DIR) && make clean
> -rm -f gecode_built
>
> Hopefully, the Makevars.win with same contents but
> --with-host-os=windows added to the configure flags will work on
> Windows. (It needs to be .win, not .ucrt, because your package declares
> support for R > 3.6, which predates use of UCRT in R. To think of it,
> R-3.6.0 also predates C++20...) You can probably give some more
> --disable-... arguments to ./configure to remove some more components
> that the R package doesn't use.
>
> However, this is only the beginning of the troubles.
>
> R packages are not allowed file path lengths above 100 bytes, and your
> package has seven that are longer, e.g.,
>
>>> RcppTestPackage/src/gecode/gecode/third-party/boost/numeric/interval/detail/x86gcc_rounding_control.hpp
> Maybe you can unbundle the bits of Boost and rely on the 'BH' package
> instead. Maybe there's a way to rename the files and patch the includes
> in the source code.
>
> The gecode library produces a lot of warnings, and R CMD check
> considers some of them serious:
>
>>> Found the following significant warnings:
>>> ./gecode/kernel/core.hpp:4102:24: warning: array subscript -1 is
>>> below array bounds of ‘unsigned int [1]’ [-Warray-bounds]
>>> ./gecode/kernel/core.hpp:4109:17: warning: array subscript -1 is
>>> below array bounds of ‘unsigned int [1]’ [-Warray-bounds]
>>> ./gecode/support/allocator.hpp:88:11: warning: ‘void free(void*)’
>>> called on pointer returned from a mismatched allocation function
>>> [-Wmismatched-new-delete]
> The -Warray-bounds warnings might be false positives (could pc_max ever
> be 0?), but -Wmismatched-new-delete is probably real; the compiler
> shows a place where an object allocated using new(...) can be
> deallocated using free(...).
>
> Some parts of the gecode library contain references to std::cerr, and
> the R package code calls rand(). These are not recommended in R
> packages, because the standard error stream isn't necessarily connected
> to the terminal and because rand() is disconnected from R's random
> number generator.
>
> Once you build the whole gecode yourself, be ready for it being tested
> with AddressSanitizer and UndefinedBehaviorSanitizer. Things like
> free(new(...)) that were previously invisible to them (due to the
> third-party library being compiled without sanitizers) will now be
> revealed (and become yours to fix instead of just something happening
> upstream).
>
> There are other problems noted by R CMD check, but they should be
> easier to fix.
>
[[alternative HTML version deleted]]
______________________________________________
R-package-devel@r-project.org mailing list
https://stat.ethz.ch/mailman/listinfo/r-package-devel
