pylelab / USalign

Universal Structure Alignment of Monomeric and Complex Structure of Nucleic Acids and Proteins
https://zhanggroup.org/US-align/
Other
109 stars 24 forks source link

Fix compilation issues and include a LICENSE file #18

Closed diegozea closed 1 year ago

diegozea commented 1 year ago

Hello, I was trying to create a JLL package for USalign using Julia's BinaryBuilder, but I have found some problems that this PR fixes. In particular, it avoids variable names ending by numbers on TMalign.h and adds a LICENSE.md file (using this BSD-2-Clause template). Another thing that could be useful, but is not mandatory, is to create a release with a major.minor.patch version. Right now, I am identifying the version using the commit number, and I will use 0.0.1 for the JLL package until a real version is given to this package. Is that ok with you? Best regards,

kad-ecoli commented 1 year ago

Sorry, I cannot accept your pull request.

First, the c++ code of US-align is not under BSD license. The license of US-align is at the top of the readme file. A copy is also available now at https://github.com/pylelab/USalign/blob/master/LICENSE. This license is an open source license compliant to the Debian Free Software Guidelines. While it looks similar to BSD license, it additionally request inclusion of citations to the US-align papers. The pymol plugin (usalign.py) is under BSD license, which does not apply to the rest of the package.

Second, I do not understand why you need to avoid variable names ending by numbers as this is not required by any C++ standard. We have compiled US-align successfully on g++ and clang++, both of which are supported by BinaryBuilder.

Third, the version name of US-align is based on the release date. The most recent version is 20220924. The version number is print out when you run US-align. If you have to use a major.minor.patch version, can you use 2022.09.24 instead?

diegozea commented 1 year ago

Thanks a lot for your return and clarification. I overlooked the license in the readme. The problem with the variable names is only using g++ in Linux powerpc64le, but it works perfectly in all the other platforms. I have another question: What do you do to compile cif2pdb in Windows, avoiding the compile error because of the handling of compressed files?

diegozea commented 1 year ago

Compiling pdbAtomName on Windows gives the following error that is solved by adding #include <cstdlib> at the beginning of the pdbAtomName.cpp file:

[11:38:24] pdbAtomName.cpp: In function ‘void print_help()’:
[11:38:24] pdbAtomName.cpp:18:10: error: ‘EXIT_SUCCESS’ was not declared in this scope
[11:38:24]      exit(EXIT_SUCCESS);
[11:38:24]           ^
[11:38:24] make: *** [Makefile:52: pdbAtomName] Error 1
diegozea commented 1 year ago

To allow cross-compiling for Windows, I added a dummy pstream.h file during the process on that platform.

diegozea commented 1 year ago

The problem with the variable names is only using g++ in Linux powerpc64le, but it works perfectly in all the other platforms.

This is the error message for g++ in powerpc64le:

[14:52:54] TMalign.h: In function ‘void make_sec(char*, double**, int, char*, std::string)’:
[14:52:54] TMalign.h:856:20: error: expected unqualified-id before numeric constant
[14:52:54]      vector<int> A0,B0,C0,D0;
[14:52:54]                     ^
[14:52:54] In file included from se.h:1:0,
[14:52:54]                  from MMalign.h:2,
[14:52:54]                  from USalign.cpp:3:
[14:52:54] TMalign.h:871:16: error: request for member ‘push_back’ in ‘0’, which is of non-class type ‘int’
[14:52:54]              B0.push_back(j);
[14:52:54]                 ^
[14:52:54] TMalign.h:872:13: error: ‘C0’ was not declared in this scope
[14:52:54]              C0.push_back(ii);
[14:52:54]              ^
[14:52:54] TMalign.h:873:13: error: ‘D0’ was not declared in this scope
[14:52:54]              D0.push_back(jj);
[14:52:54]              ^
[14:52:54] TMalign.h:907:24: error: ‘C0’ was not declared in this scope
[14:52:54]              if(A0[i]+j>C0[i]) break;
[14:52:54]                         ^
[14:52:54] TMalign.h:909:17: error: ‘D0’ was not declared in this scope
[14:52:54]              sec[D0[i]+j]='>';
[14:52:54]                  ^
[14:52:54] TMalign.h:916:8: error: request for member ‘clear’ in ‘0’, which is of non-class type ‘int’
[14:52:54]      B0.clear();
[14:52:54]         ^
[14:52:54] TMalign.h:917:5: error: ‘C0’ was not declared in this scope
[14:52:54]      C0.clear();
[14:52:54]      ^
[14:52:54] TMalign.h:918:5: error: ‘D0’ was not declared in this scope
[14:52:54]      D0.clear();
[14:52:54]      ^
diegozea commented 1 year ago

It is finally cross-compiling in all the supported platforms. This is the full script: https://github.com/JuliaPackaging/Yggdrasil/blob/master/U/USalign/build_tarballs.jl :)

The final JLL package is: https://github.com/JuliaBinaryWrappers/USalign_jll.jl

kad-ecoli commented 1 year ago

I have manually merged your changes in var names in this commit: https://github.com/pylelab/USalign/commit/c1e89abedee5061a13a73fa909777e8064e29998 Unfortunately, I will have to reject your license file.

diegozea commented 1 year ago

Thanks! Sure, there's no problem :) About having Windows support; could it be possible to use #ifdef for pstream.h to simply avoid the feature of working with compressed files in Windows while keeping all the rest?

kad-ecoli commented 1 year ago

We indeed used #ifdef for conditional compilation. What compiler are you using? I compile USalign by Mingw-w64 on windows and it works just fine.

kad-ecoli commented 1 year ago

I just checked that you also use mingw-w64 at https://github.com/JuliaBinaryWrappers/USalign_jll.jl it should work without modification. What is the error you get when compiling for windows?