NVIDIA-Genomics-Research / GenomeWorks

SDK for GPU accelerated genome assembly and analysis
https://clara-parabricks.github.io/GenomeWorks/
Apache License 2.0
286 stars 76 forks source link

[cmake] unify cmake cxx flags across GW #617

Closed tijyojwad closed 3 years ago

tijyojwad commented 3 years ago

Centralize the cmake flags to ensure all modules inherit common configurations.

Closes #291

mimaric commented 3 years ago

Good for me as well, but one test is failing and complains about min() and max() for 16-bit in common/base/include/claraparabricks/genomeworks/utils/limits.cuh

tijyojwad commented 3 years ago

rerun tests

tijyojwad commented 3 years ago

Hey @ahehn-nv and @mimaric - I'm running into a gcc error after the merge from https://github.com/clara-parabricks/GenomeWorks/pull/626 which added libcudaxx dependency. The -Werror now kicks in with an unused argument error for the following -

/jenkins/workspace/clara-genomics/gpuci/clara-genomics-analysis/prb/clara-genomics-analysis-cpu-build/3rdparty/libcudacxx/include/cuda/std/atomic:161:73: error: unused parameter ‘_Scope’ [-Werror=unused-parameter]

 inline __host__ __device__ void atomic_thread_fence(memory_order __m, thread_scope _Scope = thread_scope_system) {

                                                                         ^

cc1plus: all warnings being treated as errors

CMake Error at cudaaligner_generated_myers_gpu.cu.o.Release.cmake:280 (message):

  Error generating file

  /jenkins/workspace/clara-genomics/gpuci/clara-genomics-analysis/prb/clara-genomics-analysis-cpu-build/build/cudaaligner/CMakeFiles/cudaaligner.dir/src/./cudaaligner_generated_myers_gpu.cu.o

Now this shows up in the core cudaaligner compilation, so I had to ignore this particular check in myers_gpu.cu. Let me know if there's a better way to handle this.

ahehn-nv commented 3 years ago

Not sure if there is a better way. If I remember correctly, disabling warnings like that was recommended by the "C++ Coding Standards" book, right? Personally, I don't really like putting things like this in the code, especially since they are compiler specific. It is also likely to stay there forever, because you never check if it is still needed. I'd just remove the -Werror from the compilation flags, live with the warnings that are beyond our control and make sure that our code compiles warning free manually. I.e. fix a warning whenever we see one and make all regular contributors aware that their code should compile warning free (and use git blame extensively! ;) ). But that's just my personal view.

tijyojwad commented 3 years ago

I agree that once it goes it, it's hard to take out especially because there's no frequent review of these exceptions. That said, I personally prefer to err on the side of caution and leave the strict checking with -Werror in there since it forces us to evaluate what's causing the warning. With best effort, I've rarely seen a consistent effort put in by all developers to ensure warning free compilation. Since it's the recommended way, I'd say let's keep it as is and if we find ourselves adding a whole lot of exceptions, then we can revisit taking out -Werror :).