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 #369312

Closed
wants to merge 4 commits into from
Closed

poco: 1.13.3 -> 1.14.1 #369312

wants to merge 4 commits into from

Conversation

trofi
Copy link
Contributor

@trofi trofi commented Dec 30, 2024

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
Copy link
Contributor Author

trofi commented Dec 30, 2024

nixpkgs-review result

Generated using nixpkgs-review.

Command: nixpkgs-review pr 369312


x86_64-linux

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

@trofi trofi marked this pull request as draft December 30, 2024 06:57
@trofi
Copy link
Contributor Author

trofi commented Dec 30, 2024

All failures are regressions. Converting to draft.

@ofborg ofborg bot requested a review from tomodachi94 December 30, 2024 12:30
@dwt
Copy link
Contributor

dwt commented Feb 15, 2025

nixpkgs-review result

Generated using nixpkgs-review.

Command: nixpkgs-review pr 369312


aarch64-darwin

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

@dwt
Copy link
Contributor

dwt commented Feb 15, 2025

This update actually fixes compilation of poco on darwin, as it includes this fix, which previously prevented compilation with clang 19. Since I would like to port mumble and ioquake3 which depend on this, is there something I can do to get this moving forward?

I noticed that there is actually already a 1.14.1 release of poco, maybe updating to that could help?

@trofi trofi changed the title poco: 1.13.3 -> 1.14.0 poco: 1.13.3 -> 1.14.1 Feb 15, 2025
@trofi
Copy link
Contributor Author

trofi commented Feb 15, 2025

I updated to 1.14.1, but I suspect that the API change did not go away and most dependencies still break with new release. Ran the nixpkgs-review 369312 locally.

@dwt
Copy link
Contributor

dwt commented Feb 15, 2025

I tried doing this update and running nixpkgs-review locally, but I never got it to build just poco and its dependencies, could you perhaps share your workflow for running nixpkgs-review locally? Perhaps I'm missing something obvious.

I've tried nixpkgs-review rev HEAD (that built way too much) and nixpkgs-review rev HEAD --package poco (that only built poco, no dependent packages). Is there a different way to test local changes without creating a pr first that focuses better on just the packages changed by a pull request?

@dwt
Copy link
Contributor

dwt commented Feb 15, 2025

nixpkgs-review result

Generated using nixpkgs-review.

Command: nixpkgs-review pr 369312


aarch64-darwin

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

@dwt
Copy link
Contributor

dwt commented Feb 15, 2025

nixpkgs-review result

Generated using nixpkgs-review.

Command: nixpkgs-review pr 369312


aarch64-linux

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

@trofi
Copy link
Contributor Author

trofi commented Feb 15, 2025

I tried doing this update and running nixpkgs-review locally, but I never got it to build just poco and its dependencies, could you perhaps share your workflow for running nixpkgs-review locally? Perhaps I'm missing something obvious.

I've tried nixpkgs-review rev HEAD (that built way too much) and nixpkgs-review rev HEAD --package poco (that only built poco, no dependent packages). Is there a different way to test local changes without creating a pr first that focuses better on just the packages changed by a pull request?

