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

poco: 1.13.3 -> 1.14.1; fix build with new poco in dependent packages #383163

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

dwt
Copy link
Contributor

@dwt dwt commented Feb 18, 2025

Changes

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 25.05 Release Notes (or backporting 24.11 and 25.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

@trofi This is the reopened pull request for #369312 - did I miss anything?

@dwt
Copy link
Contributor Author

dwt commented Feb 18, 2025

@trofi One open question I had, is that it seems all downstream users of poco now need utf8proc as a dependency. At what point should this become part of the propagatedBuildInputs? I haven't been able to find any enlightening documentation on that topic yet, so I would love some advice.

@dwt
Copy link
Contributor Author

dwt commented Feb 18, 2025

Sorry, forgot to mark it as draft immediately, as there are still some dependents I haven't gotten around to fixing.

@dwt dwt force-pushed the update-poco-to-1.14.1 branch from a82f4ed to 65ebf87 Compare February 18, 2025 19:11
@dwt
Copy link
Contributor Author

dwt commented Feb 18, 2025

Upstream bug reports I've already opened for this:

@dwt dwt mentioned this pull request Feb 18, 2025
13 tasks
Comment on lines +64 to +65
"-D g15=OFF"
"-D CMAKE_CXX_STANDARD=17" # protobuf >22 requires C++ 17
Copy link
Member

Choose a reason for hiding this comment

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

There are helpers available that make setting cmakeFlags more "semantic" :)

Suggested change
"-D g15=OFF"
"-D CMAKE_CXX_STANDARD=17" # protobuf >22 requires C++ 17
(lib.cmakeBool "g15" false)
(lib.cmakeFeature "CMAKE_CXX_STANDARD" "17") # protobuf >22 requires C++ 17

Copy link
Contributor Author

Choose a reason for hiding this comment

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

applied

@trofi
Copy link
Contributor

trofi commented Feb 18, 2025

@trofi One open question I had, is that it seems all downstream users of poco now need utf8proc as a dependency. At what point should this become part of the propagatedBuildInputs? I haven't been able to find any enlightening documentation on that topic yet, so I would love some advice.

The typical use case for propagatedBuildInputs is when a .dev output requires another .dev output. That could happen if poco's public headers use public headers of utf8string or installed poco's .pc file refers utf8string .pc files. I think it's a natural effect of fixed unbundling.

@trofi
Copy link
Contributor

trofi commented Feb 18, 2025

nixpkgs-review result

Generated using nixpkgs-review.

Command: nixpkgs-review pr 383163


x86_64-linux

❌ 1 package failed to build:
  • collabora-online
✅ 14 packages built:
  • craftos-pc
  • ioq3-scion
  • ioquake3
  • litebrowser
  • multipass
  • mumble
  • mumble_overlay
  • murmur
  • poco
  • poco.dev
  • pothos
  • quake3demo
  • sanjuuni
  • vscode-extensions.jackmacwindows.craftos-pc

@dwt
Copy link
Contributor Author

dwt commented Feb 18, 2025

The typical use case for propagatedBuildInputs is when a .dev output requires another .dev output. That could happen if poco's public headers use public headers of utf8string or installed poco's .pc file refers utf8string .pc files. I think it's a natural effect of fixed unbundling.

Looking at the dev derivation of poco I see that there are cmake files to find utf8proc:

❯ rg utf8proc  /nix/store/67f3dgynfwmhj4a02l4zvvd4dlhmaf72-poco-1.14.1-dev/
/nix/store/67f3dgynfwmhj4a02l4zvvd4dlhmaf72-poco-1.14.1-dev/lib/cmake/Poco/FindUtf8Proc.cmake
5:# Find utf8proc
20:#      UTF8PROC_INCLUDE_DIR - where to find utf8proc.h
21:#      UTF8PROC_LIBRARY     - the utf8proc library
39:    UTF8PROC_INCLUDE_DIR NAMES utf8proc.h DOC "The utf8proc include directory"
43:    UTF8PROC_LIBRARY NAMES utf8proc DOC "The utf8proc library"

I don't understand enough cmake however to deduce if that means all users of poco auto require this?

I'm also not quite sure if you say that this should mean utf8proc should be a propagated dependency? For example litebrowser does not seem to require it? It may however get it propagated from another input. How would I find that out?

@trofi
Copy link
Contributor

trofi commented Feb 18, 2025

The typical use case for propagatedBuildInputs is when a .dev output requires another .dev output. That could happen if poco's public headers use public headers of utf8string or installed poco's .pc file refers utf8string .pc files. I think it's a natural effect of fixed unbundling.

Looking at the dev derivation of poco I see that there are cmake files to find utf8proc:

❯ rg utf8proc  /nix/store/67f3dgynfwmhj4a02l4zvvd4dlhmaf72-poco-1.14.1-dev/
/nix/store/67f3dgynfwmhj4a02l4zvvd4dlhmaf72-poco-1.14.1-dev/lib/cmake/Poco/FindUtf8Proc.cmake
5:# Find utf8proc
20:#      UTF8PROC_INCLUDE_DIR - where to find utf8proc.h
21:#      UTF8PROC_LIBRARY     - the utf8proc library
39:    UTF8PROC_INCLUDE_DIR NAMES utf8proc.h DOC "The utf8proc include directory"
43:    UTF8PROC_LIBRARY NAMES utf8proc DOC "The utf8proc library"

I don't understand enough cmake however to deduce if that means all users of poco auto require this?

I'm also not quite sure if you say that this should mean utf8proc should be a propagated dependency?

utf8proc installs a single utf8proc.h header and utf8proc library.

Let's see if any headers are directly user by poco:

$ nix build github:NixOS/nixpkgs/pull/383163/merge#poco.dev

$ fgrep -R utf8proc.h result-dev/
result-dev/lib/cmake/Poco/FindUtf8Proc.cmake:#      UTF8PROC_INCLUDE_DIR - where to find utf8proc.h
result-dev/lib/cmake/Poco/FindUtf8Proc.cmake:    UTF8PROC_INCLUDE_DIR NAMES utf8proc.h DOC "The utf8proc include directory"

Looks like poco itself does not use it as is. But it installs FindUtf8Proc.cmake which other projects might use via find_package(UTF8Proc) (or something like that, my cmake is weak). If any of them does then adding utf8proc to propagatedBuildInputs sounds like a reasonable thing to do. It's a small depend anyway.

For example litebrowser does not seem to require it? It may however get it propagated from another input. How would I find that out?

I would query the derivation graph:

$ nix-store --query --graph $(nix-instantiate -A litebrowser) |& fgrep utf8

Does not seem to contain utf8proc in the closure (it's against nixpkgs/master).

@dwt
Copy link
Contributor Author

dwt commented Feb 19, 2025

@trofi Running this command after I've built litebrowser on this branch, I get this result:

❯ nix-store --query --graph (nix-instantiate -A litebrowser --system aarch64-linux) &| rg utf8
warning: you did not specify '--add-root'; the result might be removed by the garbage collector
"kq1w8y2ciyqxkz59x8xqisnc8yvd817w-utf8proc-2.10.0.drv" -> "fv4ay05c9hn3ip9y9jdrwjn3c80hj2pj-poco-1.14.1.drv" [color = "magenta"];
"kq1w8y2ciyqxkz59x8xqisnc8yvd817w-utf8proc-2.10.0.drv" [label = "utf8proc-2.10.0.drv", shape = box, style = filled, fillcolor = "#ff0000"];
"81lwjm4ijn2vz1wim8b96m83a3la88l3-bash-5.2p37.drv" -> "kq1w8y2ciyqxkz59x8xqisnc8yvd817w-utf8proc-2.10.0.drv" [color = "black"];
"gc2x3v0z9y5rm72lbjbbkdvqdcp8zang-stdenv-linux.drv" -> "kq1w8y2ciyqxkz59x8xqisnc8yvd817w-utf8proc-2.10.0.drv" [color = "red"];
"mphxjjp4jjgyq2mh28wxz01iw12dpcmd-source.drv" -> "kq1w8y2ciyqxkz59x8xqisnc8yvd817w-utf8proc-2.10.0.drv" [color = "green"];
"rjlqmzx73qzik0rpj44hc6dp6s4blpq0-cmake-3.31.3.drv" -> "kq1w8y2ciyqxkz59x8xqisnc8yvd817w-utf8proc-2.10.0.drv" [color = "blue"];
"shkw4qm9qcw5sc5n1k5jznc83ny02r39-default-builder.sh" -> "kq1w8y2ciyqxkz59x8xqisnc8yvd817w-utf8proc-2.10.0.drv" [color = "magenta"];
"vj1c3wf9c11a0qs6p3ymfvrnsdgsdcbq-source-stdenv.sh" -> "kq1w8y2ciyqxkz59x8xqisnc8yvd817w-utf8proc-2.10.0.drv" [color = "burlywood"];

Does that mean that indeed litebrowser does contain utf8proc here?

@trofi
Copy link
Contributor

trofi commented Feb 19, 2025

@trofi Running this command after I've built litebrowser on this branch, I get this result:

❯ nix-store --query --graph (nix-instantiate -A litebrowser --system aarch64-linux) &| rg utf8
"kq1w8y2ciyqxkz59x8xqisnc8yvd817w-utf8proc-2.10.0.drv" -> "fv4ay05c9hn3ip9y9jdrwjn3c80hj2pj-poco-1.14.1.drv" [color = "magenta"];

Does that mean that indeed litebrowser does contain utf8proc here?

No, it only means that poco now depends on utf8proc. What fails if you don't add it? litebrowser? And how does it fail?

@dwt
Copy link
Contributor Author

dwt commented Feb 19, 2025

The build succeeds, but I wasn't able to test it (haven't gotten around to setting up a non headless nix that can forward ui to me yet).

@dwt dwt force-pushed the update-poco-to-1.14.1 branch from fe4f82e to d43e893 Compare February 19, 2025 17:22
@dwt
Copy link
Contributor Author

dwt commented Feb 19, 2025

@trofi: litebrowser works fine without adding utf8proc as a dependency. Does that prove that utf8proc should not be a propagatedBuildInput?

@dwt
Copy link
Contributor Author

dwt commented Feb 19, 2025

@trofi Regarding the failure of collabora-online, as far as I can determine, that is actually a failure of libreoffice-collabora, which is currently broken on master. See:

nix build --system aarch64-linux github:NixOS/nixpkgs/master#libreoffice-collabora --print-build-logs
nix build --system x86_64-linux github:NixOS/nixpkgs/master#libreoffice-collabora --print-build-logs

Is that something that should stop this getting merged?

@trofi
Copy link
Contributor

trofi commented Feb 19, 2025

@trofi: litebrowser works fine without adding utf8proc as a dependency. Does that prove that utf8proc should not be a propagatedBuildInput?

I'd say it's fine to not include it to propagatedBuildInput until we have an evidence of at least one package needing it.

@trofi Regarding the failure of collabora-online, as far as I can determine, that is actually a failure of libreoffice-collabora, which is currently broken on master. See:

nix build --system aarch64-linux github:NixOS/nixpkgs/master#libreoffice-collabora --print-build-logs
nix build --system x86_64-linux github:NixOS/nixpkgs/master#libreoffice-collabora --print-build-logs

Is that something that should stop this getting merged?

I would say this should not be blocking this PR merge to master.

@dwt dwt marked this pull request as ready for review February 20, 2025 06:47
@dwt
Copy link
Contributor Author

dwt commented Feb 20, 2025

nixpkgs-review result

Generated using nixpkgs-review.

Command: nixpkgs-review pr 383163


x86_64-darwin

⏩ 2 packages marked as broken and skipped:
  • litebrowser
  • sanjuuni
✅ 2 packages built:
  • poco
  • poco.dev

aarch64-darwin

⏩ 2 packages marked as broken and skipped:
  • litebrowser
  • sanjuuni
✅ 2 packages built:
  • poco
  • poco.dev

@dwt
Copy link
Contributor Author

dwt commented Feb 20, 2025

nixpkgs-review result

Generated using nixpkgs-review.

Command: nixpkgs-review pr 383163


aarch64-linux

❌ 1 package failed to build:
  • collabora-online
✅ 12 packages built:
  • craftos-pc
  • ioq3-scion
  • ioquake3
  • litebrowser
  • mumble
  • murmur
  • poco
  • poco.dev
  • pothos
  • quake3demo
  • sanjuuni
  • vscode-extensions.jackmacwindows.craftos-pc

@dwt
Copy link
Contributor Author

dwt commented Feb 20, 2025

Not sure why, but my x86 builds keep failing because pkg-config and ld explode at unfortunate moments. Seems my Rosetta is acting up. :/

@dwt
Copy link
Contributor Author

dwt commented Feb 21, 2025

@trofi How would I proceed to get this merged?

@dwt dwt changed the title poco: 1.13.3 -> 1.14.1 poco: 1.13.3 -> 1.14.1; fix build with new poco in dependent packages Feb 21, 2025
@trofi
Copy link
Contributor

trofi commented Feb 21, 2025

@dwt dwt force-pushed the update-poco-to-1.14.1 branch from d43e893 to 042f5b3 Compare February 21, 2025 21:40
@dwt
Copy link
Contributor Author

dwt commented Feb 21, 2025

Many thanks for @afh for the in depth review and the improvements I gained from that.

@dwt dwt force-pushed the update-poco-to-1.14.1 branch from 042f5b3 to fca7d66 Compare February 21, 2025 21:44
trofi and others added 5 commits February 21, 2025 22:52
Upstream seems to have errors, where it doesn't
include all the poco headers it requires for it
to build. I've provided a patch and
opened a pull request upstream to get it
merged: MCJack123/craftos2#391
This adds some missing includes for collabora online to actually use
the poco-json library.

Upstream pull request at
CollaboraOnline/online#11175
@dwt dwt force-pushed the update-poco-to-1.14.1 branch from fca7d66 to ab40ec5 Compare February 21, 2025 21:52
@dwt
Copy link
Contributor Author

dwt commented Feb 21, 2025

nixpkgs-review result

Generated using nixpkgs-review.

Command: nixpkgs-review pr 383163


x86_64-darwin

⏩ 2 packages marked as broken and skipped:
  • litebrowser
  • sanjuuni
✅ 2 packages built:
  • poco
  • poco.dev

aarch64-darwin

⏩ 2 packages marked as broken and skipped:
  • litebrowser
  • sanjuuni
✅ 2 packages built:
  • poco
  • poco.dev

@dwt
Copy link
Contributor Author

dwt commented Feb 21, 2025

nixpkgs-review result

Generated using nixpkgs-review.

Command: nixpkgs-review pr 383163


aarch64-linux

❌ 1 package failed to build:
  • collabora-online
✅ 12 packages built:
  • craftos-pc
  • ioq3-scion
  • ioquake3
  • litebrowser
  • mumble
  • murmur
  • poco
  • poco.dev
  • pothos
  • quake3demo
  • sanjuuni
  • vscode-extensions.jackmacwindows.craftos-pc

@dwt
Copy link
Contributor Author

dwt commented Feb 23, 2025

I have some hope that #384586 fixes the collabora-online build, but I do not have machines able to build libreoffice at this time. :-/

@dwt
Copy link
Contributor Author

dwt commented Feb 23, 2025

@trofi Is there anything else you would require to get this pull request merged? I would love some feedback.

@trofi
Copy link
Contributor

trofi commented Feb 23, 2025

@dwt It looks good to me. Note that I'm not a nixpkgs committer and can't merge any PRs.

@dwt
Copy link
Contributor Author

dwt commented Feb 24, 2025

Pinging Maintainers of the touched packages, to help me get this merged:

Copy link
Member

@jnsgruk jnsgruk left a comment

Choose a reason for hiding this comment

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

LGTM for Multipass, thank you!

@wegank wegank added the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Feb 24, 2025
Copy link
Member

@felixsinger felixsinger left a comment

Choose a reason for hiding this comment

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

Changes to Mumble look good to me except for one.

@dwt dwt force-pushed the update-poco-to-1.14.1 branch from ab40ec5 to 33f6d24 Compare February 25, 2025 20:12
Copy link
Member

@tomodachi94 tomodachi94 left a comment

Choose a reason for hiding this comment

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

Changes to Poco and CraftOS-PC lgtm, with the added note that the patch should probably be submitted upstream (the upstream maintainer generally appreciates patches like this, that fix compatibility with bleeding-edge versions of libraries and the like).

@dwt
Copy link
Contributor Author

dwt commented Feb 26, 2025

Changes to Poco and CraftOS-PC lgtm, with the added note that the patch should probably be submitted upstream (the upstream maintainer generally appreciates patches like this, that fix compatibility with bleeding-edge versions of libraries and the like).

  • Poco: Already upstream, that's why I was able to use fetchpatch in the derivation
  • craftos-pc: linked in the derivation

@wegank wegank added 12.approvals: 2 This PR was reviewed and approved by two reputable people 12.approved-by: package-maintainer This PR was reviewed and approved by a maintainer listed in the package and removed 12.approvals: 1 This PR was reviewed and approved by one reputable person labels Feb 26, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
10.rebuild-darwin: 1-10 10.rebuild-linux: 11-100 12.approvals: 2 This PR was reviewed and approved by two reputable people 12.approved-by: package-maintainer This PR was reviewed and approved by a maintainer listed in the package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants