[JOSS Pre-Review]: Hardcoded compiler assumptions in Makefile

villaa commented 2 years ago

See the second-submission's pre-review thread.

[ ] Installation: Does installation proceed as outlined in the documentation?

No. The Makefile has hardcoded assumptions on the C++ standard that will be used, while they should instead rely on the information provided by root-config --cflags. Here the Makefile has hardcoded c++14. If this software wants to make the decision to enforce an uppper bound on the C++ standards they support that's not fundamentally wrong, but it needs to be made explicitly clear to the user in advance.

Reproducible example of failing with ROOT compiled against c++17:

$ docker run --rm -ti -v $PWD:$PWD -w $PWD atlasamglab/stats-base:root6.24.06
# git clone --depth 1 https://github.com/villano-lab/nrCascadeSim.git --branch v1.2.3 --single-branch
# cd nrCascadeSim/
# gcc --version | head -n 1
gcc (Debian 10.2.1-6) 10.2.1 20210110
# root --version
ROOT Version: 6.24/06
Built for linuxx8664gcc on Oct 19 2021, 19:44:00
From tags/v6-24-06@v6-24-06
# root-config --cflags
-pthread -std=c++17 -m64 -I/usr/local/venv/include
# make
g++ -fPIC -c -D__GIT_VERSION=\"v1.2.3\" -IMersenne -Iinc src//isotope_info.c `root-config --cflags --glibs` -std=c++14 -L. -L/tmp/nrCascadeSim/bin/lib
mv isotope_info.o /tmp/nrCascadeSim/bin/lib/
g++ -fPIC -c -D__GIT_VERSION=\"v1.2.3\" -IMersenne -Iinc src//weisskopf.c `root-config --cflags --glibs` -std=c++14 -L. -L/tmp/nrCascadeSim/bin/lib
mv weisskopf.o /tmp/nrCascadeSim/bin/lib/
g++ -fPIC -c -D__GIT_VERSION=\"v1.2.3\" -IMersenne -Iinc src//lindhard.c `root-config --cflags --glibs` -std=c++14 -L. -L/tmp/nrCascadeSim/bin/lib
mv lindhard.o /tmp/nrCascadeSim/bin/lib/
g++ -fPIC -c -D__GIT_VERSION=\"v1.2.3\" -IMersenne -Iinc src//cascadeProd.c `root-config --cflags --glibs` -std=c++14 -L. -L/tmp/nrCascadeSim/bin/lib
src//cascadeProd.c: In function ‘cri* Cascade(int, int, double, int, double*, double*, double, MTRand*)’:
src//cascadeProd.c:1113:1: warning: control reaches end of non-void function [-Wreturn-type]
 1113 | }
      | ^
mv cascadeProd.o /tmp/nrCascadeSim/bin/lib/
g++ -fPIC -c -D__GIT_VERSION=\"v1.2.3\" -IMersenne -Iinc src//edepmath.c `root-config --cflags --glibs` -std=c++14 -L. -L/tmp/nrCascadeSim/bin/lib
mv edepmath.o /tmp/nrCascadeSim/bin/lib/
g++ -fPIC -c -D__GIT_VERSION=\"v1.2.3\" -IMersenne -Iinc src//rootUtil.c `root-config --cflags --glibs` -std=c++14 -L. -L/tmp/nrCascadeSim/bin/lib
In file included from /usr/local/venv/include/TString.h:29,
                 from /usr/local/venv/include/TNamed.h:26,
                 from /usr/local/venv/include/TDictionary.h:44,
                 from /usr/local/venv/include/TClass.h:23,
                 from /usr/local/venv/include/TTree.h:36,
                 from inc/rootUtil.h:3,
                 from src//rootUtil.c:8:
/usr/local/venv/include/ROOT/RStringView.hxx:84:17: error: expected type-specifier
   84 |        operator std::string_view() const { return std::string_view(fData,fLength); }
      |                 ^~~