I just run $ nixpkgs-review pr --no-shell --post-result --build-args '--max-jobs 1' 369312 and wait A Lot (I use a tiny wrapper around it to call it as nr $pr: https://github.com/trofi/home/blob/master/bin/nr).

@trofi
Copy link
Contributor Author

trofi commented Feb 15, 2025

nixpkgs-review result

Generated using nixpkgs-review.

Command: nixpkgs-review pr 369312


x86_64-linux

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

@dwt
Copy link
Contributor

dwt commented Feb 15, 2025

I've looked into the build failures from pothos, and found two problems:

  • I'ts not using c++17 to build, which fails because poco 1.14 requires it. Enabling that gets the build further, until it fails again because this 5 (!) year old upstream fix does not seem to be part of the release we use

That is kind of a shame, as this concrete error is probably easy to fix, and upstream already has some fixes. However, the upstream project looks quite dormant, so I don't have high hopes for an upstream fix. Even though, I've opened a bug there about the issue

@dwt
Copy link
Contributor

dwt commented Feb 15, 2025

I actually did get pothos to compile with this patch:

diff --git a/pkgs/applications/radio/pothos/cstring.patch b/pkgs/applications/radio/pothos/cstring.patch
new file mode 100644
index 000000000000..4e8957c3f122
--- /dev/null
+++ b/pkgs/applications/radio/pothos/cstring.patch
@@ -0,0 +1,50 @@
+Submodule blocks contains modified content
+diff --git a/blocks/file/BinaryFileSink.cpp b/blocks/file/BinaryFileSink.cpp
+index 31c9a41..0083b0d 100644
+--- a/blocks/file/BinaryFileSink.cpp
++++ b/blocks/file/BinaryFileSink.cpp
+@@ -13,6 +13,7 @@
+ #endif //_MSC_VER
+ #include <stdio.h>
+ #include <cerrno>
++#include <cstring>
+
+ #ifndef O_BINARY
+ #define O_BINARY 0
+diff --git a/blocks/file/BinaryFileSource.cpp b/blocks/file/BinaryFileSource.cpp
+index 0151231..379d383 100644
+--- a/blocks/file/BinaryFileSource.cpp
++++ b/blocks/file/BinaryFileSource.cpp
+@@ -13,6 +13,7 @@
+ #endif //_MSC_VER
+ #include <stdio.h>
+ #include <cerrno>
++#include <cstring>
+
+ #ifndef O_BINARY
+ #define O_BINARY 0
+diff --git a/blocks/file/TextFileSink.cpp b/blocks/file/TextFileSink.cpp
+index b4b2f08..2be66e2 100644
+--- a/blocks/file/TextFileSink.cpp
++++ b/blocks/file/TextFileSink.cpp
+@@ -6,6 +6,7 @@
+ #include <complex>
+ #include <fstream>
+ #include <cerrno>
++#include <cstring>
+
+ /***********************************************************************
+  * |PothosDoc Text File Sink
+Submodule soapy contains modified content
+diff --git a/soapy/DemoController.cpp b/soapy/DemoController.cpp
+index 4ce8ead..9a4e742 100644
+--- a/soapy/DemoController.cpp
++++ b/soapy/DemoController.cpp
+@@ -6,6 +6,7 @@
+ #include <iostream>
+ #include <chrono>
+ #include <algorithm> //min/max
++#include <cstring>
+
+ /***********************************************************************
+  * |PothosDoc SDR Demo Controller
diff --git a/pkgs/applications/radio/pothos/default.nix b/pkgs/applications/radio/pothos/default.nix
index 641df521c7fd..0cb55472d342 100644
--- a/pkgs/applications/radio/pothos/default.nix
+++ b/pkgs/applications/radio/pothos/default.nix
@@ -41,8 +41,13 @@ mkDerivation rec {
       url = "https://github.com/pothosware/PothosCore/commit/092d1209b0fd0aa8a1733706c994fa95e66fd017.patch";
       hash = "sha256-bZXG8kD4+1LgDV8viZrJ/DMjg8UvW7b5keJQDXurfkA=";
     })
+    # various source files are missing imports of <cstring>
+    ./cstring.patch
   ];
 
+  # poco 1.14 requires c++17
+  NIX_CFLAGS_COMPILE = [ "-std=gnu++17" ];
+
   nativeBuildInputs = [
     cmake
     pkg-config

Caveat: Even though I have a linux builder, I don't (yet) have a linux machine capable of actually testing this package.

@trofi Do you think it viable to include something like that in in this pull request? Do you think fixing the broken packages in this way a viable way forward to get this poco update into nixpkgs?

@trofi
Copy link
Contributor Author

trofi commented Feb 15, 2025

Yeah, that sounds great. If you want you can sent PRs against https://github.com/trofi/nixpkgs/tree/poco-update and I'll apply them to appear in this proposal.

@trofi
Copy link
Contributor Author

trofi commented Feb 15, 2025

Or alternatively I can close this PR so you could create the PR and fix all the fallouts.

@dwt
Copy link
Contributor

dwt commented Feb 15, 2025

I'd be happy to send you pull requests against your branch, as I'm still quite new to nixpkgs and would love your guidance.

@dwt
Copy link
Contributor

dwt commented Feb 15, 2025

Here you go

@dwt
Copy link
Contributor

dwt commented Feb 15, 2025

I just run $ nixpkgs-review pr --no-shell --post-result --build-args '--max-jobs 1' 369312 and wait A Lot (I use a tiny wrapper around it to call it as nr $pr: https://github.com/trofi/home/blob/master/bin/nr).

That means you create the pull request first and only then test using nixpkgs-review? I was hoping for a workflow where I could check before posting the pull request.

@trofi
Copy link
Contributor Author

trofi commented Feb 15, 2025

Here you go

Pulled. Thank you!

@dwt
Copy link
Contributor

dwt commented Feb 15, 2025

Investigating the build further, I found that the option

 configureFlags = [
   "--unbundled"
 ];

does not actually have any effect, as cmake is used to build this. It was changed from a cmake flag to a configure flag a while back, to fix the nix build .#pkgsStatic.poco build. That earlier fix seems faulty - not sure it was ever correct. Reverting that, I found that there was actually a dependency missing, and upstream seems to have a bug with static builds. I have provided a workaround as a second pull request to you @trofi.

@trofi
Copy link
Contributor Author

trofi commented Feb 16, 2025

Pulled the PR and slightly reworded the comment

@dwt
Copy link
Contributor

dwt commented Feb 16, 2025

Pulled the PR and slightly reworded the comment

Sorry, I did push some additional commits that also fixed the comments. For example, the patch actually disables the internal pcre artifacts for static builds, not all the time. :/ Not quite sure what happened, but those didn't seem to become part of the pull request. I'll create a new one for that.

See trofi#4

@dwt
Copy link
Contributor

dwt commented Feb 16, 2025

And another pull request to get craftos-pc compiling with this poco version

@dwt
Copy link
Contributor

dwt commented Feb 16, 2025

@dwt
Copy link
Contributor

dwt commented Feb 16, 2025

I think this should be it, as far as I can see this fixes all the builds that are failing because of the poco update. Do you see anything else that is missing to make this mergeable?

trofi and others added 3 commits February 16, 2025 21:03
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
@trofi
Copy link
Contributor Author

trofi commented Feb 16, 2025

And the last pull request to fix the build of collabora-online

Pulled it, rebased, squashed and renamed commits to have consistent $pkg: prefix.

@trofi
Copy link
Contributor Author

trofi commented Feb 16, 2025

nixpkgs-review result

Generated using nixpkgs-review.

Command: nixpkgs-review pr 369312


x86_64-linux

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

@dwt
Copy link
Contributor

dwt commented Feb 17, 2025

Ah damn. More work. At least, now I'm getting to the packages I initially wanted to work on. Ah well.

Pull request for Pothos is ready

@dwt
Copy link
Contributor

dwt commented Feb 17, 2025

And another one for mumble

As I was not yet able to find good documentation: At what time would I consider using a propagatedBuildInput for something like utf8proc? If all the libraries that use poco need it, should it then become a propagated dependency?

@dwt
Copy link
Contributor

dwt commented Feb 17, 2025

Not quite sure what's up with multipass my build fails with an error like this, that seems... odd?

┃ error: builder for '/nix/store/76m5rq1q63h3pm2c4ql1myx673c06zc8-flutter-cache-dir.drv' failed with exit code 1;
┃        last 1 log lines:
┃        > error: executing '/nix/store/ck4s3x3jvml21zpny04xg7fbi8qpxjax-bash-5.2p37/bin/bash': Undefined error: 0
┃        For full logs, run 'nix log /nix/store/76m5rq1q63h3pm2c4ql1myx673c06zc8-flutter-cache-dir.drv'.

@trofi what kind of failures do you get?

@trofi
Copy link
Contributor Author

trofi commented Feb 17, 2025

$ nix build --no-link github:NixOS/nixpkgs/pull/369312/merge#multipass -L
...
multipassd> CMake Error at /nix/store/wjb1rq9xgxzq6n1b7pbrbsk5lrdn9qc7-cmake-3.31.3/share/cmake-3.31/Modules/FindPackageHandleStandardArgs.cmake:233 (message):
multipassd>   Could NOT find Utf8Proc (missing: UTF8PROC_LIBRARY UTF8PROC_INCLUDE_DIR)

@trofi
Copy link
Contributor Author

trofi commented Feb 17, 2025

Let me close this PR. I don't provide much value here. Please reopen a new one with your fixes. I can run build tests against it and provide some basic explanation around build failures.

@trofi trofi closed this Feb 17, 2025
@dwt
Copy link
Contributor

dwt commented Feb 18, 2025

Well, the nix-know-how you provide is (and will be) much appreciated. Since I'm very much new to nix (just started using it a few weeks ago) I very much don't know how to do to many things and wether they are acceptable or right / idiomatic for nix. So hopefully I can count on your know how for those things in that follow-up pull request.

@dwt
Copy link
Contributor

dwt commented Feb 18, 2025

For all upstream projects that where linked to this pull request: please see the continuation of this work at #383163

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants