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

tbb.h deprecated features #191

Open
Enchufa2 opened this issue Jan 24, 2023 · 11 comments
Open

tbb.h deprecated features #191

Enchufa2 opened this issue Jan 24, 2023 · 11 comments

Comments

@Enchufa2
Copy link
Member

Enchufa2 commented Jan 24, 2023

Packages compiling against RcppParallel show:

In file included from /usr/local/lib/R/library/RcppParallel/include/RcppParallel/TBB.h:10,
                 from /usr/local/lib/R/library/RcppParallel/include/RcppParallel.h:21,
                 from fittingFunctions.cpp:6:
/usr/include/tbb/tbb.h:21:154: note: '#pragma message: TBB Warning: tbb.h contains deprecated functionality. For details, please see Deprecated Features appendix in the TBB reference manual.'
   21 | #pragma message("TBB Warning: tbb.h contains deprecated functionality. For details, please see Deprecated Features appendix in the TBB reference manual.")
      |                                                                                                                                                          ^

tbb/tbb.h is included here. We should think about moving away from that header.

@rorynolan
Copy link

rorynolan commented Jan 28, 2023

Possibly related:
CRAN told me to move away from C++11 specification. When I do, win-builder devel gives me

* checking whether package 'detrendr' can be installed ... WARNING
Found the following significant warnings:
  D:/RCompile/CRANpkg/lib/4.3/RcppParallel/include/tbb/concurrent_hash_map.h:343:23: warning: 'template<class _Category, class _Tp, class _Distance, class _Pointer, class _Reference> struct std::iterator' is deprecated [-Wdeprecated-declarations]
  D:/RCompile/CRANpkg/lib/4.3/RcppParallel/include/tbb/internal/_concurrent_queue_impl.h:744:21: warning: 'template<class _Category, class _Tp, class _Distance, class _Pointer, class _Reference> struct std::iterator' is deprecated [-Wdeprecated-declarations]
  D:/RCompile/CRANpkg/lib/4.3/RcppParallel/include/tbb/internal/_concurrent_queue_impl.h:1003:21: warning: 'template<class _Category, class _Tp, class _Distance, class _Pointer, class _Reference> struct std::iterator' is deprecated [-Wdeprecated-declarations]
  D:/RCompile/CRANpkg/lib/4.3/RcppParallel/include/tbb/internal/_concurrent_unordered_impl.h:67:36: warning: 'template<class _Category, class _Tp, class _Distance, class _Pointer, class _Reference> struct std::iterator' is deprecated [-Wdeprecated-declarations]
  D:/RCompile/CRANpkg/lib/4.3/RcppParallel/include/RcppParallel/RMatrix.h:18:24: warning: 'template<class _Category, class _Tp, class _Distance, class _Pointer, class _Reference> struct std::iterator' is deprecated [-Wdeprecated-declarations]

All warnings about deprecation of std::iterator.

@eddelbuettel
Copy link
Member

CRAN told me to move away from C++11 specification.

On upload? Or did you (as I did notice and note here) just saw the note from r-devel? Sometimes those come and go, and not everything added to r-devel checks gets added to incoming checks. That said, they have been making moves there and where we can we should probably move forward. For package BH I commented out an explicit warning that the next upstream will require C++14 (commented out because CRAN dislikes noisy warning) so yes Boost is also moving forward. Generally the right thing to do.

But as we know RcppParallel is also not an easy package to maintain so thanks again to @kevinushey for doing the hard work here.

@rorynolan
Copy link

rorynolan commented Jan 28, 2023

The C++11 warning is a current CRAN note for me on devel.
https://cran.r-project.org/web/checks/check_results_detrendr.html

When I remove the C++11 stuff it and do win-builder, I get the std::iterator warning
https://win-builder.r-project.org/vLN5P4A1iKHT/00check.log

And yes, every thanks to Kevin Ushey.

@eddelbuettel
Copy link
Member

This CRAN test, as far as I know only days old, clearly creates confusion as someone on SO already claimed that 'C++11 is now outlawed at CRAN' which is of course not entirely correct. But as they move the minimum language standard from C++14 (currently) to C++17 they simply flag that maintainers way want to consider moving forward. Which may then, as in your case, trigger new and different warnings (from the compiler and C++ library).

@rorynolan
Copy link

Good to know.
For now, I will just live with the CRAN note, but I suspect poor Kevin will be forced to replace std::iterator eventually.
https://www.fluentcpp.com/2018/05/08/std-iterator-deprecated/

@Enchufa2
Copy link
Member Author

It would be better to open a new issue to track that specifically, because I don't think it has anything to do with the report here.

