-stdlib=libc++ should be conditional on compiler and not hardcoded on macOS #120

Activity

1. wch commented on Dec 7, 2022

   

   wch

   on Dec 7, 2022

   Collaborator

   More actions

   This is similar to #116 (which was fixed) but I think it's a separate issue, since the lines you're referring to are in the Makefile from libsass itself: https://github.com/sass/libsass/blob/2102188d21d2b7577c2b3edb12832e90786a2831/Makefile#L112-L116

   What compiler are you using?
2. barracuda156 commented on Dec 7, 2022

   

   barracuda156

   on Dec 7, 2022

   Author

   More actions

   What compiler are you using?

   @wch It is seen in the log above, GCC 12.

   P. S. I am aware that latest versions of GCC can be configured to support libc++, but a) that is not default behavior, AFAIU, b) it does not work on some archs (ppc, ppcc64), c) older version of GCC do not support that at all, but can be used with R (C++11 support is there), d) using libc++ with GCC is largely untested even on Intel.
3. wch commented on Dec 7, 2022

   

   wch

   on Dec 7, 2022

   Collaborator

   More actions

   Right, I see it's GCC 12. I'm wondering, though, what does the mp mean in gcc-mp-12?

   I also see that you are building for ppc. It may be true that, strictly speaking, the flags in the Makefile should be conditional on the compiler. I'll note, though, that Makefile comes directly from a very widely used (though now dormant) project, libsass, and we would really prefer not to maintain our own fork of the code. There would have to be a very compelling reason for us to do that, and I don't think that supporting a dead platform like PowerPC merits modifying, testing, and maintaining our own version of libsass.
4. barracuda156 commented on Dec 8, 2022

   

   barracuda156

   on Dec 8, 2022

   Author

   More actions

   @wch mp is for Macports.

   The problem is not PPC-specific though. Most of versions of GCC on Intel won’t support linking to libc++, and even those which do, it is an untested and undesirable behavior. The code gets away with these wrong settings simply because most people use Clang on MacOS. That does not make the code correct though.
   The test should not be OS-based, but either test for compiler used or C++ library directly.

   👍React with 👍1Reacted by Galén
5. cpsievert

   closed this as completedin #130on Jul 7, 2023
6. cpsievert commented on Jul 7, 2023

   

   cpsievert

   on Jul 7, 2023

   Last edited by cpsievert

   Contributor

   More actions

   It's seems that #130 might fix this. It'd be much appreciated if you could confirm @barracuda156 by installing the latest dev version -- remotes::install_github("rstudio/sass")

   👍React with 👍1Reacted by Sergey Fedorov
7. cpsievert

   reopened this on Jul 7, 2023
8. barracuda156 commented on Jul 7, 2023

   

   barracuda156

   on Jul 7, 2023

   Author

   More actions

   @cpsievert Thank you, I will try this out soon (and also re-run tests to see if that example still fails).
9. barracuda156 commented on Jul 7, 2023

   

   barracuda156

   on Jul 7, 2023

   Author

   More actions

   It is still broken, since it still hardcodes the wrong runtime for C++, and the source uses C++:

   * installing *source* package ‘sass’ ...
   ** using staged installation
   ** libs
   /opt/local/Library/Frameworks/R.framework/Resources/share/make/shlib.mk:18: warning: overriding commands for target `shlib-clean'
   Makevars:12: warning: ignoring old commands for target `shlib-clean'
   using C compiler: ‘gcc-mp-12 (MacPorts gcc12 12.3.0_0) 12.3.0’
   using C++ compiler: ‘g++-mp-12 (MacPorts gcc12 12.3.0_0) 12.3.0’
   Warning in system2("xcrun", "--show-sdk-path", TRUE, TRUE) :
     running command ''xcrun' --show-sdk-path 2>&1' had status 64
   using SDK: ‘NA’‘NA’‘NA’‘NA’‘NA’‘NA’
   /opt/local/Library/Frameworks/R.framework/Resources/share/make/shlib.mk:18: warning: overriding commands for target `shlib-clean'
   Makevars:12: warning: ignoring old commands for target `shlib-clean'
   /opt/local/bin/gcc-mp-12 -I"/opt/local/Library/Frameworks/R.framework/Resources/include" -DNDEBUG -I./libsass/include  -isystem/opt/local/include/LegacySupport -I/opt/local/include    -fPIC  -pipe -Os -arch ppc  -c compile.c -o compile.o
   /opt/local/bin/g++-mp-12 -std=gnu++17 -I"/opt/local/Library/Frameworks/R.framework/Resources/include" -DNDEBUG -I./libsass/include  -isystem/opt/local/include/LegacySupport -I/opt/local/include    -fPIC  -pipe -Os -arch ppc  -c init.cpp -o init.o
   MAKEFLAGS= CC="/opt/local/bin/gcc-mp-12" CFLAGS="-pipe -Os -arch ppc " CXX="/opt/local/bin/g++-mp-12 -std=gnu++17" AR="ar" LDFLAGS="-Wl,-headerpad_max_install_names -Wl,-rpath,/opt/local/lib/libgcc -L/opt/local/lib -lMacportsLegacySupport -arch ppc" make -C libsass
   /opt/local/bin/gcc-mp-12 -pipe -Os -arch ppc  -O2 -I ./include  -fPIC -c -o src/cencode.o src/cencode.c
   /opt/local/bin/g++-mp-12 -std=gnu++17 -Wall -O2 -std=c++11 -I ./include  -stdlib=libc++ -fPIC -c -o src/ast.o src/ast.cpp
   g++-mp-12: error: unrecognized command-line option '-stdlib=libc++'
   make[1]: *** [src/ast.o] Error 1
   make: *** [libsass/lib/libsass.a] Error 2
   ERROR: compilation failed for package ‘sass’
   
10. barracuda156 commented on Jul 7, 2023

    

    barracuda156

    on Jul 7, 2023

    Author

    More actions

    Why is that flag even hardcoded? Any sane compiler adds its primary runtime libs automatically. I think this is the only R package out of some 3000+ tested so far which has this bug.

    👍React with 👍1Reacted by Akifumi S. Tanabe