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

suite-sparse: depend on libomp on macOS #129100

Closed
wants to merge 1 commit into from

Conversation

carlocab
Copy link
Member

  • Have you followed the guidelines for contributing?
  • Have you ensured that your commits follow the commit style guide?
  • 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?
  • Is your test running fine brew test <formula>, where <formula> is the name of the formula you're submitting?
  • Does your build pass brew audit --strict <formula> (after doing brew install --build-from-source <formula>)? If this is a new formula, does it pass brew audit --new <formula>?

Upstream relies on OpenMP by default, and even warns about the lack of
thread-safety without OpenMP.

Multiple users have also already asked for this.

Fixes #129011.

Upstream relies on OpenMP by default, and even warns about the lack of
thread-safety without OpenMP. [1]

Multiple users have also already asked for this.

Fixes Homebrew#129011.

[1] https://github.com/DrTimothyAldenDavis/SuiteSparse#compilation-options
@github-actions github-actions bot added the long build Set a long timeout for formula testing label Apr 23, 2023
@fxcoudert
Copy link
Member

Because this depends on OpenBLAS, where OpenMP is provided by GCC, it should do the same.

@carlocab
Copy link
Member Author

carlocab commented Apr 24, 2023

That's going to be a problem for dependents, since this has C++ code, and we don't want to mix C++ standard library implementations.

@fxcoudert
Copy link
Member

But we don't want to mix OpenMP libraries either…

@carlocab
Copy link
Member Author

Yea, I figured that: #129011 (comment)

Not really sure about the way out other than to make OpenBLAS use libomp too. But that'll require some care as well.

@ywwry66
Copy link
Contributor

ywwry66 commented Apr 24, 2023

make OpenBLAS use libomp too

Because of #50252 (comment), I have been doing this on my own for a while (ywwry66@a47be51). And for my workflow (R user), it has been working pretty well.

Apple is really giving us a hard time :(

@cho-m cho-m mentioned this pull request May 14, 2023
6 tasks
@junghans
Copy link
Contributor

And there is #129512, which also fails due to mixing.

@junghans
Copy link
Contributor

@fxcoudert I looked at your comment again(#50252 (comment)). Has anything changed in the last 2 years? What is the status of clang + openMP these days?

@carlocab carlocab mentioned this pull request May 19, 2023
6 tasks
@junghans
Copy link
Contributor

junghans commented Jun 5, 2023

I think this is a more general issue and we should track all the packages that are build with gcc only and document why.

I see two main reasons, a.) OpenMP, but that can be mostly be achieved with clang + libomp these days, b.) Fortran and OpenMP, this one is harder, but it would be work a try to build with the new flang (flang-new) compiler.

@fxcoudert
Copy link
Member

I don't think things have changed at all since my 2020 comment. Flang is not stable / production-ready. Apple still does not ship libomp with its system compiler.

@github-actions
Copy link
Contributor

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

@github-actions github-actions bot added the stale No recent activity label Jun 27, 2023
@fxcoudert fxcoudert closed this Jun 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
long build Set a long timeout for formula testing stale No recent activity
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Suitesparse compiled without OpenMP (and without _POSIX_C_SOURCE enabled)
4 participants