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

openlp: init at 2.4.6 #91291

Merged
merged 1 commit into from Apr 6, 2021
Merged

openlp: init at 2.4.6 #91291

merged 1 commit into from Apr 6, 2021

Conversation

jorsn
Copy link
Member

@jorsn jorsn commented Jun 22, 2020 •
edited

free church presentation software

The package is split in two parts: The main python package to build (lib.nix) and a wrapper package pulling in configurable runtime dependencies (default.nix), so that rebuilds are unnecessary when changing runtime deps/features.

Review notes
  1. The critical parts to test are: Video playback (manual) via the 3 backends (system, webkit, vlc) and presentation support (manual) via mupdf and libreoffice (uno).
  2. All these tests require the package openlpFull. In openlp they aren't used, as they are optional.
  3. When switching the video backend, it is best to restart OpenLP afterwards to make sure only the right one is initialized. To switch, select only one backend in Settings → Configure OpenLP → Players.
Things done

(na) means “not applicable”

  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • (na) Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • (na) Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
    package openlp: 676.1 MB, package openlpFull: 3.0 GB.
  • (na) Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

Additionally:

  • Tested in a pure nix-shell. There, audio does not work, as pulseaudio cannot be connected, but this is also the case using plain vlc.

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/qt-plugin-path-unset-in-test-phase/6962/2

jorsn added a commit to jorsn/nur-packages that referenced this pull request Jun 23, 2020
@jorsn
remove openlp overlay (old; adding new version to nixpkgs)
15236c4 
see NixOS/nixpkgs#91291
@jorsn

This comment has been minimized.

Sign in to view
@jorsn jorsn mentioned this pull request Sep 18, 2020
Closed
jonringer
jonringer reviewed Sep 18, 2020
View reviewed changes
pkgs/applications/misc/openlp/BUGS Outdated Show resolved Hide resolved
pkgs/development/interpreters/python/default.nix Outdated Show resolved Hide resolved
pkgs/top-level/all-packages.nix Outdated Show resolved Hide resolved
pkgs/top-level/all-packages.nix Outdated Show resolved Hide resolved
@jorsn jorsn marked this pull request as draft September 18, 2020 21:20
@jorsn jorsn marked this pull request as ready for review September 22, 2020 20:50
@ofborg ofborg bot removed the 6.topic: python label Sep 22, 2020
@jorsn jorsn force-pushed the new.openlp branch 2 times, most recently from 8767bdf to ab54809 Compare September 23, 2020 10:23
@jorsn jorsn force-pushed the new.openlp branch 2 times, most recently from 0e15884 to 2fd709a Compare September 23, 2020 12:36
rnhmjoj
rnhmjoj approved these changes Sep 27, 2020
View reviewed changes
Copy link
Contributor

@rnhmjoj rnhmjoj left a comment •
edited

Choose a reason for hiding this comment

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

I don't see any other issue with the expression. I built the packages with nixpkgs-review and tested the application: to check the full functionality someone familiar with it should try it, but the program seem packaged and wrapped correctly.
To me it's all good for inclusion in Nixpkgs.

SuperSandro2000
SuperSandro2000 reviewed Feb 9, 2021
View reviewed changes
pkgs/top-level/all-packages.nix Show resolved Hide resolved
pkgs/top-level/all-packages.nix Outdated Show resolved Hide resolved
pkgs/applications/misc/openlp/lib.nix Outdated Show resolved Hide resolved
pkgs/applications/misc/openlp/lib.nix Outdated Show resolved Hide resolved
pkgs/applications/misc/openlp/lib.nix Outdated Show resolved Hide resolved
pkgs/applications/misc/openlp/lib.nix Show resolved Hide resolved
pkgs/applications/misc/openlp/default.nix Outdated Show resolved Hide resolved
pkgs/applications/misc/openlp/default.nix Outdated Show resolved Hide resolved
pkgs/applications/misc/openlp/default.nix Outdated Show resolved Hide resolved
pkgs/applications/misc/openlp/default.nix Outdated Show resolved Hide resolved
FRidh
FRidh reviewed Feb 9, 2021
View reviewed changes
pkgs/top-level/all-packages.nix Outdated
@@ -27479,6 +27479,14 @@ in

octopus = callPackage ../applications/science/chemistry/octopus { };

