Skip to content

Comments

python3Packages.lttng: init at 2.13.11#285390

Merged
bjornfor merged 2 commits intoNixOS:masterfrom
hacker1024:package/python3-lttng
Feb 14, 2024
Merged

python3Packages.lttng: init at 2.13.11#285390
bjornfor merged 2 commits intoNixOS:masterfrom
hacker1024:package/python3-lttng

Conversation

@hacker1024
Copy link
Member

@hacker1024 hacker1024 commented Jan 31, 2024 •
edited
Loading

Description of changes

This package contains Python bindings to liblttng-ctl from LTTng‑tools.

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/)
  • 24.05 Release Notes (or backporting 23.05 and 23.11 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.

@bjornfor

@github-actions github-actions bot added the 6.topic: python Python is a high-level, general-purpose programming language. label Jan 31, 2024
@ofborg ofborg bot added the 8.has: package (new) This PR adds a new package label Feb 1, 2024
@ofborg ofborg bot requested a review from bjornfor February 1, 2024 00:27
@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. labels Feb 1, 2024
bjornfor
bjornfor approved these changes Feb 1, 2024
View reviewed changes
Copy link
Contributor

@bjornfor bjornfor left a comment

Choose a reason for hiding this comment

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

LGTM.

The two commits can be squashed.

@delroth delroth added 12.approvals: 1 This PR was reviewed and approved by one person. 12.approved-by: package-maintainer This PR was reviewed and approved by a maintainer listed in any of the changed packages. labels Feb 1, 2024
@bjornfor
Copy link
Contributor

bjornfor commented Feb 1, 2024

Maybe you can amend the commit and say in the commit body why this is a separate attribute which rebuilds lttng-tools, instead of adding python support directly to lttng-tools? (I'm guessing it's about closure size?)

@hacker1024 hacker1024 force-pushed the package/python3-lttng branch from 0e156ba to 93a6158 Compare February 1, 2024 22:53
@hacker1024
Copy link
Member Author

hacker1024 commented Feb 1, 2024

LGTM.

The two commits can be squashed.

Maybe you can amend the commit and say in the commit body why this is a separate attribute which rebuilds lttng-tools, instead of adding python support directly to lttng-tools? (I'm guessing it's about closure size?)

Done and done.

@delroth delroth removed the 12.approvals: 1 This PR was reviewed and approved by one person. label Feb 1, 2024
@ofborg ofborg bot requested a review from bjornfor February 1, 2024 23:19
@hacker1024 hacker1024 force-pushed the package/python3-lttng branch from 93a6158 to 33972b4 Compare February 2, 2024 00:09
@hacker1024
Copy link
Member Author

hacker1024 commented Feb 2, 2024

I have switched to using the PYTHON and PYTHON_CONFIG environment variables to avoid polluting the nativeBuildInputs.

@bjornfor
Copy link
Contributor

bjornfor commented Feb 2, 2024

Done and done.

Thanks.

I have switched to using the PYTHON and PYTHON_CONFIG environment variables to avoid polluting the nativeBuildInputs.

Why is putting a build input in nativeBuildInputs "polluting" it? Using those variables seem more complicated to me. Or am I missing something?

Nit: Commit messages should be wrapped at 72 chars to avoid overlong lines.

@hacker1024
Copy link
Member Author

hacker1024 commented Feb 7, 2024

Why is putting a build input in nativeBuildInputs "polluting" it? Using those variables seem more complicated to me. Or am I missing something?

Nix treats nativeBuildInputs specially for cross-compilation, but in this case, cross-compilation is accounted for explicitly. Using the variables ensures that the platform setup isn't messed with further. It also allows regular Python to be added in the future if it is ever needed.

@bjornfor
Copy link
Contributor

bjornfor commented Feb 8, 2024

Why is putting a build input in nativeBuildInputs "polluting" it? Using those variables seem more complicated to me. Or am I missing something?

Nix treats nativeBuildInputs specially for cross-compilation, but in this case, cross-compilation is accounted for explicitly. Using the variables ensures that the platform setup isn't messed with further. It also allows regular Python to be added in the future if it is ever needed.

Put that in a code comment maybe?

@hacker1024
python3Packages.lttng: init at 2.13.11
1cc8e03 
A new attribute overriding lttng-tools is created, rather than enabling
Python support in lttng-tools itself, in order to reduce the closure
size and leave lttng-tools Python-version-agnostic.
@hacker1024
python3Packages.lttng: Explain use of PYTHON environment variable
d38fa22
@hacker1024 hacker1024 force-pushed the package/python3-lttng branch from 33972b4 to d38fa22 Compare February 12, 2024 02:12
@hacker1024
Copy link
Member Author

hacker1024 commented Feb 12, 2024

I have added the comment and wrapped commit messages to 72 lines.

@bjornfor bjornfor merged commit ac6c0ae into NixOS:master Feb 14, 2024
This was referenced Feb 16, 2024
Closed
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bjornfor bjornfor bjornfor approved these changes

Assignees

No one assigned

Labels

6.topic: python Python is a high-level, general-purpose programming language. 8.has: package (new) This PR adds a new package 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. 12.approved-by: package-maintainer This PR was reviewed and approved by a maintainer listed in any of the changed packages.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@hacker1024 @bjornfor @delroth