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.

Sign up for GitHub

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

Jump to bottom

Adding quorum #2765

Merged
merged 18 commits into from
Feb 4, 2017
Merged

Adding quorum #2765

merged 18 commits into from
Feb 4, 2017

Conversation

colinbrislawn
Copy link
Contributor

  • I have read the guidelines above.
  • This PR adds a new recipe.
  • This PR updates an existing recipe.
  • This PR does something else (explain below).

This works fine with conda build but I should probably also test with ./simulate-travis.py...

@colinbrislawn
Copy link
Contributor Author

colinbrislawn commented Nov 2, 2016 •
edited
Loading

🤕 I accidentally included an older recipe along with this PR. Doh!

This PR now just includes Quorum, as intended.

bgruening
bgruening reviewed Nov 2, 2016
View reviewed changes
recipes/quorum/build.sh Outdated
@@ -0,0 +1,9 @@
#!/bin/bash

pushd $SRC_DIR
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can be removed.

recipes/quorum/meta.yaml Outdated
@@ -0,0 +1,30 @@
build:
number: 1
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

0

recipes/quorum/meta.yaml Outdated
- yaggo >=1.5.8
- jellyfish
run:
- libgcc
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

libgcc # [linux]

recipes/quorum/meta.yaml Outdated
md5: 14cc37310ff81b447148797afe551fe9
requirements:
build:
- gcc
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

gcc # [linux]
llvm # [osx]

bgruening
bgruening reviewed Nov 2, 2016
View reviewed changes
recipes/quorum/build.sh Outdated
#!/bin/bash

pushd $SRC_DIR

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add: sed -i.bak '1 s|^.*$|#!/usr/bin/env perl|g' $PREFIX/bin/autoreconf hopefully it will make it work.

@colinbrislawn
Copy link
Contributor Author

colinbrislawn commented Nov 2, 2016 •
edited
Loading

Thank you for helping me troubleshoot this build (and also the autoconf package). I really appreciate your guidance.

(I'll squash commits before merging.)

bgruening
bgruening reviewed Nov 2, 2016
View reviewed changes
recipes/quorum/meta.yaml Outdated
- gcc # [linux]
- llvm # [osx]
- autoconf
- automake
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you also need perl-threaded as build dependency

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added!

@colinbrislawn
Copy link
Contributor Author

colinbrislawn commented Nov 3, 2016 •
edited
Loading

Looks like autoreconf is still not working on linux.
autoreconf: failed to run aclocal: No such file or directory
This is strange because automake installed fine.

Also, it looks like it's getting jellyfish from conda-forge, while it should get the newer version from bioconda.

configure: error: Package requirements (jellyfish-2.0) were not met:
...
    jellyfish-0.5.6            |           py35_0         228 KB  conda-forge

EDIT: see this PR on conda-forge for jellyfish: conda-forge/staged-recipes#1567

EDIT: Looks like it might need jellyfish 2.0. I've updated the needed version, so let's see!
https://github.com/gmarcais/Quorum/blob/8e104d47d87b2f0e33c927ac9b8ec2057ef65ce9/configure.ac#L29

@colinbrislawn colinbrislawn reopened this Nov 7, 2016
@colinbrislawn
Copy link
Contributor Author

Let's see if the new autoconf helps with this...

@colinbrislawn colinbrislawn mentioned this pull request Nov 11, 2016
Open
bgruening
bgruening reviewed Nov 12, 2016
View reviewed changes
recipes/quorum/build.sh Outdated
@@ -0,0 +1,6 @@
sed -i.bak '1 s|^.*$|#!/usr/bin/env perl|g' $PREFIX/bin/autoreconf
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We fixed autoconf, can you try to remove this and rebase against latest master?

@colinbrislawn
Copy link
Contributor Author

colinbrislawn commented Nov 14, 2016 •
edited
Loading

@bgruening I've removed that sed command, as requested.

My git skills are not that great, and It's possible that I may not properly synced this with the master branch. Your guidance is really helpful.

@bgruening
Copy link
Member

Please try the following:

git checkout master
git remote add upstream https://github.com/bioconda/bioconda-recipes.git
git pull upstream master # getting latest changes from this repo, not your fork
git checkout adding-Quorum # go back to your branch
git rebase master # rebase against the lastest master
git push origin adding-Quorum -f # force push your rebase

@colinbrislawn
Copy link
Contributor Author

colinbrislawn commented Nov 30, 2016 •
edited
Loading

Thank you for your feedback! I've rebased my branch and updated this PR. Let's see how this works...
Edit: this is failing due to the AttributeError: 'FileNotFoundError' error. Once that gets fixed, let's see how well this works.

@colinbrislawn
Copy link
Contributor Author

Still not working... I've reached out to the quorum devs because I think this may have something to do with linking to jellyfish.

Any ideas?

@colinbrislawn
Copy link
Contributor Author

My colleague and I can build this locally, but only once we set this as a local path:

export JELLYFISH2_0_CFLAGS=-I/home/me/anaconda2/pkgs/jellyfish-2.2.6-0/include/jellyfish-2.2.6
export JELLYFISH2_0_LIBS=/home/me/anaconda2/pkgs/jellyfish-2.2.6-0/lib/libjellyfish-2.0.so

We can set these paths in build.sh, but should we generate these using the program?

@bgruening
Copy link
Member

setting it to xport JELLYFISH2_0_CFLAGS=-I$PREFIX/include/jellyfish-2.2.6 and the other is not enough?

@colinbrislawn
Copy link
Contributor Author

Ah, using $PREFIX works really well! Thank you.

I've got it working, but only with using gcc for both Linux and OSX. Is this OK, or should I try to get it working with llvm like a 'normal' OSX build?

@bgruening
Copy link
Member

Ideally, we should get it working with llvm. But we had already a few recipes where this was not possible and in this case gcc is ok on OSX:

@colinbrislawn
Copy link
Contributor Author

Thanks for the feedback. This should now build using gcc on both Linux and OSX.

Should I pursue a working llvm build or is this acceptable?

@bgruening
Copy link
Member

I'm fine with this. But I'm not a OSX users, let's ask our famous @bioconda/osx subgroup :)

@acaprez
Copy link
Contributor

acaprez commented Feb 2, 2017 •
edited
Loading

Looks like building on OS X is a known problem with the package: gmarcais/Quorum#2. A bit of searching of that error leads down the path of Apple vs GCC C++ compatibility issues, which IMO are best left to the package devs to resolve. So using GCC for both platforms seems reasonable to me.

@colinbrislawn
Copy link
Contributor Author

colinbrislawn commented Feb 3, 2017 •
edited
Loading

The lib file name is slightly different on OSX and linux.

linux: libjellyfish-2.0.so
macOS: libjellyfish-2.0.dylib

I'll try this code block: if [ "$(uname)" == "Darwin" ]; then

Is this the recommended way to accommodate this?

@colinbrislawn colinbrislawn merged commit ccbbbec into bioconda:master Feb 4, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bgruening bgruening bgruening left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@colinbrislawn @bgruening @acaprez