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

Fix Plasma NixOS tests #73876

Merged
merged 4 commits into from Nov 22, 2019
Merged

Fix Plasma NixOS tests #73876

merged 4 commits into from Nov 22, 2019

Conversation

@jtojnar
Copy link
Contributor

@jtojnar jtojnar commented Nov 21, 2019

Required to build with Phonon 4.11 (#71745). Not having this blocks the channels.

Phonon no longer supports Qt 4 so I marked it as broken there.

cc @ttuegel

@jtojnar jtojnar force-pushed the jtojnar:phonon-backends branch from d07e991 to 1c04e1a Nov 21, 2019
@ofborg ofborg bot requested a review from ttuegel Nov 21, 2019
@jtojnar jtojnar force-pushed the jtojnar:phonon-backends branch from 1c04e1a to 8e20453 Nov 21, 2019
@jtojnar jtojnar mentioned this pull request Nov 21, 2019
55 of 223 tasks complete
@jtojnar jtojnar requested a review from Ma27 Nov 21, 2019
@jtojnar jtojnar force-pushed the jtojnar:phonon-backends branch from c8d62de to 7e1fd81 Nov 21, 2019
@jtojnar
Copy link
Contributor Author

@jtojnar jtojnar commented Nov 21, 2019

Had to do more drastic changes than just update since Qt 4 is no longer supported by Phonon since #71745

@jtojnar jtojnar changed the title libsForQt5.phonon-backend-gstreamer: 4.9.0 → 4.10.0 Fix Plasma NixOS tests Nov 22, 2019
@FRidh
Copy link
Member

@FRidh FRidh commented Nov 22, 2019

This needs rebasing. Note many parts of KDE are failing since the staging-next merge. #73756

Phonon no longer supports Qt4 so this is useless.
@jtojnar jtojnar force-pushed the jtojnar:phonon-backends branch from 7e1fd81 to c9ab490 Nov 22, 2019
@jtojnar
Copy link
Contributor Author

@jtojnar jtojnar commented Nov 22, 2019

Rebased.

@worldofpeace
Copy link
Member

@worldofpeace worldofpeace commented Nov 22, 2019

@jtojnar Can you change the commit message phonon: mark as broken to phonon: mark as broken for qt4? At a glance I thought phonon was completely broken.

@jtojnar
Copy link
Contributor Author

@jtojnar jtojnar commented Nov 22, 2019

Qt5 libraries use libsForQt5 prefix. Anyway, I will be away from computer for the rest of the day so feel free to reword it yourself.

Copy link
Member

@worldofpeace worldofpeace left a comment

https://phabricator.kde.org/D22688 says qt4 was removed there entirely.
Can we remove all withQt4 options, and the ? null for the function arguments?

@worldofpeace
Copy link
Member

@worldofpeace worldofpeace commented Nov 22, 2019

Qt5 libraries use libsForQt5 prefix. Anyway, I will be away from computer for the rest of the day so feel free to reword it yourself.

I see. Will push my changes then.

@worldofpeace worldofpeace force-pushed the jtojnar:phonon-backends branch from c9ab490 to 9b236c2 Nov 22, 2019
@worldofpeace
Copy link
Member

@worldofpeace worldofpeace commented Nov 22, 2019

Here were my changes

There was already a todo to remove it completely, so I added aliases and removed the qt4 versions and options. @jtojnar I think you kept the arguments to fail gracefully, it kinda sucks we don't have a nice thing like aliases like this. What do you think about keeping withQt4 but instead asserting it to be false, with an assertMsg like Qt4 support has been removed. Please remove withQt4?

@ofborg ofborg bot added the 8.has: clean-up label Nov 22, 2019
@jtojnar
Copy link
Contributor Author

@jtojnar jtojnar commented Nov 22, 2019

@worldofpeace
Copy link
Member

@worldofpeace worldofpeace commented Nov 22, 2019

What we should not do is point the aliases to the Qt5 versions – they
are fundamentally different packages. I think changing the aliases to
throw should work here.

I actually only considered it because it wasn't the only entry in aliases.nix that does this.
Though also, considering there's going to be a massive removal of attributes for qt4, perhaps it's like a forced upgrade to qt5? In the end, that's what a user of the alias would need this for.

But I do get the point of an alias not pointing to a fundamentally changed package, though it is still phonon.

phonon = throw "Please use libsForQt5.phonon, as Qt4 support in this package has been removed.";
jtojnar and others added 3 commits Nov 21, 2019
Required to build with Phonon 4.11 (#71745). Not having this blocks the channels.

Requires qttools for Qt5LinguistTools.

Qt4 support removed since Phonon no longer supports it either.

Co-authored-by: worldofpeace <worldofpeace@protonmail.ch>
Required to build with Phonon 4.11 (#71745).

Requires qttools for Qt5LinguistTools.

Qt4 support removed since Phonon no longer supports it either.

Co-authored-by: worldofpeace <worldofpeace@protonmail.ch>
Qt4 is no longer supported.

https://phabricator.kde.org/D22688

Co-authored-by: worldofpeace <worldofpeace@protonmail.ch>
@jtojnar jtojnar force-pushed the jtojnar:phonon-backends branch from 9b236c2 to 62a09e7 Nov 22, 2019
@jtojnar
Copy link
Contributor Author

@jtojnar jtojnar commented Nov 22, 2019

Pushed, thanks.

Copy link
Member

@worldofpeace worldofpeace left a comment

LGTM, going to merge right away because our channel is blocked.

@worldofpeace worldofpeace merged commit 9995881 into NixOS:master Nov 22, 2019
16 checks passed
16 checks passed
libsForQt5.phonon-backend-gstreamer, libsForQt5.phonon-backend-vlc on aarch64-linux Failure
Details
libsForQt5.phonon-backend-gstreamer, libsForQt5.phonon-backend-vlc on x86_64-darwin No attempt
Details
Evaluation Performance Report Evaluator Performance Report
Details
grahamcofborg-eval ^.^!
Details
grahamcofborg-eval-check-maintainers matching changed paths to changed attrs...
Details
grahamcofborg-eval-check-meta config.nix: checkMeta = true
Details
grahamcofborg-eval-darwin nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A darwin-tested
Details
grahamcofborg-eval-nixos nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release-combined.nix -A tested
Details
grahamcofborg-eval-nixos-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A manual
Details
grahamcofborg-eval-nixos-options nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A options
Details
grahamcofborg-eval-nixpkgs-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A manual
Details
grahamcofborg-eval-nixpkgs-tarball nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A tarball
Details
grahamcofborg-eval-nixpkgs-unstable-jobset nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A unstable
Details
grahamcofborg-eval-package-list nix-env -qa --json --file .
Details
grahamcofborg-eval-package-list-no-aliases nix-env -qa --json --file . --arg config { allowAliases = false; }
Details
libsForQt5.phonon-backend-gstreamer, libsForQt5.phonon-backend-vlc on x86_64-linux Success
Details
@worldofpeace worldofpeace deleted the jtojnar:phonon-backends branch Nov 22, 2019
@matthewbauer

This comment has been minimized.

This throws a warning in my config with services.xserver.desktopManager.plasma5.enableQt4Support = false. Is there a way we can make this only an error when it was set to true?

This comment has been minimized.

Copy link
Contributor Author

@jtojnar jtojnar replied Jan 2, 2020

We would need to make mkRemovedOptionModule take a default value and ignore the error when set to the default value but I am not sure it is worth the effort. Also I would argue that having it warn you is a good thing to push you to clean up your config before the eventual hard removal.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.