Skip to content

python312Packages.granian: don't force a custom allocator #414234

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

K900
Copy link
Contributor

@K900 K900 commented Jun 5, 2025

Fixes #414214

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/)
  • Nixpkgs 25.11 Release Notes (or backporting 24.11 and 25.05 Nixpkgs Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
  • NixOS 25.11 Release Notes (or backporting 24.11 and 25.05 NixOS Release notes)
    • (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.

@pbsds
Copy link
Member

pbsds commented Jun 5, 2025

I'm currently testing

diff --git a/pkgs/development/python-modules/granian/default.nix b/pkgs/development/python-modules/granian/default.nix
index 6c9b5ceba619..77b5a22cacc4 100644
--- a/pkgs/development/python-modules/granian/default.nix
+++ b/pkgs/development/python-modules/granian/default.nix
@@ -19,6 +19,17 @@
   nix-update-script,
 }:
 
+let
+  rust-jemalloc-sys' = rust-jemalloc-sys.override (old: {
+    jemalloc = old.jemalloc.override {
+      # "libjemalloc.so.2: cannot allocate memory in static TLS block"
+      disableInitExecTls = true;
+    };
+  });
+in
+
+assert builtins.elem "--disable-initial-exec-tls" rust-jemalloc-sys'.configureFlags;
+
 buildPythonPackage rec {
   pname = "granian";
   version = "2.3.2";
@@ -41,20 +52,11 @@ buildPythonPackage rec {
     maturinBuildHook
   ];
 
-  buildInputs = lib.optionals (stdenv.hostPlatform.isAarch64) [
+  buildInputs = [
     # fix "Unsupported system page size" on aarch64-linux with 16k pages
     # https://github.com/NixOS/nixpkgs/issues/410572
-    # only enabled on aarch64 due to https://github.com/NixOS/nixpkgs/pull/410611#issuecomment-2939564567
-    (rust-jemalloc-sys.overrideAttrs (
-      { configureFlags, ... }:
-      {
-        configureFlags = configureFlags ++ [
-          # otherwise import check fails with:
-          # ImportError: {{storeDir}}/lib/libjemalloc.so.2: cannot allocate memory in static TLS block
-          "--disable-initial-exec-tls"
-        ];
-      }
-    ))
+    # Another alternative may be to try `mimalloc`
+    rust-jemalloc-sys'
   ];
 
   dependencies = [

but this is also a sound approach

@K900
Copy link
Contributor Author

K900 commented Jun 5, 2025

Honestly forcing a global allocator at library level will never be a good idea, for any reason.

@gi0baro
Copy link

gi0baro commented Jun 5, 2025

Honestly forcing a global allocator at library level will never be a good idea, for any reason.

Granian maintainer here. I'm not a big fan of never/always takes.
There are valid reasons behind the choice of using custom allocators in Granian, those are not there just because it was fun.
I could make a similar take saying "honestly patching whatever piece of sw 'cause your build process is broken will never be a good idea" but that won't be much productive, am I right?
Given the wheels produced by Granian build process work on any manylinux x86 target, I would just say IMHO the way to fix this should be to just copy the build process you can find in the CI code in the Granian repo. But also, I'm not a nix expert, so I will just stay out of this.

@K900
Copy link
Contributor Author

K900 commented Jun 5, 2025

OK, let me rephrase that: replacing the global allocator from a library is exceptionally brittle as it effectively makes the behavior dependent on the load/constructor order. I understand the performance concerns here, but also, "it works on x86-manylinux", as you can see, is clearly not enough.

@gi0baro
Copy link

gi0baro commented Jun 5, 2025

I understand the performance concerns here, but also, "it works on x86-manylinux", as you can see, is clearly not enough.

I can't ensure that on 3rd party builds. I was talking about the official wheels produced by the project.
Also, why you need to recompile the whole thing? Is it because the the following dyn links are not avail on nix?

((.venv) ) root@98f8095abb14:/# ldd .venv/lib/python3.12/site-packages/granian/_granian.cpython-312-x86_64-linux-gnu.so
        libgcc_s.so.1 => /lib/x86_64-linux-gnu/libgcc_s.so.1 (0x00007ffffed2e000)
        librt.so.1 => /lib/x86_64-linux-gnu/librt.so.1 (0x00007ffffed29000)
        libpthread.so.0 => /lib/x86_64-linux-gnu/libpthread.so.0 (0x00007ffffed24000)
        libm.so.6 => /lib/x86_64-linux-gnu/libm.so.6 (0x00007ffffec44000)
        libdl.so.2 => /lib/x86_64-linux-gnu/libdl.so.2 (0x00007ffffec3f000)
        libc.so.6 => /lib/x86_64-linux-gnu/libc.so.6 (0x00007ffffea5e000)
        /lib64/ld-linux-x86-64.so.2 (0x00007ffffffcb000)

@gi0baro
Copy link

gi0baro commented Jun 5, 2025

Also, not sure why the patch for disabling the initial tls exec was applied on aarch64 target only, given the original cfg does that also on x86: https://github.com/emmett-framework/granian/blob/master/Cargo.toml#L60
On aarch64 the page size patch should be done in addition (https://github.com/emmett-framework/granian/blob/master/.github/workflows/release.yml#L112-L115).

Anyways, if the build process is too painful for nix, in 2.4 I can gate also jemalloc behind a feature cfg, in the same way we already do for mimalloc, so that you should be fine without patching anything?

(your patch here also drop mimalloc, but unless you explicitly set the feature cfg in the build process that won't be picked up)

@K900
Copy link
Contributor Author

K900 commented Jun 6, 2025

The problem isn't the build process, it's that Granian simply crashes if something uses the system allocator, then gets jemalloc'd.

@robert-elles
Copy link
Contributor

robert-elles commented Jun 6, 2025

when trying to apply this patch on latest nixpks unstable i get:

       error: builder for '/nix/store/wayp2if4h5wm5g4ys5z20f3cn24xsdm2-nixpkgs-patched.drv' failed with exit code 1;
       last 11 log lines:
       > Running phase: unpackPhase
       > unpacking source archive /nix/store/1zw47fx5h4x65n914j4b9iz0j3v17aw0-source
       > source root is source
       > Running phase: patchPhase
       > applying patch /nix/store/5id8m1p7xls62zdk2wkwpy881zbim4q7-414234.patch
       > patching file pkgs/development/python-modules/granian/default.nix
       > Reversed (or previously applied) patch detected!  Assume -R? [n]
       > Apply anyway? [n]
       > Skipping patch.
       > 4 out of 4 hunks ignored -- saving rejects to file pkgs/development/python-modules/granian/default.nix.rej
       > patching file pkgs/development/python-modules/granian/no-alloc.patch

@K900
Copy link
Contributor Author

K900 commented Jun 6, 2025

Are you maybe trying to apply it to an older nixpkgs version?

@gi0baro
Copy link

gi0baro commented Jun 6, 2025

The problem isn't the build process, it's that Granian simply crashes if something uses the system allocator, then gets jemalloc'd.

As I said

Also, not sure why the patch for disabling the initial tls exec was applied on aarch64 target only, given the original cfg does that also on x86: https://github.com/emmett-framework/granian/blob/master/Cargo.toml#L60

So I think you're not looking at the root cause for this, but rather trying to solve a side effect. IMHO the change @pbsds proposed actually tackle the real issue here.
On aarch64 the only thing left at that point is to set the correct page size, as I stated few lines above:

On aarch64 the page size patch should be done in addition (https://github.com/emmett-framework/granian/blob/master/.github/workflows/release.yml#L112-L115).

When you launch whatever application with Granian, Granian itself is the entry point, thus having whatever allocator on its side should be perfectly fine.

If you just don't want to deal with it, again, the only thing I can do to simplify downstream management is to gate the jemalloc thing behind a feature in 2.4. but for 2.3, correctly configuring jemalloc should just work.

@SuperSandro2000
Copy link
Member

Also, why you need to recompile the whole thing? Is it because the the following dyn links are not avail on nix?

NixOS is a dsitro, hence we try to compile everything on our own. Otherwise security updates to libraries would be even more impossible as they are right now and distro specific patches to some software would be hard to apply.

Also those links do not exist on NixOS but we could make it work with patchelf.

On aarch64 the page size patch should be done in addition (emmett-framework/granian@master/.github/workflows/release.yml#L112-L115).

We already do that for jemalloc

Anyways, if the build process is too painful for nix

Na, so far we can do everything, we just need to figure out what and where exactly 😂

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.

nixos/paperless: granian asginl dumped core while starting paperless
5 participants