Skip to content

openblas: add pkg-config aliases #45257

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
Merged
merged 1 commit into from
Aug 26, 2018
Merged

openblas: add pkg-config aliases #45257

merged 1 commit into from
Aug 26, 2018

Conversation

timokau
Copy link
Member

@timokau timokau commented Aug 17, 2018

Motivation for this change

Openblas provides implementations for various standards. To be able to assume the place of cblas for example in the eyes of a program using pkg-config, it needs to provide a .pc file telling pkg-config that it is cblas. Concretely, sage needs these pc files. I included this in sage but now I think this is not really sage specific.

The current implementation adds those pc-files as independent packages that always assume openblasCompat. That is not ideal. Other implementations I could think of:

  • add openblasCompat-cblas-pc and openblas-cblas-pc speperately (ugly)
  • add the pc as a dependency of openblas. This would always include them and be the easiest option. Not sure if that could break something.
  • add the pc as a wrapper around openblas that can optionally be enabled (no rebuild of openblas necessary)

What do you think @ttuegel?

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Fits CONTRIBUTING.md.

@GrahamcOfBorg GrahamcOfBorg added 8.has: package (new) This PR adds a new package 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. labels Aug 17, 2018
@matthewbauer
Copy link
Member

Would it be possible to just install these 3 files in the openblas derivation? IIUC its harmless to have multiple of them, so might as well aim for maximum compatiblity.

@timokau
Copy link
Member Author

timokau commented Aug 18, 2018

I think that is what I meant by option 2. That would really be the easiest and nicest option. I just wasn't sure if it could be harmful. What would pkgconfig do if it finds multiple options?

@timokau
Copy link
Member Author

timokau commented Aug 18, 2018

If it isn't harmful we may even suggest the inclusion upstream.

@timokau
Copy link
Member Author

timokau commented Aug 18, 2018

Apparently upstream actually used to have this: OpenMathLib/OpenBLAS#1096

@timokau timokau mentioned this pull request Aug 18, 2018
Closed
@timokau
Copy link
Member Author

timokau commented Aug 18, 2018

Upstream report: OpenMathLib/OpenBLAS#1740

@ttuegel
Copy link
Member

ttuegel commented Aug 19, 2018

I don't see a problem with including these in our OpenBLAS build.

@timokau timokau mentioned this pull request Aug 20, 2018
Closed
9 tasks
@timokau
Copy link
Member Author

timokau commented Aug 21, 2018

Upstream seems to more or less agree:

I am not opposed to reinstating the additional pkgconfig files, as long as they do not cause any new and interesting problems. Getting the recent thread memory allocating mess fixed is much higher on my list of priorities however.

So I'll amend the PR to add the pkg-config files unconditionally and be based on stable.

@timokau timokau changed the base branch from master to staging August 21, 2018 14:41
@timokau
Copy link
Member Author

timokau commented Aug 21, 2018

Done, but still testing.

@GrahamcOfBorg GrahamcOfBorg added 6.topic: stdenv Standard environment 10.rebuild-darwin-stdenv This PR causes stdenv to rebuild on Darwin and must target a staging branch. 10.rebuild-linux-stdenv This PR causes stdenv to rebuild on Linux. 10.rebuild-darwin: 501+ This PR causes many rebuilds on Darwin and should normally target the staging branches. 10.rebuild-linux: 501+ This PR causes many rebuilds on Linux and should normally target the staging branches. 10.rebuild-darwin: 101-500 This PR causes between 101 and 500 packages to rebuild on Darwin. and removed 6.topic: stdenv Standard environment 10.rebuild-darwin-stdenv This PR causes stdenv to rebuild on Darwin and must target a staging branch. 10.rebuild-linux-stdenv This PR causes stdenv to rebuild on Linux. 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. 10.rebuild-darwin: 501+ This PR causes many rebuilds on Darwin and should normally target the staging branches. labels Aug 21, 2018
@timokau
Copy link
Member Author

timokau commented Aug 25, 2018

Okay, tests (built sage and tested it and its openblas-using dependencies) succeed. Upstream doesn't include it since it would conflict with other blas implementations providing the same pkgconfig files. Since nix's pkgconfig files are not system-wide, I think there is no issue here. A problem would only occur if a package depends on two blas implementations and depends on the blas.pc file. In that case, it would be a simple matter of which blas implementation appears first in PKG_CONFIG_PATH (so probably which one appears first in buildInputs.

So in summary I think this can't hurt. I'll merge this tomorrow if nobody complains.

@timokau timokau changed the title [WIP] openblas: add pkg-config aliases openblas: add pkg-config aliases Aug 26, 2018
@timokau timokau merged commit c614c15 into NixOS:staging Aug 26, 2018
@timokau timokau deleted the openblas-pc branch August 29, 2018 18:05
@timokau timokau mentioned this pull request Sep 3, 2018
Merged
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
8.has: package (new) This PR adds a new package 10.rebuild-darwin: 101-500 This PR causes between 101 and 500 packages to rebuild on Darwin. 10.rebuild-linux: 501+ This PR causes many rebuilds on Linux and should normally target the staging branches.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@timokau @matthewbauer @ttuegel @GrahamcOfBorg