Skip to content

libvisual: remove SDL dependency and refactor#384912

Closed
marcin-serwin wants to merge 3 commits intoNixOS:stagingfrom
marcin-serwin:push-ysxukwtkksss
Closed

libvisual: remove SDL dependency and refactor#384912
marcin-serwin wants to merge 3 commits intoNixOS:stagingfrom
marcin-serwin:push-ysxukwtkksss

Conversation

@marcin-serwin
Copy link
Contributor

@marcin-serwin marcin-serwin commented Feb 25, 2025

This PR refactors the libvisual package to make it slimmer and fix the cross compilation issue. Some refactoring was also applied.

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.

@github-actions github-actions bot added 10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-linux: 2501-5000 This PR causes many rebuilds on Linux and should target the staging branches. 10.rebuild-linux: 501+ This PR causes many rebuilds on Linux and should normally target the staging branches. labels Feb 25, 2025
jopejoe1
jopejoe1 reviewed Feb 25, 2025
View reviewed changes
@github-actions github-actions bot added 6.topic: python Python is a high-level, general-purpose programming language. 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` 6.topic: vscode A free and versatile code editor that supports almost every major programming language. labels Mar 6, 2025
@github-actions github-actions bot removed 6.topic: python Python is a high-level, general-purpose programming language. 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` 6.topic: vscode A free and versatile code editor that supports almost every major programming language. labels Mar 6, 2025
@marcin-serwin marcin-serwin requested a review from jopejoe1 March 6, 2025 23:46
@github-actions github-actions bot added 10.rebuild-darwin: 101-500 This PR causes between 101 and 500 packages to rebuild on Darwin. 10.rebuild-linux: 5001+ This PR causes many rebuilds on Linux and must target the staging branches. 10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-linux: 2501-5000 This PR causes many rebuilds on Linux and should target the staging branches. and removed 10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-linux: 2501-5000 This PR causes many rebuilds on Linux and should target the staging branches. 10.rebuild-darwin: 101-500 This PR causes between 101 and 500 packages to rebuild on Darwin. 10.rebuild-linux: 5001+ This PR causes many rebuilds on Linux and must target the staging branches. labels Mar 6, 2025
SuperSandro2000
SuperSandro2000 reviewed Mar 8, 2025
View reviewed changes
pkgs/by-name/li/libvisual/package.nix Outdated
Copy link
Member

@SuperSandro2000 SuperSandro2000 Mar 8, 2025

Choose a reason for hiding this comment

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

We just added the # Remove when 0.5.x is published. in the last commit. We might as well don't add it to have a cleaner history.

Copy link
Contributor Author

@marcin-serwin marcin-serwin Mar 9, 2025

Choose a reason for hiding this comment

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

This comment is moved, not added. Previously it was near the configureFlags.

K900
K900 reviewed Mar 8, 2025
View reviewed changes
pkgs/by-name/li/libvisual/package.nix Outdated
Copy link
Contributor

@K900 K900 Mar 8, 2025

Choose a reason for hiding this comment

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

Do we even need the full variant? gstreamer is the only consumer of this thing.

Copy link
Contributor Author

@marcin-serwin marcin-serwin Mar 9, 2025

Choose a reason for hiding this comment

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

Most likely not, I was overly cautious.

pkgs/by-name/li/libvisual/package.nix Outdated
Copy link
Contributor

@K900 K900 Mar 8, 2025

Choose a reason for hiding this comment

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

Do we need pkgconf specifically?

Copy link
Member

@SuperSandro2000 SuperSandro2000 Mar 8, 2025

Choose a reason for hiding this comment

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

seems like it, see f326ee0

Copy link
Contributor

@LordGrimmauld LordGrimmauld Mar 8, 2025

Choose a reason for hiding this comment

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

I am currently working on a treewide replace of SDL -> SDL_compat (which, because of cyclic dependencies, relies on this PR being merged).

I noticed the issue with pkg-config too and am currently trying a workaround in SDL_compat.

Copy link
Contributor

@LordGrimmauld LordGrimmauld Mar 8, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I just opened #388304 trying to get the pkg-config+SDL_compat issues dealt with, currently waiting for my system rebuild with that change as well as a dependency count of the bot before running it through review. But seeing as this seems related, figured i'd put the heads-up here.

Copy link
Contributor

@K900 K900 Mar 9, 2025

Choose a reason for hiding this comment

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

Yeah I definitely prefer that solution because otherwise we'd have to do this on EVERY PACKAGE.

Copy link
Contributor Author

@marcin-serwin marcin-serwin Mar 9, 2025

Choose a reason for hiding this comment

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

I am currently working on a treewide replace of SDL -> SDL_compat

@LordGrimmauld I was also working on this. To avoid duplicated work: #387419 and #387357 are also needed in case you haven't seen them.

@LordGrimmauld LordGrimmauld mentioned this pull request Mar 9, 2025
Merged
13 tasks
K900
K900 reviewed Mar 9, 2025
View reviewed changes
pkgs/by-name/li/libvisual/package.nix Outdated
Copy link
Contributor

@K900 K900 Mar 9, 2025

Choose a reason for hiding this comment

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

Honestly do we even need lvtool? Can we just like, not build it? Or maybe even yeet this entire package? Probably not the best place to have this conversation, but it feels like a lot of complexity for not a lot of gain.

Copy link
Contributor Author

@marcin-serwin marcin-serwin Mar 9, 2025

Choose a reason for hiding this comment

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

Honestly do we even need lvtool?

Probably not

Or maybe even yeet this entire package?

I had a similar thought: #388447

@marcin-serwin marcin-serwin mentioned this pull request Mar 9, 2025
Merged
13 tasks
@marcin-serwin
libvisual: don't build lv-tool and examples
253451b 
The full package provides lv-tool and examples which are currently
not used by anything in nixpkgs: the tool does not work without
libvisual-plugins which are not packaged, and examples themselves
are not even installed. Avoiding building these allows us to drop the
SDL dependency.
@marcin-serwin
libvisual: update homepage
4f523a6
@marcin-serwin
libvisual: add pkg-config validation
b5b947f
@marcin-serwin marcin-serwin deleted the push-ysxukwtkksss branch March 10, 2025 22:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@K900 K900 K900 left review comments

@SuperSandro2000 SuperSandro2000 SuperSandro2000 left review comments

@LordGrimmauld LordGrimmauld LordGrimmauld left review comments

@jopejoe1 jopejoe1 Awaiting requested review from jopejoe1

Assignees

No one assigned

Labels

10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-linux: 501+ This PR causes many rebuilds on Linux and should normally target the staging branches. 10.rebuild-linux: 2501-5000 This PR causes many rebuilds on Linux and should target the staging branches.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@marcin-serwin @K900 @SuperSandro2000 @jopejoe1 @LordGrimmauld