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

bam2cns doesn't handle spaces in the reference path #116

Open
ressy opened this issue Jan 3, 2018 · 6 comments
Open

bam2cns doesn't handle spaces in the reference path #116

ressy opened this issue Jan 3, 2018 · 6 comments

Comments

@ressy
Copy link

ressy commented Jan 3, 2018

bam2cns bombs out if the path to the reference file contains whitespace (I know, not my idea of a good filename either) because of how glob() works here at line 244:

...
if($opt_ref_pat){
	($opt_ref_file) = glob($opt_ref_pat);
	$v->exit("Reference file not found ($opt_ref_pat)") unless $opt_ref_file;
...

It doesn't get caught there since there's some text in $opt_ref_file, but later in lib/Fastq/Parser.pm when it tries to read the file.

Maybe something like this would work instead?

	($opt_ref_file) = glob("'${opt_ref_pat}'");

I can make that a pull request if it's helpful. Thanks for proovread by the way!

@thackl
Copy link
Contributor

thackl commented Jan 4, 2018

Cool, thanks for reporting the issue. I'm guessing you have tried your solution - I'm surprised this is the only thing that breaks with spaces in paths. I have to admit, I never tested for that.

If it works, I'm happy to merge a pull request.

@ressy
Copy link
Author

ressy commented Jan 9, 2018

Well, it works for me, but I'm not much of a Perl person so here's some more detail to hopefully show my thinking makes sense. (And I wouldn't have noticed that either, except I had a script download a set of reference sequences and name them automatically, and one of the names happened to have a space.)

Before making any changes to the code, it works so long as there are no spaces or if I double-escape them for both the shell and Perl. For example these first two commands run OK (in the second one bash takes the path with a space as a single argument and then presumably Perl understands that backslash) but the third one fails when it sets the reference path to just "example".

$ proovread/bin/bam2cns --bam example_reads.bam --ref example_reference.fasta --prefix example_output
$ proovread/bin/bam2cns --bam example_reads.bam --ref "example\ reference.fasta" --prefix example_output
$ proovread/bin/bam2cns --bam example_reads.bam --ref "example reference.fasta" --prefix example_output
Fastq::Parser::new: example, No such file or directory at <path>/proovread/bin/../lib/Fastq/Parser.pm line 229, <SAM> line 3.
Use of uninitialized value in numeric eq (==) at <path>/proovread/bin/../lib/Fastq/Parser.pm line 694, <SAM> line 3.
        (in cleanup) Can't use an undefined value as a symbol reference at <path>/proovread/bin/../lib/Fastq/Parser.pm line 246, <SAM> line 3.

Then, if I change that glob() line like I described, any of those variants work.

@ressy
Copy link
Author

ressy commented Jan 9, 2018

OK, pull request created. Oh and I'm on Perl 5.22.1 in an Anaconda 3 environment on Ubuntu.

@ressy
Copy link
Author

ressy commented Jan 11, 2018

Whoops, looks like that change breaks bam2cns for Perl < 5.18.3 (thanks Travis). I'll see if there's an easy fix for that, but if not I'll just cancel the PR for the time being.

@thackl
Copy link
Contributor

thackl commented Jan 11, 2018

Yeah, too bad. Thanks for making the effort, and sorry are now getting dragged into the fabulous world of Perl madness ;). Don't waste too much time on it, though. I can take a look at it next week as well.

@ressy
Copy link
Author

ressy commented Jan 18, 2018

Yeah, I just closed the pull request for now. If I start digging into Perl more I'll get back to you!

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

No branches or pull requests

2 participants