sci-biology/fastqc does not install the package's java code #834
Conversation
|
This version works for me but has some issues:
|
|
Hi, Here are bits of our communication with Simon: I see there is build.xml in current fastqc_v0.11.5_source.zip so the one attached is probably outdated. Here I attache build.log.txt file from Simon showing how the compilation went for him. Thank you for pursuing packaging it for Gentoo. |
|
Thanks a lot for your reply - and for the comments you left in the e-build file, they are super helpful! I did work with Java before and the words you say and quote make sense to me. Will need some help with the ebuild writing, though. I'm a bit busy at work right now, so as time permits the plan is:
|
|
@mmokrejs @anton-molyboha are you still interested in maintaining this? We recently fixed up the overlay and fixed 6k+ QA errors, and are looking to improve the quality overall. As I do not work with Java either, I would be willing to help and learn if you would get involved as well, but if the onus falls fully on me, I think we would need to stop providing the package. |
|
I am willing to bring this package into a "works for me" state and update the pull request. What else can I do to help maintain it? |
If you could also enable tests for the package and get them working, that would be highly appreciated. |
|
@anton-molyboha providing your email address in |
|
Thanks for the feedback. I think I will be able to get to it at the end of the week, and will update with the progress by Tuesday. Let me know if there is any timeline related to this (if I'm blocking somebody, or if there is a scheduled release that would be nice to hit, etc) |
|
I have updated the Pull Request to a works-for-me state. Testing done:
I suggest that making an automated test be a separate PR. |
|
From a conversation with epsilonKNOT on #gentoo-science, several suggestions:
|
|
Hi @anton-molyboha Feel free to take over maintainership of this or even other packages I worked on in the past in Gentoo, I am too busy these days. Good luick! |
|
Thanks, @mmokrejs |
|
A new version of the PR!
There are still issues re missing tests and two libraries being bundled by the upstream (instead of us using the Gentoo versions, which would probably be better) I would prefer to look at these issues separately. |
|
@anton-molyboha |
| RDEPEND="${DEPEND} | ||
| RDEPEND=" | ||
| dev-lang/perl | ||
| >=virtual/jre-1.5:* |
There was a problem hiding this comment.
remove the jdk/jre requirement, the eclasses ensure that.
There was a problem hiding this comment.
Yes, you are right! Will remove them.
There was a problem hiding this comment.
When I remove jdk from the dependencies, I am getting
QA Notice: Package is using java-ant, but doesn't depend on a Java VM
!!! ERROR: Couldn't find a VM dep
!!! ERROR: Couldn't find a VM dep
* Could not find valid -source/-target values
and the build fails. I guess, an explicit jdk dependency is still necessary.
|
|
||
| src_prepare(){ | ||
| cp "${FILESDIR}"/build.xml . || die | ||
| eapply_user |
There was a problem hiding this comment.
do default, which also includes this.
| doins -r bin | ||
| chmod a+x "${ED}/opt/${PN}/bin/fastqc" | ||
| # Add the package's bin directory to the PATH. | ||
| echo "PATH=\"${EPREFIX}/opt/${PN}/bin\"" > "${T}/00fastqc" || die |
There was a problem hiding this comment.
better to create this file in files/00fastqc instead of doing an echo.
Don't need the ${EPREFIX}.
There was a problem hiding this comment.
Without the ${EPREFIX} it would not work in a Prefix environment, would it?
There was a problem hiding this comment.
env-update handles the creating of $PATH from env.d files, it manages the ${EPREFIX} too.
There was a problem hiding this comment.
From my experiments: env-update is called automatically, but does not manage ${EPREFIX}. Instead, one needs to call hprefixify defined by the prefix eclass. Example: baselayout-2.7.ebuild, line 187
| dev-lang/perl | ||
| >=virtual/jre-1.5:* | ||
| " | ||
| DEPEND="$RDEPEND |
There was a problem hiding this comment.
small nit add {} to make it ${RDEPEND}.
|
|
||
| dodoc README.txt RELEASE_NOTES.txt | ||
|
|
||
| # There is no fastqc.jar. The output from the compilation is the set of |
There was a problem hiding this comment.
Now I know that this is not your fault but could this message be cleared up? Do we still need it?
If it is actually important and needs action by the user, can you make this into a message that is given to the user in pkg_postinst?
Why do we not have a dependency on these packages sci-libs/jhdf5 and sci-biology/picard? You seem to have dropped sci-libs/jhdf5 in the update?
Give the fact that we don't have tests, we manually need to make sure we are installing the proper dependencies.
Seeing that you haven't made a version bump, possibly upstream has not made any new releases, so it is unlikely that we are now magically compatible with the newer version of the codebase. Is it possible (though unlikely) that upstream has changed to a new url?
I know these are a lot of questions but unfortunately with the lack of tests the only way I can know anything is by voicing it out :(
There was a problem hiding this comment.
- By "this message" you mean the long comment below the highlighted code? Yes, I think it can be cleared up. It was a note for a future maintainner (me) from the previous maintainer (mmokrejs).
- I have dropped the dependency on sci-libs/jhdf5 and sci-biology/picard because they are bundled with the package itself, so that fastqc is using its own copy of them.
- Yes, to "manually make sure we are installing the proper dependencies" I install the package into a Prefix environment and check that it runs for a sample input. For jhdf5 and picard, I also see that their .jar files are indeed included as promised, and get installed with the package, and that the main program adds them to the java's CLASSPATH.
- I believe upstream did make new releases, but I am trying to follow the motto of "make many small changes, rather than one huge one". Small changes are easier to review, and easier to not make mistakes in. I promise I will make a version bump separately.
- "Voicing it out" works for me! Really appreciate it.
There was a problem hiding this comment.
This is amazing though, they are using a perl script to invoke a java binary...
Absolutely amazing...
There was a problem hiding this comment.
@anton-molyboha Indeed, 0.11.9 is out. The HDF5 is necessary to work with Oxford nanopore FAST5 files, IMO.
There was a problem hiding this comment.
@mmokrejs Thanks. It's a moon-shot, but do you happen, by any chance, have an example of a FAST5 file I could use for testing?
There was a problem hiding this comment.
Try to fetch some from NCBI SRA. Maybe by default you get only FASTQ format but it should be possible to fetch some using sra-toolkit to obtain the original FAST5-formatted data.
https://www.ncbi.nlm.nih.gov//sra?term=(nanopore)%20NOT%20cluster_dbgap%5BPROP%5D
Here is another FAST5 set: https://trace.ncbi.nlm.nih.gov/Traces/sra/sra.cgi?study=SRP166020
Alternatively, here you have plenty demo human data:
https://github.com/nanopore-wgs-consortium/NA12878
Some more background info:
https://simpsonlab.github.io/2017/02/27/packing_fast5/
There was a problem hiding this comment.
Haha, to download from NCBI SRA I would have to fix the sra-toolkit package first, which is also broken! I will try the nanopore-wgs-consortium link you gave.
|
@epsilon-0 Do you want to take a look at the updated PR? On the other hand, in my testing, .fast5 files don't work (thanks, mmokrejs, for the test data) Two options make sense to me:
|
|
Lets merge it, it looks good enough to me (given the horrible state it was in previously). |
|
Squash all your commits into one commit and add the sign-off to that. |
|
I think Travis CI is failing because of the scikits_learn rename... It started when I rebased my changes onto the latest HEAD, and the dependency errors all mention scikits_learn. I see people are working on it right now, so I guess I'll just wait until it is resolved. |
|
Yes, the update happened yesterday, we have A LOT of packages depending on scikit-learn. |
Based on local testing, the program now works for .fastq files, but does not work for .fast5 files. This is also not the latest version. However, this is a step forward from "not working at all". The issues will need to be solved in the future. Signed-off-by: Anton Molyboha <anton.stay.connected@gmail.com>
|
thanks for the fix 😸 |
4 participants
It looks like sci-biology/fastqc-0.11.3 is supposed to install some java *.class files but fails to do so.
'emerge sci-biology/fastqc' completes without an error, but when I try running fastqc with any meaningful options, I am getting:
Error: Could not find or load main class uk.ac.babraham.FastQC.FastQCApplication
The ebuild's src_install() only installs the wrapper scripts and docs, and has a comment:
I am guessing that the .class files mentioned in the comment should also be installed somewhere (not really sure where, though) If my guess is true, I will come up with a work-around and will post it here.