-
-
Notifications
You must be signed in to change notification settings - Fork 14.4k
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
ffado: Remove Qt dependency #306407
ffado: Remove Qt dependency #306407
Conversation
The new nixfmt is making reviewing this commit by commit actually hard... |
Hiding whitespace changes (or using difftastic) will help. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The approach looks reasonable to me and the mixer is probably not required to be there for pipewire but I can't properly review the changes on mobile with the formatting changes all in between...
Hiding whitespace changes (or using difftastic) will help.
That's not possible in the mobile app 🙃
|
||
prefixKey = "PREFIX="; | ||
sconsFlags = [ | ||
"DEBUG=False" | ||
"ENABLE_ALL=True" | ||
"BUILD_TESTS=True" | ||
"WILL_DEAL_WITH_XDG_MYSELF=True" | ||
"BUILD_MIXER=True" | ||
"BUILD_MIXER=${if withMixer then "True" else "False"}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideally, we would only build the mixer in the mixer
package but it is not easy to prevent libffado from building.
And the mixer will try to launch ffado-dbus-server
so we would need to be able to specify path to it from the libffado prefix.
i've confirmed on my end that pipewire works fine without the mixer using a similar, but not identical patch. the changes here look reasonable, and once this lands i'll rebase what i've got there to make ffado cross compile. then with Mindavi's appstream cross PR i think we'll have the whole suite of basic nitpick: since it includes all of the base |
Will probably wait for 2.4.9, which will include all the patches used. |
Any chances we merge it? Curiously, we have |
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: |
Doesn't look good for ffado's upstream development. Their subversion is down: http://subversion.ffado.org/ |
I cherry-picked the first two commits into its own pull request: #318999 |
@K900 you added ffado support to pipewire in the first place to nixpkgs presumably because of https://gitlab.freedesktop.org/pipewire/pipewire/-/commit/3c44634dd80975f13def3c09a1f05aaf9ad08110 |
There are definitely users, but Pipewire only requires the library, so moving the mixer to another package is good IMO. |
Ok. Can you than point me to one that would also test changes on real hardware? Otherwise I cannot be sure that I am not breaking anything. |
Not off the top of my head, sorry :( I think if Pipewire builds, it should be fine. |
I reported it to upstream a while ago and I vaguely remember upstream acknowledging that but cannot find the reply. New patches are still being accepted over the mailing list just fine.
I do not think there is any hurry, the metainfo is not really used by anything so waiting for 2.4.9 should be fine. The only critical thing is https://ffado.org/posts/ffado-2.4.8-tarball_fix/ 🙀 I still need to write an e-mail telling upstream off. Though, I just noticed the hash update was reverted so it is probably still broken: #317396 |
] | ||
++ lib.optionals withMixer [ | ||
python | ||
python3.pkgs.pyqt5 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it actually necessary here and not just in withPackages?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like I added it in 8b1816e. I believe this is for pyuic5
program but not sure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM other than one small nit
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Didn't test but lgtm
This slims up `pipewire` closure. The full package is available as `ffado-mixer`.
]; | ||
nativeBuildInputs = | ||
[ | ||
(scons.override { python3Packages = python311.pkgs; }) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we still need the scons pin?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, the SConstruct
still pulls in import distutils.sysconfig
, which was removed in Python 3.12.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool. Can you add a comment for that so whoever updates this next knows to remove it?
This was introduced in 20001b7
it was removed from the package in the [2.4.8 -> 2.4.9 upgrade](NixOS#306407) but never removed from git.
Description of changes
This slims up
pipewire
build closure.The full package is available as
ffado-mixer
.Things done
nix.conf
? (See Nix manual)sandbox = relaxed
sandbox = true
nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD"
. Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/
)Add a 👍 reaction to pull requests you find important.