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

poco: propagate dependencies imported by CMake scripts #98186

Merged
merged 2 commits into from Sep 17, 2021
Merged

poco: propagate dependencies imported by CMake scripts #98186

merged 2 commits into from Sep 17, 2021

Conversation

lopsided98
Copy link
Contributor

Motivation for this change

poco's CMake scripts search for a few libraries at build time, so they should be propagated to allow dependent packages to build without manually adding these libraries as buildInputs. I enabled split outputs to prevent the runtime closure size from increasing.

The only packages affected by this change are clickhouse, which builds successfully, and toggldesktop, which was already broken.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • 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 nixpkgs-review --run "nixpkgs-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)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@lopsided98 lopsided98 mentioned this pull request Sep 17, 2020
Closed
@lopsided98 lopsided98 marked this pull request as draft September 17, 2020 23:41
@lopsided98
Copy link
Contributor Author

Actually this is broken. I assumed it worked because clickhouse worked, but I tried another out of tree package and it failed because for some unfathomable reason poco doesn't use GNUInstallDirs.

@lopsided98
Copy link
Contributor Author

I have submitted pocoproject/poco#3105 upstream to make poco use GNUInstallDirs.

I added the patch to this PR.

@lopsided98 lopsided98 marked this pull request as ready for review September 18, 2020 19:56
@lopsided98
Copy link
Contributor Author

cc @orivej

@lopsided98 lopsided98 mentioned this pull request Sep 20, 2020
Closed
@stale
Copy link

stale bot commented Mar 19, 2021

I marked this as stale due to inactivity. → More info

@stale stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Mar 19, 2021
@stale stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Mar 21, 2021
@lopsided98 lopsided98 mentioned this pull request Mar 22, 2021
Closed
@mikepurvis mikepurvis mentioned this pull request Jun 29, 2021
Closed
9 tasks
@mikepurvis
Copy link
Contributor

mikepurvis commented Jul 9, 2021 •
edited

I've been using this from @lopsided98's fork for a few weeks and it's worked great for me— I'd love to get it merged.

Looks like the only changes needed to resolve the merge conflict are on the first line: pkg-config instead of pkgconfig, and including lib separately from stdenv.

@lopsided98
Copy link
Contributor Author

I fixed the conflicts.

@mikepurvis mikepurvis mentioned this pull request Sep 17, 2021
Merged
12 tasks
@risicle
Copy link
Contributor

risicle commented Sep 17, 2021

Why omit some of the CMakeLists.txts?

@risicle
Copy link
Contributor

risicle commented Sep 17, 2021

nixpkgs-review reveals no new failures, macos 10.15 & nixos x86_64

@lopsided98
Copy link
Contributor Author

Why omit some of the CMakeLists.txts?

Those files are not included in the release tarball.

@risicle
Copy link
Contributor

risicle commented Sep 17, 2021

Cool - that would be nice to include in a comment.

Oh and while I'm making a wishlist, adding a name = "use-gnuinstalldirs.patch"; to the fetchpatch call and a comment linking to the PR 😁

@lopsided98 lopsided98 force-pushed the poco-propagate-deps branch 2 times, most recently from 4822285 to 2cccf09 Compare September 17, 2021 21:42
@lopsided98
poco: add patch to use GNUInstallDirs
8f41eda
@lopsided98
poco: propagate dependencies imported by CMake scripts
2122017 
This also requires enable multiple outputs to prevent the closure size from
increasing.
@risicle risicle merged commit 6f205e5 into NixOS:master Sep 17, 2021
@lopsided98 lopsided98 deleted the poco-propagate-deps branch September 17, 2021 23:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@risicle risicle risicle left review comments

Assignees
No one assigned
Labels
10.rebuild-darwin: 1-10 10.rebuild-darwin: 1 10.rebuild-linux: 1-10
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@lopsided98 @mikepurvis @risicle @SuperSandro2000