In file included from /usr/local/venv/include/TNamed.h:26,
                 from /usr/local/venv/include/TDictionary.h:44,
                 from /usr/local/venv/include/TClass.h:23,
                 from /usr/local/venv/include/TTree.h:36,
                 from inc/rootUtil.h:3,
                 from src//rootUtil.c:8:
/usr/local/venv/include/TString.h:115:13: error: expected type-specifier
  115 |    operator std::string_view() const { return std::string_view(Data(),fExtent); }
      |             ^~~
/usr/local/venv/include/TString.h:280:32: error: ‘string_view’ in namespace ‘std’ does not name a type
  280 |    explicit TString(const std::string_view &sub);
      |                                ^~~~~~~~~~~
/usr/local/venv/include/TString.h:280:27: note: ‘std::string_view’ is only available from C++17 onwards
  280 |    explicit TString(const std::string_view &sub);
      |                           ^~~
/usr/local/venv/include/TString.h:317:37: error: ‘string_view’ in namespace ‘std’ does not name a type
  317 |    TString    &operator=(const std::string_view &s);
      |                                     ^~~~~~~~~~~
/usr/local/venv/include/TString.h:317:32: note: ‘std::string_view’ is only available from C++17 onwards
  317 |    TString    &operator=(const std::string_view &s);
      |                                ^~~
/usr/local/venv/include/TString.h:444:9: error: ‘string_view’ in namespace ‘std’ does not name a type
  444 |    std::string_view View() const { return std::string_view(GetPointer(),Length()); }
      |         ^~~~~~~~~~~
/usr/local/venv/include/TString.h:444:4: note: ‘std::string_view’ is only available from C++17 onwards
  444 |    std::string_view View() const { return std::string_view(GetPointer(),Length()); }
      |    ^~~
In file included from /usr/local/venv/include/TNamed.h:26,
                 from /usr/local/venv/include/TDictionary.h:44,
                 from /usr/local/venv/include/TClass.h:23,
                 from /usr/local/venv/include/TTree.h:36,
                 from inc/rootUtil.h:3,
                 from src//rootUtil.c:8:
/usr/local/venv/include/TString.h:839:53: error: ‘string_view’ in namespace ‘std’ does not name a type
  839 | inline Bool_t operator==(const char *s1, const std::string_view &s2)
      |                                                     ^~~~~~~~~~~
/usr/local/venv/include/TString.h:839:48: note: ‘std::string_view’ is only available from C++17 onwards
  839 | inline Bool_t operator==(const char *s1, const std::string_view &s2)
      |                                                ^~~
/usr/local/venv/include/TString.h:839:15: error: ‘Bool_t operator==(const char*, const int&)’ must have an argument of class or enumerated type
  839 | inline Bool_t operator==(const char *s1, const std::string_view &s2)
      |               ^~~~~~~~
/usr/local/venv/include/TString.h:844:37: error: ‘string_view’ in namespace ‘std’ does not name a type
  844 | inline Bool_t operator==(const std::string_view &s1, const char *s2)
      |                                     ^~~~~~~~~~~
/usr/local/venv/include/TString.h:844:32: note: ‘std::string_view’ is only available from C++17 onwards
  844 | inline Bool_t operator==(const std::string_view &s1, const char *s2)
      |                                ^~~
/usr/local/venv/include/TString.h:844:15: error: ‘Bool_t operator==(const int&, const char*)’ must have an argument of class or enumerated type
  844 | inline Bool_t operator==(const std::string_view &s1, const char *s2)
      |               ^~~~~~~~
/usr/local/venv/include/TString.h:857:37: error: ‘string_view’ in namespace ‘std’ does not name a type
  857 |   std::string printValue(const std::string_view* val);
      |                                     ^~~~~~~~~~~
/usr/local/venv/include/TString.h:857:32: note: ‘std::string_view’ is only available from C++17 onwards
  857 |   std::string printValue(const std::string_view* val);
      |                                ^~~
In file included from inc/rootUtil.h:4,
                 from src//rootUtil.c:8:
