search

BenLangmead / bowtie

An ultrafast memory-efficient short read aligner
Other
257 stars 76 forks source link

Add aarch64, ppc64le and s390x cases to Travis CI #102

Closed junaruga closed 3 years ago

junaruga commented 4 years ago

I have a suggestion.

This PR is to add aarch64, ppc64le and s390x cases to Travis CI, updating Makefile to build each arch easily. It is related to https://github.com/BenLangmead/bowtie/pull/13 and https://github.com/BenLangmead/bowtie/pull/95 . How do you think?

There are 2 commits in the PR. First commit is to fiix following error on s390x.

alphabet.cpp:293:1: error: narrowing conversion of ‘-1’ from ‘int’ to ‘char’ inside { } [-Wnarrowing]

This prevents to build on s390x. You can see this Travis log on my forked repository for detail.

Here is my forked repository's Travis CI result.

One note is right now for s390x, only make allall is executed because make simple-test fails. You can see this Travis log for detail.

./bowtie-build  --threads 2 --quiet --sanity  .simple_tests.pl.fa .simple_tests.tmp
Bad exitlevel from bowtie-build: 139 at ./scripts/test/simple_tests.pl line 1141.

Dear maintainers, do you have any idea to fix this?

@mr-c do you know how you built bowtie for Debian bowtie s390x package? Have you run the make simple-test on s390x? Do we need to apply one of the Debian patche files to this repository?

Best regards, Jun

junaruga commented 4 years ago

This repository's Travis builds are running here. But the link is not shown in this page. I do not know why. https://travis-ci.org/BenLangmead/bowtie/builds/633533222

junaruga commented 4 years ago

I found another pull-request fixing the issue in alphabet.cpp and adding aarch64 support here. https://github.com/BenLangmead/bowtie/pull/95

mr-c commented 4 years ago

@junaruga In Debian, we fail at building bowtie2 on s390x, armel, armhf, i386, and mipsel along with some unsupported architectures: https://buildd.debian.org/status/package.php?p=bowtie2

I will try your patch for s390x!

junaruga commented 4 years ago

@mr-c Thanks for sharing the building status about bowtie2. This PR is for bowtie, not for bowtie2. I think the alphabet.cpp's patch fixes issue for bowtie2 too. So, seeing the status about bowtie. It seems the you succeeded at building bowtie at s390x, right? Have you been running make simple-test in the building process? https://buildd.debian.org/status/package.php?p=bowtie

mr-c commented 4 years ago

@junaruga Sorry for the bowtie/bowtie2 confusion. I didn't work on the Debian bowtie package, so I can't say much. Looking at the patches the biggest thing is that the embedded copy of seqan was upgraded to seqan 1.4.2 and seqan's portable popcount method was used instead of bowtie's.

Have you been running make simple-test in the building process?

No, but I will try that

mr-c commented 4 years ago

@junaruga so s390x builds, and most of the tests pass, but bowtie 1.2.3+dfsg-3 segfaults with 2+ threads on s390x: https://buildd.debian.org/status/fetch.php?pkg=bowtie&arch=s390x&ver=1.2.3%2Bdfsg-3&stamp=1578504914&file=log

mr-c commented 4 years ago

(a qemu s390x build passed the tests when threads are limited to just one)

junaruga commented 4 years ago

@mr-c Thank you for checking it on Debian build system. It's interesting that the behavior of the Debian's s390x tests is different from the Travis s390x. And the threads and QEMU information are useful too.

junaruga commented 4 years ago

I didn't work on the Debian bowtie package, so I can't say much. Looking at the patches the biggest thing is that the embedded copy of seqan was upgraded to seqan 1.4.2 and seqan's portable popcount method was used instead of bowtie's.

And also thank you for sharing the situation in Debian.

mr-c commented 4 years ago

@junaruga You are welcome. I'm sure the different result is due to the extensive patching we did to update to newer Seqan. I'm happy to contribute the patches to this repo, if there is interest and willingness to merge them.

junaruga commented 4 years ago

Hi @ch4rr0 Congrats, new release for bowtie2. I rebased this PR aligning it with bowtie2 master's .traivs.yml and Makefile , adding gcc-N case, osx case and other CPU architecture cases.

Here is the Travis CI result.

Some notes

$ git diff
diff --git a/.travis.yml b/.travis.yml
index 6abc003..8732fe4 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -52,8 +52,6 @@ matrix:
       env:
         - POPCNT_CAPABILITY=0
         - NO_TBB=1
-      # Skip make simple-test as it fails.
-      script: make -j $NPROC allall
     # PPC64LE
     - os: linux
       arch: ppc64le
@@ -61,8 +59,10 @@ matrix:
       env:
         - POPCNT_CAPABILITY=0
         - NO_TBB=1
-      # Skip make simple-test as it fails.
-      script: make -j $NPROC allall
+  allow_failures:
+    - arch: s390x
+    - arch: ppc64le
+  fast_finish: true
 cache: apt
 env:
   global:

MACS's .travis.yml and Travis result might be a good example for you to see the allow_failures syntax of .travis.yml.

I wish you like this PR.

junaruga commented 4 years ago

But in my understanding, as those are not executed from Travis make simple-test, I did not use language: python to run it on Python 3 for the test cases.

Sorry my mistake. Python is used in bowtie, bowtie-build and bowtie-inspect as bowtie2 as well. But modifying the code from python to python3 could be another PR. I would prefer this PR is for only .travis.yml and Makefile.

$ grep -rl python *
bowtie
bowtie-build
bowtie-inspect
scripts/test/large_idx.py
scripts/test/build_big.py
scripts/test/btdata.py
scripts/test/dataface.py
scripts/test/btface.py
ch4rr0 commented 4 years ago

I copied over the .travis.yml you contributed to the bowtie2 repo. With a few minor changes we see that all builds and simple-tests succeed with the exception of s390x. I’ll investigate why that is.

ch4rr0 commented 4 years ago

I fixed the s390x bug. The simple tests now pass for all architectures.

junaruga commented 3 years ago

Thanks! I close this PR, as Travis has the 3 architecture cases by https://github.com/BenLangmead/bowtie/commit/d20447f6ad1865d35486adeae8c41f17d38ed325 .

I fixed the s390x bug. The simple tests now pass for all architectures.

@ch4rr0 I assume the fix is this commit. https://github.com/BenLangmead/bowtie/commit/4fbac0eb4206c9f68248c6f8bd732d7bf07cf1fa

How did you fix it? Because if you did not have the native local s390x machine, it was hard to debug and fix it.

ch4rr0 commented 3 years ago

I used the s390x instance in Travis to help debug and fix the issue. Since I don’t have direct access to the node I used print statements to verify inputs/outputs.