@Enchufa2 Enchufa2 changed the title tbb.h deprecated features and compilation error with gcc13 tbb.h deprecated features Jan 28, 2023
@Enchufa2
Copy link
Member Author

After all, the compilation error was a gcc issue, present in v13.0.0 but fixed in v13.0.1. First comment edited accordingly. We still should think about moving away from the deprecated header.

@OVVO-Financial
Copy link

Including

CXX_STD = CXX14

in the src/Makevars did not solve this issue...

@eddelbuettel
Copy link
Member

eddelbuettel commented Feb 24, 2023

When I build the main branch from your NNS repo, it compiles fine as C++11 under the default flags I keep (and also without them):

edd@rob:/tmp/rcpp/NNS(NNS-Beta-Version)$ R_LIBS=../lib/ R CMD INSTALL .
* installing to library ‘/tmp/rcpp/lib’
* installing *source* package ‘NNS’ ...
** using staged installation
** libs
ccache g++  -std=gnu++11 -I"/usr/share/R/include" -DNDEBUG  -I'/usr/local/lib/R/site-library/Rcpp/include' -I'/usr/local/lib/R/site-library/RcppParallel/include'    -fpic  -g -O3 -Wall -pipe  -Wno-parentheses -Wno-ignored-attributes -Wno-unused-local-typedefs -Wno-deprecated-declarations -Wno-unused-function  -c RcppExports.cpp -o RcppExports.o
ccache g++  -std=gnu++11 -I"/usr/share/R/include" -DNDEBUG  -I'/usr/local/lib/R/site-library/Rcpp/include' -I'/usr/local/lib/R/site-library/RcppParallel/include'    -fpic  -g -O3 -Wall -pipe  -Wno-parentheses -Wno-ignored-attributes -Wno-unused-local-typedefs -Wno-deprecated-declarations -Wno-unused-function  -c nns_rcpp.cpp -o nns_rcpp.o
ccache g++  -std=gnu++11 -I"/usr/share/R/include" -DNDEBUG  -I'/usr/local/lib/R/site-library/Rcpp/include' -I'/usr/local/lib/R/site-library/RcppParallel/include'    -fpic  -g -O3 -Wall -pipe  -Wno-parentheses -Wno-ignored-attributes -Wno-unused-local-typedefs -Wno-deprecated-declarations -Wno-unused-function  -c partial_moments.cpp -o partial_moments.o
ccache g++  -std=gnu++11 -I"/usr/share/R/include" -DNDEBUG  -I'/usr/local/lib/R/site-library/Rcpp/include' -I'/usr/local/lib/R/site-library/RcppParallel/include'    -fpic  -g -O3 -Wall -pipe  -Wno-parentheses -Wno-ignored-attributes -Wno-unused-local-typedefs -Wno-deprecated-declarations -Wno-unused-function  -c partial_moments_rcpp.cpp -o partial_moments_rcpp.o
ccache g++ -std=gnu++11 -Wl,-S -shared -L/usr/lib/R/lib -Wl,-Bsymbolic-functions -flto=auto -ffat-lto-objects -flto=auto -Wl,-z,relro -o NNS.so RcppExports.o nns_rcpp.o partial_moments.o partial_moments_rcpp.o -L/usr/lib/R/lib -lR
installing to /tmp/rcpp/lib/00LOCK-NNS/00new/NNS/libs
** R
** byte-compile and prepare package for lazy loading
** help
*** installing help indices
** building package indices
** installing vignettes
** testing if installed package can be loaded from temporary location
** testing if installed package can be loaded from final location
** testing if installed package keeps a record of temporary installation path
* DONE (NNS)
edd@rob:/tmp/rcpp/NNS(NNS-Beta-Version)$ 

@OVVO-Financial
Copy link

Thank you Dirk, yes, but CRAN is rejecting this src/Makevars configuration (CXX_STD = CXX14), as well as the following:

CXX_STD = CXX17 and removal (in src/Makevars and ensuring no mention in DESCRIPTION) altogether.

It was a Windows NOTE, as the Debian build was clean...

@eddelbuettel
Copy link
Member

I wonder if in this case you could request that they let you keep CXX_STD=CXX11 until RcppParallel has moved to a new TBB library? (Also, as I recall, on Windows it is yet again a different matter as RcppParallel switches to tinythread (or at least it used to)). All this is a little complicated so you could also consider to bite the bullet and for a release or two to simply skip using RcppParallel until this is sorted out. Different options to consider...

OVVO-Financial pushed a commit to OVVO-Financial/NNS that referenced this issue Feb 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants