snugel / cas-offinder

An ultrafast and versatile algorithm that searches for potential off-target sites of CRISPR/Cas-derived RNA-guided endonucleases.
Other
84 stars 27 forks source link

Cannot compile 3.0.0.b (develop branch) on macOS 10.14.6 #35

Closed richardkmichael closed 3 years ago

richardkmichael commented 3 years ago

I noticed development is starting on 3.0.0b on the develop branch, and I wanted to test it.

I am unable to compile it on macOS 10.14.6 (clang 10), due to a problem with the new add_compare macro:

source/cas-offinder/cas-offinder.cpp:559:4: error: expected
      '(' for function-style cast or type construction
                        add_compare(string(m_dnabulgesize, 'N') + sline[0], ci, bi);
                        ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
source/cas-offinder/cas-offinder.cpp:13:49: note: expanded from
      macro 'add_compare'
                m_compares[a] = make_pair(b, vector<bulgeinfo>{c});\
                                             ~~~~~~~~~~~~~~~~~^
source/cas-offinder/cas-offinder.cpp:564:6: error: expected
      '(' for function-style cast or type construction
  ...add_compare(string(preNcnt, 'N') + sline[0].substr(0, j) + string(i, 'N') + sline[0].substr(j), ci, bi);
     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
source/cas-offinder/cas-offinder.cpp:13:49: note: expanded from
      macro 'add_compare'
                m_compares[a] = make_pair(b, vector<bulgeinfo>{c});\
                                             ~~~~~~~~~~~~~~~~~^
source/cas-offinder/cas-offinder.cpp:571:6: error: expected
      '(' for function-style cast or type construction
  ...add_compare(string(preNcnt, 'N') + sline[0].substr(0, j) + sline[0].substr(j + i), ci, bi);
     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
source/cas-offinder/cas-offinder.cpp:13:49: note: expanded from
      macro 'add_compare'
                m_compares[a] = make_pair(b, vector<bulgeinfo>{c});\
                                             ~~~~~~~~~~~~~~~~~^
25 warnings and 3 errors generated.
make[2]: *** [CMakeFiles/cas-offinder.dir/cas-offinder.cpp.o] Error 1
make[1]: *** [CMakeFiles/cas-offinder.dir/all] Error 2
make: *** [all] Error 2
 simple-build-changes *$ $ clang++ --version
Apple LLVM version 10.0.1 (clang-1001.0.46.4)
Target: x86_64-apple-darwin18.7.0
Thread model: posix
InstalledDir: /Library/Developer/CommandLineTools/usr/bin
pjb7687 commented 3 years ago

Thanks, I will add Mojave in GA and see what happens...

richardkmichael commented 3 years ago

Thank you for the fast reply!

Also, recently I was unable to compile the master branch due to OpenMP not being linked properly. I am not certain what caused this, but I made changes to CMake to fix it. If you are willing to update to latest cmake, I can send a simple patch for master. I also have a longer patch against develop which does more to simplify the build configuration.

Also, I am wondering about a 3.0 roadmap? I have ideas for a few features I would like to contribute. (If you are curious, I keep track of my ideas as issues in my fork.)

pjb7687 commented 3 years ago

For 3.0, I am just going to add bulge support. No big roadmap from my side, since I am not working at SNU anymore - I am just volunteering to manage the tool. The reason for the major update is due to the output format - it won't be compatible with 2.0.

pjb7687 commented 3 years ago

And thanks for sharing the lists of the issues! I see that some of them are also fixed in develop branch. Of course, you can still freely contribute - this repo is not dead yet.

pjb7687 commented 3 years ago

And - seems the oldest macOS supported by GA is 10.15. I can't test 10.14 since I don't have a mac. But I think what you have seen is basically a syntax error. I guess this might be due to the syntax that I newly introduced, which is only supported in c++11. Is the appleclang 10.0.1 properly configured for c++11?

richardkmichael commented 3 years ago

Yes, I noticed the addition of bulge support (the new typedefs, etc.), great.

What is "GA" ?

Did it compile on macOS 10.15?

I believe it is a syntax error, yes, as the compiler indicates. I do not know C++ well, so I couldn't fix it quickly. This project is my motivation to learn C++/OpenCL. :) I tried inlining the macro and discovering the error, but initializing the vector<bulgeinfo> with a list failed. Perhaps it is also C++11 standard issue.

I'll check on Clang and C++11 (I also noticed it added the the CMake configuration).

I am curious, why did you write add_compare as a macro instead of a function?

(I could add issues to this repo, if you like. I did not want to create feature request issues without making an attempt myself.)

pjb7687 commented 3 years ago

GA: github actions. You can see that compile on 10.15 worked well: https://github.com/snugel/cas-offinder/actions And yes that is the new C++11 syntax. I just wrote the macro to avoid typing the same thing three times.

richardkmichael commented 3 years ago

I should have experimented with this more myself, reading closer now I see list-initialization of aggregates is C++11.

It seems this clang needs -std=c++11, and then it does compile. I added it manually to flags.make just to test quickly.

It is difficult to explain what is going on both on macOS Mojave 10.14 and on latest Catalina 10.15. In both OS versions, the clang man page indicates the default C++ standard is gnu++14. If I add gnu++14 to flags.make, it does compile, so that standard is also compatible with C++11 (not too surprising). I do not yet know why the code compiles on Catalina. :( It has newer Clang (v12), which should not matter in this case. It will probably turn out to be Apple's configuration of Clang.

Unfortunately, the clang website indicates indicates it compiles with C++98 by default!

So something either in Apple's configuration of Clang or in CMake is causing clang++ to use a language standard which does not understand the C++11 syntax. Perhaps I can coerce clang into emitting it's configuration during compile and I can track down what is happening.

I am surprised setting CMAKE_CXX_STANDARD 11 in CMakeLists.txt did not handle this. In any case, I will investigate how to instruct CMake to emit a suitable compiler flag on macOS and send a patch.

FWIW, C++11 was completely supported way back in Clang 3.3. Clang supports all the current standards and some of C++20.

Thanks for the help.

pjb7687 commented 3 years ago

Great! Thanks! Then it seems to be cmake issue. May I ask which version of CMake did you try? I am curious this is fixed in the latest CMake. Then I can just increase the minimum version of CMake required.

richardkmichael commented 3 years ago

I am using latest cmake, 3.19.3.

I updated to the latest cmake when fixing includes for OpenMP. I have that work done and was going to send a patch after I test compiling.

Latest cmake alone does not fix this language standard situation. I will figure that out and reply to this issue. (I am writing a tiny test program using C++11 to try and understand clang configuration on macOS.)

richardkmichael commented 3 years ago

I found the problem.

This cmake configuration line set (CMAKE_CXX_STANDARD 11) must occur before other directives such as add_executable() or find_package() or the -std=gnu++11 flag will be lost (this difference is visible in the generated flags.make). I will confirm this on the cmake help forum.

I will include this patch in my forthcoming PR regarding CMake.

Thank you again for your help!!

richardkmichael commented 3 years ago

Just to be clear:

CMakeLists.txt on develop is fine as it is for compiling C++11 code. (But not for OpenMP code on macOS.)

I introduced a problem when I was fixing a different problem for OpenMP includes on macOS. Now that I understand what I broke, and I have fixed it in my patch.