rcedgar / muscle

Multiple sequence and structure alignment with top benchmark scores scalable to thousands of sequences. Generates replicate alignments, enabling assessment of downstream analyses such as trees and predicted structures.
https://drive5.com/muscle
GNU General Public License v3.0
186 stars 21 forks source link

Aarch64 muscle built from source fails #52

Closed lukaszsobala closed 5 months ago

lukaszsobala commented 1 year ago

Hello,

Since only a Mac arm64 version is provided, I tried to build my own for a distribution on aarch64 ubuntu.

The build finished with zero errors, but when I ran muscle with no parameters I git this:

---Fatal error---
myutils.cpp(122) assert failed: sizeof(void *) == 4
rcedgar commented 1 year ago

The problem is this test in line 14 of myutils.h:

#if defined(__x86_64__) || defined(_M_X64) || defined(__arm64__)

If you add a preprocessor constant for aarch64 here it might work, suppose it is __aarch64__ then update line 14 to look like this:

#if defined(__x86_64__) || defined(_M_X64) || defined(__arm64__) || defined(__aarch64__)

There should be a compiler option to report a list of pre-defined constants, e.g. cpp -dM test.cpp.

lukaszsobala commented 1 year ago

I changed that line, recompiled and the error is the same. I know next to nothing about programming, so this log won't tell me much, but please tell me what I should post. test.cpp doesn't exist but other files that start with test do.

rcedgar commented 1 year ago

try cpp -dM xxx.cpp > output.txt where xxx is any *.cpp file, the source file must exist but it's actually ignored, post output.txt somewhere I can take a look

lukaszsobala commented 1 year ago

Please take a look at your repository, the only "test" files that are in /src are: testfb.cpp testlog.cpp testscoretype.cpp

Anyway, here are the lists from all three: testfb.txt testlog.txt testscoretype.txt

I hope this is useful!

rcedgar commented 1 year ago

Looks like my guess was right, if you change line 14 in myutils.h to look like this it should work: #if defined(__x86_64__) || defined(_M_X64) || defined(__arm64__) || defined(__aarch64__)

lukaszsobala commented 1 year ago

Yes, sorry, I changed it but made a typo!

Recompiled again... and now it works. Thank you. Should I do any more testing for you? It would be nice if a precompiled aarch64 version was available to download!

rcedgar commented 1 year ago

this would need an aarch64 github action to do the build, that should be very similar to Ubuntu if you add an action and issue a pull request that could work

CaldoSmash commented 7 months ago

Does this solution work? I’m trying to help my girlfriend with her bioinformatics for her degree. I’m an IT technician not a programmer so if I change line 14 in myutils.h and recompile the code should muscle work with aarch64

lukaszsobala commented 7 months ago

Yes. When I compile it locally it works perfectly. I tried to create a PR but anything I tried failed. I don't really know how to use Github actions.

martin-g commented 5 months ago

I will work on a new Github Action CI job for Linux ARM64!

martin-g commented 5 months ago

https://github.com/rcedgar/muscle/pull/73

martin-g commented 5 months ago

By the way at Bioconda Linux ARM64 CI I get this error:

12:37:16 BIOCONDA INFO (OUT) $BUILD_PREFIX/bin/aarch64-conda-linux-gnu-c++ -DNDEBUG -D_FORTIFY_SOURCE=2 -O2 -isystem $PREFIX/include -DNDEBUG -pthread -fvisibility-inlines-hidden -fmessage-length=0 -ftree-vectorize -fPIC -fstack-protector-strong -fno-plt -O3 -pipe -isystem $PREFIX/include -fdebug-prefix-map=$SRC_DIR=/usr/local/src/conda/muscle-5.1 -fdebug-prefix-map=$PREFIX=/usr/local/src/conda-prefix -O3 -fopenmp -ffast-math -c -o Linux/testlog.o testlog.cpp
12:37:16 BIOCONDA INFO (OUT) In file included from testlog.cpp:2:
12:37:16 BIOCONDA INFO (OUT) In function 'uint64_t GetClockTicks()',
12:37:16 BIOCONDA INFO (OUT)     inlined from 'void cmd_testlog()' at testlog.cpp:71:26:
12:37:16 BIOCONDA INFO (OUT) timing.h:26:9: error: impossible constraint in 'asm'
12:37:16 BIOCONDA INFO (OUT)    26 |         __asm__ __volatile__ ("rdtsc" : "=a" (lo), "=d" (hi));
12:37:16 BIOCONDA INFO (OUT)       |         ^~~~~~~
12:37:16 BIOCONDA INFO (OUT) In function 'uint64_t GetClockTicks()',
12:37:16 BIOCONDA INFO (OUT)     inlined from 'void cmd_testlog()' at testlog.cpp:83:26:
12:37:16 BIOCONDA INFO (OUT) timing.h:26:9: error: impossible constraint in 'asm'
12:37:16 BIOCONDA INFO (OUT)    26 |         __asm__ __volatile__ ("rdtsc" : "=a" (lo), "=d" (hi));
12:37:16 BIOCONDA INFO (OUT)       |         ^~~~~~~

I don't get this error at Github Actions and on my local build ...

rcedgar commented 5 months ago

This is coming from the GetClockTicks() function defined in timing.h, it's very old code from a time when it was tricky to get get the value of the CPU clock (RDTSC instruction). GCC versions >=5 have an __rdtsc() intrinsic, if you edit timing.h to replace the assembler with the intrinsic for the appropriate version of GetClockTicks() it will probably work. The problem with this fix is that it will be necessary to re-test all the build variations on other platforms.

martin-g commented 5 months ago

Thanks, @rcedgar ! The problem is that I cannot reproduce it easily, so I don't feel confident changing the code. I am trying to find what is different at Bioconda CI.

martin-g commented 5 months ago

I found the reason! Bioconda uses tag 5.1 - https://github.com/rcedgar/muscle/blame/v5.1/src/timing.h But main and 5.1.0 has this https://github.com/rcedgar/muscle/commit/d5b2c520998571b00b19fdfa2d4d0a2ad9b850c6 - it disables the content inside if / endif

martin-g commented 5 months ago

If you plan to tag a new version with the improvement in myutils.h then I will wait for it! Otherwise I could use either 5.1.0 or a .patch for both myutils.h and timings.h!

rcedgar commented 5 months ago

Sorry, I don't follow, which improvement in myutils.h, and what do you mean by "tag"? If you mean post a new release, then I have no plans to do that in the near future.

martin-g commented 5 months ago

I meant the addition of aarch64 in the PR [https://github.com/rcedgar/muscle/pull/73] that fixes the build on Linux ARM64.

And yes, I meant a new Git tag, i. e. a release.

On Sat, 16 Mar 2024 at 17:59, Robert Edgar @.***> wrote:

Sorry, I don't follow, which improvement in myutils.h, and what do you mean by "tag"? If you mean post a new release, then I have no plans to do that in the near future.

— Reply to this email directly, view it on GitHub https://github.com/rcedgar/muscle/issues/52#issuecomment-2002029367, or unsubscribe https://github.com/notifications/unsubscribe-auth/AABYUQUEQKXDKYKO5ZO3LVLYYRT7DAVCNFSM6AAAAAAV3ATIT6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAMBSGAZDSMZWG4 . You are receiving this because you commented.Message ID: @.***>

martin-g commented 5 months ago

Bioconda now provides linux-aarch64 build for muscle - https://anaconda.org/bioconda/muscle ! https://github.com/bioconda/bioconda-recipes/pull/46525