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

fasttree: replace gcc dependency with libomp #1123

Merged
merged 4 commits into from
Aug 5, 2020
Merged

fasttree: replace gcc dependency with libomp #1123

merged 4 commits into from
Aug 5, 2020

Conversation

SeekingMeaning
Copy link
Contributor

  • Have you followed the guidelines for contributing?
  • Have you checked that there aren't other open pull requests for the same formula update/change?
  • Have you built your formula locally with brew install --build-from-source FORMULA, where FORMULA is the name of the formula you're submitting?
  • Does your build pass brew audit --strict FORMULA (after doing brew install FORMULA)?

Formulae that might need to be bumped:

  • pirate
  • parsnp
  • nullarbor
  • roary

@SeekingMeaning SeekingMeaning self-assigned this Jul 24, 2020
sjackman
sjackman reviewed Jul 24, 2020
View reviewed changes
Formula/fasttree.rb Outdated
end

def install
opts = %w[-O3 -finline-functions -funroll-loops]
opts << "-DOPENMP" << "-fopenmp" if build.with? "openmp"
opts << "-DOPENMP" << "-L#{Formula["libomp"].opt_lib}" << "-lomp" if build.with? "openmp"
Copy link
Member

Choose a reason for hiding this comment

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

libomp shouldn't be used on Linux.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh yeah whoops

sjackman
sjackman reviewed Jul 24, 2020
View reviewed changes
Formula/fasttree.rb Outdated
@@ -21,13 +22,14 @@ class Fasttree < Formula
option "without-sse", "Disable SSE parallel instructions"
Copy link
Member

Choose a reason for hiding this comment

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

You can remove these options if you like.

sjackman
sjackman reviewed Jul 24, 2020
View reviewed changes
Formula/fasttree.rb Outdated
depends_on "gcc" if OS.mac? # needs openmp
on_macos do
depends_on "libomp"
end
Copy link
Member

Choose a reason for hiding this comment

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

on_macos should go on the outside of the if.

Copy link
Member

Choose a reason for hiding this comment

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

You can remove the if build.with? though if you remove the options.

sjackman
sjackman requested changes Jul 24, 2020
View reviewed changes
@SeekingMeaning
Copy link
Contributor Author

What should we do for broken dependents?

@sjackman
Copy link
Member

sjackman commented Jul 24, 2020 •
edited
Loading 

==> brew test --verbose brewsci/bio/roary
118
==> FAILED
119
Testing brewsci/bio/roary
120
==> /home/linuxbrew/.linuxbrew/Cellar/roary/3.13.0/bin/roary -w 2>&1
121
Util.c: loadable library and perl binaries are mismatched (got handshake key 0xcd00080, needed 0xed00080)
122
Error: brewsci/bio/roary: failed

https://github.com/brewsci/homebrew-bio/pull/1123/checks?check_run_id=908545358#step:5:121

In a separate PR bump roary for perl.

I have a concept that I call "long shelf life bottles". Basically bumping a formulae in Homebrew/core should not cause breakage in bottles of third party taps. I'd really like this to be a feature of Homebrew 3. Happy to discuss more in Slack. Please record all the formulae that caused breakage in Brewsci/bio. perl, python, gcc, gsl, …

@sjackman
Copy link
Member 

==> brew test --verbose brewsci/bio/parsnp
113
==> FAILED
114
Testing brewsci/bio/parsnp
115
/usr/bin/sandbox-exec -f /private/tmp/homebrew20200724-17982-1eg180e.sb ruby -W0 -I $LOAD_PATH -- /usr/local/Homebrew/Library/Homebrew/test.rb /usr/local/Homebrew/Library/Taps/brewsci/homebrew-bio/Formula/parsnp.rb --verbose
116
==> /usr/local/Cellar/parsnp/1.2_3/bin/parsnp -v 2>&1
117
sh: line 1: 17995 Abort trap: 6           /usr/local/Cellar/parsnp/1.2_3/bin/parsnp -v 2>&1
118
dyld: Library not loaded: /usr/local/opt/gcc/lib/gcc/9/libgomp.1.dylib
119
  Referenced from: /usr/local/Cellar/parsnp/1.2_3/bin/parsnp
120
  Reason: image not found
121
Error: brewsci/bio/parsnp: failed

https://github.com/brewsci/homebrew-bio/pull/1123/checks?check_run_id=908545349#step:4:118

Merge the parsnp PR first, then rebase this PR onto origin/develop.

@SeekingMeaning SeekingMeaning mentioned this pull request Jul 24, 2020
Closed
@SeekingMeaning SeekingMeaning mentioned this pull request Jul 25, 2020
Merged
4 tasks
sjackman
sjackman requested changes Aug 5, 2020
View reviewed changes
Copy link
Member

@sjackman sjackman left a comment

Choose a reason for hiding this comment

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

Build with SSE.

@SeekingMeaning @sjackman
Update Formula/fasttree.rb
95b3e05 
Co-authored-by: Shaun Jackman <[email protected]>
sjackman
sjackman approved these changes Aug 5, 2020
View reviewed changes
Copy link
Member

@sjackman sjackman left a comment

Choose a reason for hiding this comment

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

Thank you, Seeker!

@SeekingMeaning SeekingMeaning merged commit 6777b96 into brewsci:develop Aug 5, 2020
@SeekingMeaning SeekingMeaning deleted the fasttree branch August 5, 2020 22:47
@SeekingMeaning SeekingMeaning mentioned this pull request Aug 15, 2020
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sjackman sjackman sjackman approved these changes

Assignees

@SeekingMeaning SeekingMeaning

Labels
revision
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@SeekingMeaning @sjackman