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

sci-biology/GFF3toolkit: hint for a new package #1068

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

Conversation

mmokrejs
Copy link
Contributor

Something is still broken, please fix it on your own
and cherrypick

Package-Manager: Portage-3.0.17, Repoman-3.0.2
Signed-off-by: Martin Mokrejs [email protected]

Something is still broken, please fix it on your own
and cherrypick

Package-Manager: Portage-3.0.17, Repoman-3.0.2
Signed-off-by: Martin Mokrejs <[email protected]>
@thesamesam
Copy link
Member

What's broken? Can you share some information to help you fix it...?

@mmokrejs
Copy link
Contributor Author

It installs only README file. And also the downloads in setup.py's CustomBuildCommand() look we want to disable them. But maybe they don't get triggered ... I just thought somebody may be faster getting this into shape. Thank you.

The tool uses sometimes blastn and makeblastdb

Package-Manager: Portage-3.0.17, Repoman-3.0.2
Signed-off-by: Martin Mokrejs <[email protected]>
@thesamesam
Copy link
Member

This should be a good place to start

@Nowa-Ammerlaan
Copy link
Member

This package tries to fetch ncbi-blast+, extract it, and place it in gff3tool/lib/ncbi-blast+. To make this work we have to sed out CustomBuildCommand and manually place a symlink in the install phase from the ncbi-tools+ files to the location where gff3toolkit expects them to be.

Then there is the next issue, namely that many files in this package have #! /usr/local/bin/python3 in the header, which is obviously not going to work .... :/

Overall, I'm not sure if this is worth the effort, the whole thing should just be fixed upstream to not hardcode all the things IMO.

@leycec
Copy link
Contributor

leycec commented Jul 20, 2021

Something is still broken, please fix it on your own
and cherrypick

...well, alrighty then. 🙉

This should probably be closed with apologies – which is awful but the lesser of multiple evils here.

@mmokrejs
Copy link
Contributor Author

Well it was a proposal for a new package but I agree it has issues like many biology-oriented tools. Thanks you for your efforts, I understand we are all short on time.

@leycec
Copy link
Contributor

leycec commented Jul 22, 2021

I'd happily lend a hand even though biology is outside my wheelhouse. Sadly, there are just a few too many unresolved issues here.

On the one hand, disabling downloads in setup.py usually just requires a sed one-liner to patch out the function call performing the downloads. Here's one I previously hacked together for dev-python/panel in this repository:

	sed -i -e '/^\s*_build_paneljs()$/d' setup.py || die

On the other hand, manually fetching, extracting, and symlink ncbi-blast+ in is quite a bit more work – as is recursively patching bogus #! /usr/local/bin/python3 headers. The latter is also a code smell that suggests upstream may be out of its depth; the standard POSIX-compliant shebang line for Python 3 is #!/usr/bin/env python3. #! /usr/local/bin/python3 is basically never right, so that's dark and suspicious.

But the worst offence is setup.py only installing a single README. Like, how does something like that even happen? O_o

@Nowa-Ammerlaan
Copy link
Member

On the one hand, disabling downloads in setup.py usually just requires a sed one-liner to patch out the function call performing the downloads.

When I tried this earlier a simple sed -i -e "/'build': CustomBuildCommand,/d" setup.py did the trick.

However, then I tried to enable the test phase with:

python_test() {
      sh test.py || die
}

(Yes, you need to run the .py file with (ba)sh, because the file name extension is wrong for some reason)
And then I ran into the Interpreter Not Found error, and lost all faith in that this can be made to work.

The latter is also a code smell that suggests upstream may be out of its depth; the standard POSIX-compliant shebang line for Python 3 is #!/usr/bin/env python3. #! /usr/local/bin/python3 is basically never right, so that's dark and suspicious.

I have a very strong suspicion that upstream is using MacOS, since non-system versions of python are often (always?) installed in /usr/local/ on that operating system. Of course that is no excuse for using a hardcoded shebang.

In theory the eclasses should automatically fix this, since somewhere in the install phase python_fix_shebang is called (I think). But for some reason it is not working in this package.

@Nowa-Ammerlaan Nowa-Ammerlaan marked this pull request as draft January 31, 2022 12:50
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