habitat-sim icon indicating copy to clipboard operation
habitat-sim copied to clipboard

Update to use modern CMake practices, tighten warning levels

Open mosra opened this issue 6 years ago • 5 comments

Various good-to-have things to do:

  • [ ] replace include_directories() with target_include_directories(), preferring PRIVATE where possible; if a dependency provides a CMake target use that instead
  • [x] remove use of file(GLOB), list sources explicitly (makes the build more predictable without things getting broken when there are untracked files in the working copy) -- #158
  • [x] use configure_file() instead of add_definitions() -- #143
  • [x] convert if(${FOO}) to just if(FOO) -- #58
  • [ ] look into TODOs with CMAKE_CXX_VISIBILITY_PRESET
  • [x] ~~set up some JOB_POOLS so it's possible to use more cores for compilation without risking OOM issues (currently a compilation of one pybind11 file takes up to 1.8 GB of RAM, we definitely don't want to run that together with 7 other jobs)~~ will not be needed after #64 lands, since it reduces the bindings code quite a lot
  • [ ] enable CORRADE_USE_PEDANTIC_FLAGS for all Habitat code (postponed from #116)
  • [x] ~~go back to CMake 3.5 since that's the oldest version currently in widespread use (Ubuntu 16.04)? -- blocked by tinyply, which requires 3.10 (#139) (which is blocked by magnum's mesh import rework)~~ no need to be that backwards compatible
  • [ ] look into suppressing warnings from dependencies
    • [x] glog has some (or go with #121 instead)
    • [ ] assimp needs -fvisibility-inlines-hidden to fix ld: warning: direct access in function 'Assimp::IOSystem::CurrentDirectory() const' from file 'deps/magnum-plugins/src/MagnumPlugins/AssimpImporter/libAssimpImporter.a(AssimpImporter.cpp.o)' to global weak symbol 'Assimp::IOSystem::CurrentDirectory() const::Dummy' from file 'deps/assimp/code/libassimp.a(Importer.cpp.o)' means the weak symbol cannot be overri dden at runtime. This was likely caused by different translation units being compiled with different visibility settings.

mosra avatar May 10 '19 20:05 mosra

@mosra Can this be closed?

Skylion007 avatar Oct 13 '20 21:10 Skylion007

Why? There's still a few good-to-have items from the list that are not done yet. Feel free to tackle those, if you have time.

mosra avatar Oct 14 '20 12:10 mosra

@mosra any more updates here?

aclegg3 avatar Aug 31 '22 15:08 aclegg3

Hmm. Do we have any other place to list such low-priority tech debt TODOs? In my case (with Magnum repos) I have a bunch of such long-lived "aggregate issues" full of checkboxes that act more as a reminder of what needs to be eventually solved, and I opened this one with a similar intent.

But if the aim here is to only have an issue open if it's likely to be resolved soon, feel free to close.

mosra avatar Aug 31 '22 15:08 mosra

I'll leave this open for now until we establish such a list and then we can migrate the remainder there.

aclegg3 avatar Aug 31 '22 17:08 aclegg3