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

ghostz: replace gcc with libomp #1140

Merged
merged 3 commits into from
Aug 20, 2020
Merged

ghostz: replace gcc with libomp #1140

merged 3 commits into from
Aug 20, 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)?

libomp might be required for Linux too; I'm not sure

@SeekingMeaning SeekingMeaning self-assigned this Aug 15, 2020
@SeekingMeaning
Copy link
Contributor Author

SeekingMeaning commented Aug 15, 2020 •
edited
Loading 

  * The URL https://www.bi.cs.titech.ac.jp/ghostz/ is not reachable
  * Stable: The URL https://www.bi.cs.titech.ac.jp/ghostz/releases/ghostz-1.0.2.tar.gz is not reachable

There's https://github.com/akiyamalab/ghostz but it doesn't seem to be updated to the latest version (akiyamalab/ghostz#6)

https://web.archive.org/web/20190501080707/http://www.bi.cs.titech.ac.jp/ghostz/history

v1.0.1
  -latest release
    Bug fix: Fixed the problem that this program terminates abnormally if a query length is under 11 residues.
v1.0.2
  -latest release
    New option: If query length is shorter than the number given in parameter with '-n' in amino acid residues, the query will be skipped from the analysis.

@SeekingMeaning SeekingMeaning mentioned this pull request Aug 15, 2020
Open
@sjackman
Copy link
Member

sjackman commented Aug 17, 2020 •
edited
Loading 

$ curl -s https://www.bi.cs.titech.ac.jp/ghostz/releases/ghostz-1.0.2.tar.gz | tar t | head -n3
ghostz-1.0.2/
ghostz-1.0.2/LICENSE
ghostz-1.0.2/Makefile

Works for me. Was it a transient network failure?

I triggered a re-run of CI.

@SeekingMeaning
Copy link
Contributor Author

Huh okay, I'll use this opportunity to archive it on archive.org

https://web.archive.org/web/20200817164848/https://www.bi.cs.titech.ac.jp/ghostz/releases/ghostz-1.0.2.tar.gz

@sjackman
Copy link
Member

Huh okay, I'll use this opportunity to archive it on archive.org

I probably wouldn't bother for the first such transient failure, but thanks all the same!

sjackman
sjackman reviewed Aug 18, 2020
View reviewed changes
Formula/ghostz.rb Outdated

def install
if OS.mac?
ENV.cxx11
Copy link
Member

Choose a reason for hiding this comment

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

What's the error message without this line? I'm still curious what it actually does. I thought that -std=c++11 was the default for most modern compilers.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ext/seg/src/seg.cpp:424:18: error: use of overloaded operator '==' is ambiguous (with operand types 'std::shared_ptr<SeqmentType>' and 'long')
                if (temp->next == NULL) {
                    ~~~~~~~~~~ ^  ~~~~

I kinda just threw in ENV.cxx11 to see if it would fix the error

Copy link
Member

Choose a reason for hiding this comment

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

Change NULL to nullptr.

Copy link
Member

Choose a reason for hiding this comment

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

I kinda just threw in ENV.cxx11 to see if it would fix the error.

Did it fix the error?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I kinda just threw in ENV.cxx11 to see if it would fix the error.

Did it fix the error?

Yeah

Change NULL to nullptr.

That worked 🤔

sjackman
sjackman reviewed Aug 18, 2020
View reviewed changes
Formula/ghostz.rb
"-fopenmp",
"-L#{Formula["libomp"].opt_lib} -lomp"
inreplace Dir["**/*.{cpp,h}"] do |s|
s.gsub! "#include <tr1/",
Copy link
Member

Choose a reason for hiding this comment

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

😬 TR1. Yikes.

sjackman
sjackman approved these changes Aug 20, 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!

@SeekingMeaning SeekingMeaning merged commit 77e278b into brewsci:develop Aug 20, 2020
@SeekingMeaning SeekingMeaning deleted the ghostz branch August 20, 2020 22:16
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