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.

Sign up for GitHub

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

Jump to bottom

blender: allow functional declaration within withPackages #264292

Merged
merged 1 commit into from
Nov 3, 2023
Merged

blender: allow functional declaration within withPackages #264292

merged 1 commit into from
Nov 3, 2023

Conversation

the-furry-hubofeverything
Copy link
Contributor

@the-furry-hubofeverything the-furry-hubofeverything commented Oct 30, 2023 •
edited
Loading

Description of changes

Originally included in #257780, separated since it used to introduce breaking changes.
(not anymore, now blender-with-packages is being deprecated.)

This allows the following syntax to be used

blender.withPackages(ps: [ ps.bpycv ])

instead of

blender-with-packages { packages = [ python310Packages.bpycv ]; }

Allowing withPackages to use the pythonPackages from the blender package.

I've updated passthru.tests.render for bpycv to use the new implementation.

blender-with-packages is deprecated, as the new implementation breaks the currently used syntax, and it would have trouble integrating with #257780, where multiple versioned variants of blender would be involved.

Also passed through pythonPackages to pass thorugh any overrides applied to blender's current python310Packages, and without splicing issues (discussed in #257780)

*to test, enter nix-build -I nixpkgs=./ -E 'with (import <nixpkgs> {}); blender.withPackages(ps: [ps.bpycv])', then opening blender-wrapped, go to the "scripting" workplace (top tabs), and try importing bpycv through the python terminal`

or try nix-build -A blender.pythonPackages.bpycv.tests.render

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

@github-actions github-actions bot added 6.topic: python 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: documentation This PR adds or changes documentation 8.has: changelog labels Oct 30, 2023
@the-furry-hubofeverything the-furry-hubofeverything added this to the 23.11 milestone Oct 30, 2023
@the-furry-hubofeverything
Copy link
Contributor Author

While rebasing the other PR onto this I noticed I forgot to remove blender-with-packages from bpycv

@the-furry-hubofeverything the-furry-hubofeverything mentioned this pull request Oct 30, 2023
Closed
21 tasks
@the-furry-hubofeverything the-furry-hubofeverything force-pushed the blender-withPackages-test branch 2 times, most recently from 7aba915 to 3fe85a3 Compare October 30, 2023 04:10
@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 labels Oct 30, 2023
@the-furry-hubofeverything
Copy link
Contributor Author

That should be everything, mostly the breaking changes part.

@the-furry-hubofeverything
Copy link
Contributor Author

Failing checks may be related to #264290, rebasing...

@the-furry-hubofeverything the-furry-hubofeverything mentioned this pull request Oct 30, 2023
Closed
38 tasks
@lucasew
Copy link
Contributor

lucasew commented Oct 30, 2023

Result of nixpkgs-review pr 264292 run on x86_64-linux 1

2 packages blacklisted:
  • nixos-install-tools
  • tests.nixos-functions.nixos-test

@lucasew
Copy link
Contributor

lucasew commented Oct 30, 2023

bpycv test is working fine

pbsds
pbsds reviewed Oct 30, 2023
View reviewed changes
Copy link
Member

@pbsds pbsds left a comment

Choose a reason for hiding this comment

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

LGTM, mainly nits from me

nixos/doc/manual/release-notes/rl-2311.section.md Outdated Show resolved Hide resolved
pkgs/applications/misc/blender/default.nix Outdated Show resolved Hide resolved
pkgs/applications/misc/blender/default.nix Outdated Show resolved Hide resolved
pkgs/applications/misc/blender/default.nix Outdated Show resolved Hide resolved
pkgs/development/python-modules/bpycv/default.nix Outdated Show resolved Hide resolved
@the-furry-hubofeverything
Copy link
Contributor Author

@pbsds nits picked

Followed @K900's advice and made blender.withPackages backwards compatible with blender-with-packages. All functionality is preserved... I think.

This would no longer be a breaking change, but blender-with-packages will be deprecated.

@ofborg ofborg bot added the 2.status: merge conflict This PR has merge conflicts with the target branch label Oct 31, 2023
@the-furry-hubofeverything
Copy link
Contributor Author

Oh dear

@the-furry-hubofeverything
Copy link
Contributor Author

Should be alright now

Additionally -

  • Moved the manual entry into "Backward Incompatibilities" instead of "Other notable changes"
  • Moved blender-with-packages from all-packages.nix to aliases.nix

@the-furry-hubofeverything the-furry-hubofeverything removed the 2.status: merge conflict This PR has merge conflicts with the target branch label Oct 31, 2023
Mindavi
Mindavi reviewed Oct 31, 2023
View reviewed changes
pkgs/top-level/aliases.nix Outdated Show resolved Hide resolved
@the-furry-hubofeverything the-furry-hubofeverything added the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Oct 31, 2023
@the-furry-hubofeverything
Copy link
Contributor Author

the-furry-hubofeverything commented Oct 31, 2023 •
edited
Loading

@K900 @Mindavi

Sorry about the quick changes, but since you folks made some points to clarify the documentation, I've updated both the documentation and aliases.nix to clarify the new function better.

https://github.com/NixOS/nixpkgs/compare/a8ac511c21fd37472a54d93ce492ac04dcf2f654..b60862aeeb5ddb9b10a536f7eb99e283f289b3a8

@delroth delroth removed the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Oct 31, 2023
@the-furry-hubofeverything the-furry-hubofeverything force-pushed the blender-withPackages-test branch 2 times, most recently from b030893 to 5b20db7 Compare October 31, 2023 23:54
@the-furry-hubofeverything
Copy link
Contributor Author

Alright, no more name, if people really wants to they could rename the whole thing by using overrideAttrs

@the-furry-hubofeverything
Copy link
Contributor Author

Here's the diff: https://github.com/NixOS/nixpkgs/compare/b60862aeeb5ddb9b10a536f7eb99e283f289b3a8..327dbdf62c271a95d681a120b7a28d9c0b85c3fd

@the-furry-hubofeverything
Copy link
Contributor Author

I think a refactor of wrapper.nix is required, but I think that should be done later, not with this PR.

K900
K900 reviewed Nov 3, 2023
View reviewed changes
pkgs/top-level/aliases.nix Outdated
@@ -92,6 +92,7 @@ mapAliases ({
bird2 = bird; # Added 2022-02-21
bitwig-studio1 = throw "bitwig-studio1 has been removed, you can upgrade to 'bitwig-studio'"; # Added 2023-01-03
bitwig-studio2 = throw "bitwig-studio2 has been removed, you can upgrade to 'bitwig-studio'"; # Added 2023-01-03
blender-with-packages = args: lib.warn "blender-with-packages is deprecated in favor of blender.withPackages, which behaves similarly to python3.withPackages" (blender.withPackages (_: args.packages)).overrideAttrs (lib.optionalAttrs (args ? name) { pname = "blender-" + args.name; }); # Added 2023-10-30
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe add some line breaks here? This line really feels like a lot.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Don't worry about it. Also, the message should probably refer people to blender.withPackages.

told ya. I'll do something about it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Does this work?

https://github.com/NixOS/nixpkgs/blob/521cd5a7d283a5ef655eacba58dd488d980a03d4/pkgs/top-level/aliases.nix#L95-L97

Copy link
Contributor

Choose a reason for hiding this comment

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

Definitely more readable, yeah. Also, I'd probably drop the python3.withPackages bit entirely and just provide an example - something like "...deprecated in favor of blender.withPackages, e.g. blender.withPackages(ps: [ ps.honk ])"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Definitely more readable, yeah. Also, I'd probably drop the python3.withPackages bit entirely and just provide an example - something like "...deprecated in favor of blender.withPackages, e.g. blender.withPackages(ps: [ ps.honk ])"

I just feel iffy about referring to a specific package for a general example

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't really see a problem with that personally. If you really wanted to be fancy about it, you could generate the expression from the passed-in package names, I guess (but I feel like this would be trying way too hard lmao)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, I know what to do!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nixpkgs/pkgs/top-level/aliases.nix

Line 96 in fe4c776

lib.warn "blender-with-packages is deprecated in favor of blender.withPackages, e.g. `blender.withPackages(ps: [ ps.foobar ])`"

How's that?

@the-furry-hubofeverything
blender: allow functional declaration within withPackages
fe4c776 
Based on NixOS#257780, separated since it introduces significant changes.

bpycv: update passthru.tests.render

blender-with-packages: deprecated
it is still backwards compatible, but no longer preferred.
@delroth delroth added the 12.approvals: 2 This PR was reviewed and approved by two reputable people label Nov 3, 2023
@Mindavi Mindavi merged commit 3eccaf8 into NixOS:master Nov 3, 2023
@the-furry-hubofeverything the-furry-hubofeverything deleted the blender-withPackages-test branch November 3, 2023 15:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@K900 K900 K900 approved these changes

@lucasew lucasew Awaiting requested review from lucasew

@SomeoneSerge SomeoneSerge Awaiting requested review from SomeoneSerge

@veprbl veprbl Awaiting requested review from veprbl

@cillianderoiste cillianderoiste Awaiting requested review from cillianderoiste

@pbsds pbsds Awaiting requested review from pbsds

@Mindavi Mindavi Awaiting requested review from Mindavi

Assignees
No one assigned
Labels
6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 6.topic: python 8.has: changelog 8.has: documentation This PR adds or changes documentation 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 12.approvals: 2 This PR was reviewed and approved by two reputable people
Projects
None yet
Milestone
23.11
Development

Successfully merging this pull request may close these issues.
6 participants
@the-furry-hubofeverything @lucasew @pbsds @K900 @Mindavi @delroth