openlp = libsForQt5.callPackage ../applications/misc/openlp { };
openlpFull = openlp.override {
Copy link
Member

Choose a reason for hiding this comment

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

different name attribute is needed as well for nix-env

Copy link
Member Author

Choose a reason for hiding this comment

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

I fixed it.

Copy link
Member Author

@jorsn jorsn Feb 9, 2021 •
edited

Choose a reason for hiding this comment

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

different name attribute is needed as well for nix-env

...which applies maybe also for a couple of other packages:

[ "connmanFull" "curlFull" "mjpegtoolsFull" "octaveFull" "pythonFull" "python2Full" "python27Full" "python3Full" "python36Full" "python37Full" "python38Full" "python39Full" "krb5Full" "pulseaudioFull" "samba4Full" "sambaFull" "gitFull" "polybarFull" "wineFull" ]

says

{ pkgs ? import ./. {} }:
let
  attrNames = 
    [ "connman" "curl" "mjpegtools" "octave"
      "python" "python2" "python27" "python3" "python36" "python37" "python38" "python39"
      "harfbuzz" "krb5" "pulseaudio" "samba4" "samba" "codeblocks" "git" "polybar" "wine"
      "mercurial" # mercurial is the negative test case: its plain and Full names are distinct
    ];
  filtered = builtins.filter (n: pkgs."${n}".name == pkgs."${n}Full".name) attrNames;
in map (n: "${n}Full") filtered

For which of those is it unnecessary?

Copy link
Member Author

@jorsn jorsn Feb 9, 2021 •
edited

Choose a reason for hiding this comment

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

(I got the potential packages by grep Full pkgs/top-level/*.nix)

@SuperSandro2000

This comment has been minimized.

Sign in to view
@jorsn
Copy link
Member Author

jorsn commented Feb 23, 2021

In the last push (63ff41b2be8a6f6aa432e4e0bde257ea8ce38890) I added meta.hydraPlatforms = [ ] to the wrapper, because only the baseLib derivation needs to be built by Hydra.

@jorsn
Copy link
Member Author

jorsn commented Mar 1, 2021

Anything preventing this from being merged?

@SuperSandro2000
Copy link
Member

fails to build:

  Using setuptoolsBuildPhase
  Using setuptoolsShellHook
  Sourcing pip-install-hook
  Using pipInstallPhase
  Sourcing python-imports-check-hook.sh
  Using pythonImportsCheckPhase
  Sourcing python-namespaces-hook
  qtPreHook
  Error: wrapQtAppsHook is not used, and dontWrapQtApps is not set.
  builder for '/nix/store/cj3pjcndim4sp3b72f0i7jdqwdfy8ms8-python3.8-openlp-2.4.6.drv' failed with exit code

@SuperSandro2000

This comment has been minimized.

Sign in to view
@jorsn
openlp: init at 2.4.6
80f0061 
free church presentation software
@jorsn

This comment has been minimized.

Sign in to view
@jorsn
Copy link
Member Author

jorsn commented Mar 19, 2021

Still building and running:

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

2 packages built:
  • openlp
  • openlpFull

@SuperSandro2000
Copy link
Member

@ofborg eval

@SuperSandro2000
Copy link
Member

This is a semi-automatic executed nixpkgs-review with nixpkgs-review-checks extension. It is checked by a human on a best effort basis and does not build all packages (e.g. lumo, tensorflow or pytorch).
If you have any questions or problems please reach out to SuperSandro2000 on IRC.

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

2 packages built:
  • openlp
  • openlpFull

@SuperSandro2000 SuperSandro2000 merged commit cc44411 into NixOS:master Apr 6, 2021
jorsn added a commit to jorsn/nur-packages that referenced this pull request May 1, 2023
@jorsn
remove openlp overlay (old; adding new version to nixpkgs)
fcc0631 
see NixOS/nixpkgs#91291
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@FRidh FRidh FRidh left review comments

@SuperSandro2000 SuperSandro2000 SuperSandro2000 left review comments

@jonringer jonringer jonringer left review comments

@rnhmjoj rnhmjoj rnhmjoj approved these changes

@jagajaga jagajaga Awaiting requested review from jagajaga

@ebzzry ebzzry Awaiting requested review from ebzzry

@bjornfor bjornfor Awaiting requested review from bjornfor

Assignees
No one assigned
Labels
8.has: package (new) 10.rebuild-darwin: 0 10.rebuild-linux: 0 11.by: package-maintainer
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@jorsn @nixos-discourse @SuperSandro2000 @FRidh @rnhmjoj @jonringer @bgermann