rrwick / Porechop

adapter trimmer for Oxford Nanopore reads
GNU General Public License v3.0
323 stars 124 forks source link

C++14 requirement #29

Closed jvolkening closed 6 years ago

jvolkening commented 6 years ago

Can you tell me what parts of the code require C++14 support? I'm trying to get this on bioconda to make Galaxy packaging easier, but the GCC currently packaged by bioconda doesn't support C++14. I'm wondering whether a patch to get around the C++14 requirement would be a short-term solution until bioconda supports GCC 4.9.

rrwick commented 6 years ago

I'm afraid I may not be much help - I use Seqan to do the alignment, and this guide says that C++14 is necessary.

Now I of course only use a bit of Seqan in Porechop, so it may be that the parts which C++14 bits aren't used much in Porechop and only a few edits are required. Or it may be harder than that...

As a quick experiment I replaced -std=c++14 with -std=c++11 in the Porechop makefile and did get a handful of errors like this: porechop/include/seqan/modifier/modifier_padding.h:195:8: error: 'auto' return without trailing return type; deduced return types are a C++14 extension and this: porechop/include/seqan/align/align_interface_wrapper.h:79:18: error: 'auto' not allowed in lambda parameter

So I fear it won't be a quick fix!

rrwick commented 6 years ago

I'm not at all savvy with bioconda, but this seems to be a common problem. It isn't the first time I've encountered this kind of issue, and a quick Google search reveals others in the same boat: https://github.com/bioconda/bioconda-recipes/issues/3224 Hopefully GCC 5+ will make its way in soon!

Since, I'm not going to try to get rid of Porechop's C++14 requirement, I'm going to close this issue now. But if you make any headway on the matter, I'm be interested to know. Thanks!

Ryan

jvolkening commented 6 years ago

Since, I'm not going to try to get rid of Porechop's C++14 requirement, I'm going to close this issue now.

No problem. I was hoping there might be a quick workaround, but I can wait for an updated GCC in bioconda.

samfux84 commented 6 years ago

@jvolkening Maybe it is possible to replace the Seqan 2.2 headers with Seqan 2.1 headers. Seqan 2.1 does not require C++14.

I would also like to install Porechop and the C++14 requirement breaks my toolchain (which uses GCC 4.8.2). I had a simliar issue with another software, that was depending on a part of seqan. In that case the solution of using an older seqan version worked.

Either this will work, or I will recommend the users of our HPC cluster to use a different tool.

samfux84 commented 6 years ago

I have replaced Porechop/porechop/include/seqan with the seqan folder from seqan 2.1.0 and replaced -std=c++14 with -std=c++11 in the Makefile.

Installation so far worked:


[sfux@eu-c7-001-01 Porechop]$ python setup.py install
running install
running build
running build_py
Cleaning previous compilation: make clean
rm -f porechop/src/adapter_align.o porechop/src/alignment.o
Compiling Porechop: make -j 8
/cluster/apps/gcc/4.8.2/bin/g++ -std=c++11 -Iporechop/include -fPIC -O3 -D NDEBUG -Wall -Wextra -pedantic -mtune=native -c -o porechop/src/adapter_align.o porechop/src/adapter_align.cpp
/cluster/apps/gcc/4.8.2/bin/g++ -std=c++11 -Iporechop/include -fPIC -O3 -D NDEBUG -Wall -Wextra -pedantic -mtune=native -c -o porechop/src/alignment.o porechop/src/alignment.cpp
/cluster/apps/gcc/4.8.2/bin/g++ -std=c++11 -Iporechop/include -fPIC -O3 -D NDEBUG -Wall -Wextra -pedantic -mtune=native -shared -Wl,-soname,porechop/cpp_functions.so -o porechop/cpp_functions.so porechop/src/adapter_align.o porechop/src/alignment.o
running install_lib
running install_egg_info
running egg_info
writing porechop.egg-info/PKG-INFO
writing dependency_links to porechop.egg-info/dependency_links.txt
writing entry points to porechop.egg-info/entry_points.txt
writing top-level names to porechop.egg-info/top_level.txt
reading manifest file 'porechop.egg-info/SOURCES.txt'
reading manifest template 'MANIFEST.in'
writing manifest file 'porechop.egg-info/SOURCES.txt'
removing '/cluster/apps/gdc/python/3.6.1/lib64/python3.6/site-packages/porechop-0.2.3-py3.6.egg-info' (and everything under it)
Copying porechop.egg-info to /cluster/apps/gdc/python/3.6.1/lib64/python3.6/site-packages/porechop-0.2.3-py3.6.egg-info
running install_scripts
Installing porechop script to /cluster/apps/gdc/python/3.6.1/bin
[sfux@eu-c7-001-01 Porechop]$

I will ask our users to test and provide some feedback.

jvolkening commented 6 years ago

@samfux84 Do the Porechop unit tests pass under this build?

samfux84 commented 6 years ago

I was compiling in an interactive job on our HPC cluster. Unfortunately I have finished the interactive session and therefore deleted the source directory, as all required parts have been copied to the site-packages directory of our Python 3.6.1 installation. Afterwards I found out that there are unittests.

Since then I am trying to figure out, how to run the unittests without having the source directory (just from the installed parts in the site-packages directory).

When just downloading the test directory from git, placing it somewhere and running "python -m unittest", then all tests fail because they cannot find the required files. For instance:

======================================================================
ERROR: test_albacore_directory_1 (test_albacore_directory.TestAlbacoreDirectory)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/scratch/appssf/test/test_albacore_directory.py", line 61, in test_albacore_directory_1
    bc01_trimmed_reads = self.load_trimmed_reads('BC01.fastq')
  File "/scratch/appssf/test/test_albacore_directory.py", line 45, in load_trimmed_reads
    trimmed_reads, read_type = porechop.misc.load_fasta_or_fastq(full_filepath)
  File "/cluster/apps/gdc/python/3.6.1/lib64/python3.6/site-packages/porechop/misc.py", line 114, in load_fasta_or_fastq
    file_type = get_sequence_file_type(filename)
  File "/cluster/apps/gdc/python/3.6.1/lib64/python3.6/site-packages/porechop/misc.py", line 89, in get_sequence_file_type
    sys.exit('Error: could not find ' + filename)
SystemExit: Error: could not find TEMP_120216/BC01.fastq

I would like to test with the unittests, but I don't know where to get the required files ...

jvolkening commented 6 years ago

I am trying a local build here using seqan 2.1 and will see if they pass.

jvolkening commented 6 years ago

The unit tests pass. It would be great to have an official tag of this as Porechop could then go into bioconda.

I have created a branch here which simply makes the modifications described by @samfux84 and adds a suffix to the version. I would propose adding this as a separate stable branch to the main Porechop repo. I haven't submitted a PR because I believe the new branch would need to be created here first for me to choose it as a target.

@rrwick how does this sound to you?

rrwick commented 6 years ago

Sounds good to me! I've just made a branch called C++11, which you can use as the target for your PR.

Ryan