-
Notifications
You must be signed in to change notification settings - Fork 234
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
Update JBrowse2 to 2.17.0 #1553
Conversation
…m/fubar2/nftoolmaker It actually does work. Sort of. Hope this is useful to someone.
…ply to all future generated tools :)
Allows single ended fastq or fasta to be mapped, or a pair of forward and reverse - both must be fastq
…dio replacement...
Pity the built in fails in biocontainers.
e = os.environ | ||
e["SHELL"] = "/bin/sh" | ||
cmd = ["jbrowse", "text-index"] | ||
subprocess.run(cmd, env=e, shell=True) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is shell=true needed? It's discouraged and has some security implications afaik
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unless the environment has that variable added, there is a problem with node.js trying to read the password file, remember?
We discussed changing the biocontainer env to avoid the need for this but that is an even worse option.
This is the fourth place where that could happen and allows text indexing to be turned back on.
If you really want, this one could be reverted but reverting any of the other three cases at line 760,1114 and 1542 will result in failure trying to read /etc/passwd
. Only two of the four involve any user input - one a gff being sorted and the other a paf being indexed - so buffer overflow might be possible if anyone really wants to try - but it runs in a container...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't remember at all ;) but now that you say it :)
Can you maybe add a comment to this part of the code? Feel free to merge after that.
another update without any breakage of the config generator.