/usr/local/venv/include/TFile.h:327:45: error: ‘std::string_view’ has not been declared
  327 |    static Bool_t       SetCacheFileDir(std::string_view cacheDir, Bool_t operateDisconnected = kTRUE,
      |                                             ^~~~~~~~~~~
/usr/local/venv/include/TFile.h: In static member function ‘static Bool_t TFile::SetCacheFileDir(ROOT::Internal::TStringView, Bool_t, Bool_t)’:
/usr/local/venv/include/TFile.h:326:36: error: ‘string_view’ is not a member of ‘std’
  326 |      { return SetCacheFileDir(std::string_view(cacheDir), operateDisconnected, forceCacheread); }
      |                                    ^~~~~~~~~~~
/usr/local/venv/include/TFile.h:326:36: note: ‘std::string_view’ is only available from C++17 onwards
make: *** [Makefile:39: /tmp/nrCascadeSim/bin/lib/rootUtil.o] Error 1

If the hardcoded C++ flags are changed to reflect the version of C++ used to compile ROOT then things will pass (this is why the project CI passes, as only c++14 is tested).

...

# sed -i 's/c++14/c++17/g' Makefile
# make clean && make  # Skipping lots of build warning here for readability
# tree bin
bin
├── lib
│   ├── cascadeProd.o
│   ├── edepmath.o
│   ├── isotope_info.o
│   ├── libncap.so
│   ├── lindhard.o
│   ├── rootUtil.o
│   └── weisskopf.o
├── realizeCascades
└── realizeCascades.cpp

1 directory, 9 files
# command -v realizeCascades  # Just showing that nothing is in PATH yet as expected
# make install
cp /tmp/nrCascadeSim/bin/realizeCascades /usr/local/bin/
# command -v realizeCascades
/usr/local/bin/realizeCascades

This should not be the responsibility of the user, but should be discoverable and covered in tests.

The install Make target also hardcodes the install target directory to /usr/local/bin. Install locations can have defaults, but should allow for user input. Similarly, advocating directly for the use of sudo for installation of local software should be avoided.

matthewfeickert commented 2 years ago

Coming here from https://github.com/villano-lab/nrCascadeSim/issues/48#issuecomment-993035515, I guess this was already know to you given:

https://github.com/villano-lab/nrCascadeSim/blob/59d756c592dc0801340876e803c4a5393d6b0cc0/Makefile#L20-L23

matthewfeickert commented 2 years ago

As nrCascadeSim can currently only be compiled (I think, so let me know if you have a c++17 version I should be using) against ROOT releases that were also compiled against c++14 can you recommend an CVMFS LCG view that you've used to test this that has c++14 ROOT so I can keep testing now while c++17 support is considered?

The LCG view that I'm most used to setting up is LCG view 98python3 with x86_64-centos7-gcc8-opt but that gives me a c++17 ROOT:

$ . /cvmfs/sft.cern.ch/lcg/views/LCG_98python3/x86_64-centos7-gcc8-opt/setup.sh 
$ command -v root
/cvmfs/sft.cern.ch/lcg/views/LCG_98python3/x86_64-centos7-gcc8-opt/bin/root
$ root --version
ROOT Version: 6.22/00
Built for linuxx8664gcc on Jun 14 2020, 15:54:05
From tags/v6-22-00@v6-22-00
$ root-config --cflags
-pthread -std=c++17 -m64 -I/cvmfs/sft.cern.ch/lcg/releases/ROOT/v6.22.00-be0a0/x86_64-centos7-gcc8-opt/include

villaa commented 2 years ago

I think because of the merging of #54 we should be OK with C++14 or C++17, but should now explicitly test C++17 in the CI. After upgrading that will consider this issue closed.

villaa commented 2 years ago

Closed with PR #57 where we work in all compilers ranging from c++11 to c++17 into Travis-CI

villano-lab / nrCascadeSim

[JOSS Pre-Review]: Hardcoded compiler assumptions in Makefile #45