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

cpython: provide libuuid for the _uuid module #377458

Open
wants to merge 2 commits into
base: staging
Choose a base branch
from

Conversation

mweinelt
Copy link
Member

@mweinelt mweinelt commented Jan 28, 2025

Relying on libuuid offers synchronization primitives, so that "no two
processes can obtain the same UUID"¹.

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.

@booxter
Copy link
Contributor

booxter commented Jan 28, 2025

I think I misled you in Matrix chat. What's actually happening is that Popen() is called once per process, so it's not a big deal performance-wise. Actually, the in-python implementation using clock source is more performant than calling to _uuid -> libuuid. See:

$ cat test.py
import uuid

for i in range(1000000):
    _ = uuid.uuid1()

$ time result/bin/python test.py # <- python with _uuid
result/bin/python test.py  5.03s user 6.76s system 96% cpu 12.160 total

$ time python test.py # <- python without _uuid
python test.py  3.84s user 0.30s system 99% cpu 4.172 total

But: it looks like unless _uuid module is used to generate UUID, it will be marked as is_safe=unknown, as per https://docs.python.org/3/library/uuid.html#uuid.SafeUUID which may be important to users of the module if they want to guarantee uniqueness for uuid across processes (which makes sense, since clock is used as the source of "randomness" otherwise). So maybe for the sake of safety - and to align with other platforms like RHEL or even Darwin nixpkgs builds? - we may want to still switch to libuuid. Something to think about.

(UPD: Looks like while Darwin nixpkgs build ships _uuid, the platform does not support _safe variant of the uuid_generate_time function, so it's not an example to emulate.)

@booxter
Copy link
Contributor

booxter commented Jan 28, 2025

Yes, let's do it (on staging, obviously). 🙏

@mweinelt
Copy link
Member Author

mweinelt commented Jan 28, 2025

Doing some test builds on my hydra and using master as a base for comparisons.

Not sure if the util-linuxMinimal changes are fine.

These depend on python via audit preventing the use of libuuid from
util-linux in python builds.
Relying on libuuid offers synchronization primitives, so that "no two
processes can obtain the same UUID"¹.

[1] https://docs.python.org/3/library/uuid.html#module-uuid
@mweinelt mweinelt changed the base branch from master to staging January 29, 2025 02:08
@ofborg ofborg bot added the 2.status: merge conflict This PR has merge conflicts with the target branch label Jan 29, 2025
@mweinelt mweinelt marked this pull request as ready for review January 29, 2025 02:08
@ofborg ofborg bot removed the 2.status: merge conflict This PR has merge conflicts with the target branch label Jan 29, 2025
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