Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

spades: try to remove spades-bwa #212

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

alienzj
Copy link
Member

@alienzj alienzj commented Jul 8, 2024

Involved packages

  • spades

Involved issue

Close # 7f21e90#commitcomment-143917186

Details

  • Tested in the local machine (largest 16G RAM) and it is passed without any issue
  • Provide New Package
  • Fix the Packages
    • PKGBUILD
    • lilac.yaml
    • lilac.py
  • Would like to continue to work with us

Additional Note

  1. add gcc to makedepends
  2. update cmake parameters refer to https://github.com/ablab/spades/blob/main/spades_compile.sh#L225

@@ -12,10 +12,11 @@ depends=(
python
)
makedepends=(
bzip2
gcc
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

gcc is the default compiler, I don't think we need to add it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I used makepkg to test building process, then I got some errors.
After installing gcc, it works. That's why I added gcc into PKGBUILD.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But have you tried bioarchlinux-x86_64-build?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

gcc is a dependency of base-devel, so it should not be in makedepends. From Arch wiki:

The package base-devel is assumed to be already installed when building with makepkg. Dependencies of this package should not be included in makedepends array.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. Removed gcc.

@starsareintherose
Copy link
Member

my question is that the spades-bwa is same with bwa, why not use ln -s to solve it?

@alienzj
Copy link
Member Author

alienzj commented Jul 8, 2024

Sorry for my misunderstanding.

From the CMakeLists.txt file https://github.com/ablab/spades/blob/main/ext/src/bwa/CMakeLists.txt#L33, it appears that SPAdes does not use the most recent version of BWA, but instead includes a method to build BWA as part of the SPAdes compilation process. This approach simplifies the installation for users, as they can compile SPAdes and have BWA built alongside it without needing to separately install BWA.

Arch Linux users, embracing the KISS principle, can opt for a simplified SPAdes installation process. If the latest BWA is compatible with SPAdes, we can remove the integrated BWA build from SPAdes and instead provide a symbolic link from spades-bwa to the latest BWA installation.

Let me try this week.
Thanks.

@alienzj
Copy link
Member Author

alienzj commented Jul 8, 2024

[ 96%] Building CXX object projects/spades/CMakeFiles/spades-core.dir/main.cpp.o
[ 97%] Building CXX object projects/spades/CMakeFiles/spades-core.dir/series_analysis.cpp.o
[ 97%] Building CXX object projects/spades/CMakeFiles/spades-core.dir/__/mts/contig_abundance.cpp.o
[ 97%] Linking CXX executable ../../bin/spades-core
/usr/bin/ld: ../../common/alignment/libalignment.a(bwa_index.cpp.o): in function `alignment::BWAIndex::AlignSequence(Sequence const&, bool) const':
bwa_index.cpp:(.text+0x1f2f): undefined reference to `mem_align1_bin'
collect2: error: ld returned 1 exit status
make[2]: *** [projects/spades/CMakeFiles/spades-core.dir/build.make:186: bin/spades-core] Error 1
make[1]: *** [CMakeFiles/Makefile2:2808: projects/spades/CMakeFiles/spades-core.dir/all] Error 2
make: *** [Makefile:166: all] Error 2
make: Leaving directory '/home/alienzj/toolkits/ohbioarchlinux/bioarchlinux/Bioarchlinux-Packages/BioArchLinux/spades/src/build'
==> ERROR: A failure occurred in build().
    Aborting..
    

After removing spades-bwa building process, there is a undefined reference to mem_align1_bin.

@alienzj
Copy link
Member Author

alienzj commented Jul 8, 2024

Unfortunately,
SPAdes used:
https://github.com/ablab/spades/blob/main/src/common/alignment/bwa_index.cpp#L377
https://github.com/ablab/spades/blob/main/ext/include/bwa/bwamem.h#L153

BWA used: https://github.com/lh3/bwa/blob/master/bwamem.h

The API is not the same one.
It is difficult to use lh3's BWA directly at this moment.

@starsareintherose
Copy link
Member

Okay, great, thanks for your testing

@alienzj alienzj changed the title add GCC and update cmake Try to remove spades-bwa Jul 9, 2024
@starsareintherose
Copy link
Member

you said not compatible, but why do you now use ln?

@alienzj
Copy link
Member Author

alienzj commented Jul 9, 2024

I checked the API more in depth, basically, mem_align1 did same thing when compared to mem_align1_bin.

@starsareintherose
Copy link
Member

Have you tested it with example data?

@starsareintherose starsareintherose changed the title Try to remove spades-bwa spades: try to remove spades-bwa Aug 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants