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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

livi: init at 0.0.6 #295711

Merged
merged 2 commits into from Apr 14, 2024
Merged

livi: init at 0.0.6 #295711

merged 2 commits into from Apr 14, 2024

Conversation

mksafavi
Copy link
Contributor

Description of changes

source URL: https://gitlab.gnome.org/guidog/livi

closes #295373

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

pkgs/by-name/li/livi/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/li/livi/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/li/livi/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/li/livi/package.nix Show resolved Hide resolved
pkgs/by-name/li/livi/package.nix Show resolved Hide resolved
pkgs/by-name/li/livi/package.nix Show resolved Hide resolved
pkgs/by-name/li/livi/package.nix Show resolved Hide resolved
@mksafavi
Copy link
Contributor Author

mksafavi commented Apr 12, 2024

Thanks for reviewing this.

should be in nativeBuildInputs

I think I'm confused about this. I read the nix documentation about nativeBuildInputs and buildInputs. If I understand it correctly nativeBuildInputs are build time dependencies that are arch dependent. They make it possible to cross compile for other systems too. buildInputs are the runtime dependencies. right?

I have an issue with this packages. It requires gstreamer on runtime. I assumed adding gst_all_1 packages to buildInputs would fix that.
But It seems like that's not the case. If I create a shell with livi, it can't find gst elements.
Do I need to do anything specific to include gst in the derivation?

Not adding yourself to maintainers?

I thought it should be better if a user of the app becomes the maintainer. That being said, I'm ok with maintaining it myself too.

@mksafavi mksafavi requested a review from Aleksanaa April 12, 2024 13:01
@Aleksanaa
Copy link
Member

I have an issue with this packages. It requires gstreamer on runtime. I assumed adding gst_all_1 packages to buildInputs would fix that. But It seems like that's not the case. If I create a shell with livi, it can't find gst elements. Do I need to do anything specific to include gst in the derivation?

Oh you forgot to add wrapGAppsHook4 to nativeBuildInputs. Gstreamer packages add $GST_PLUGIN_SYSTEM_PATH_1_0 but don't create a wrapper themselves. wrapGAppsHook4 can handle it for you. If you use other wrappers, you have to manually append it.

I thought it should be better if a user of the app becomes the maintainer. That being said, I'm ok with maintaining it myself too.

That's not the case. Just others will ping you for issues and build failures, and you can deal with automatic updates yourself.

@mksafavi
Copy link
Contributor Author

Oh you forgot to add wrapGAppsHook4 to nativeBuildInputs. Gstreamer packages add $GST_PLUGIN_SYSTEM_PATH_1_0 but don't create a wrapper themselves. wrapGAppsHook4 can handle it for you. If you use other wrappers, you have to manually append it.

Thanks. adding wrapGAppsHook4 fixed the issue.

That's not the case. Just others will ping you for issues and build failures, and you can deal with automatic updates yourself.

I added myself to the maintainers.

Also, I squashed the commits and fixed the messages. I think it should fit the CONTRIBUTING.md now.

gst_all_1.gstreamer
gst_all_1.gst-plugins-base
gst_all_1.gst-plugins-bad
gst_all_1.gst-plugins-good
Copy link
Member

Choose a reason for hiding this comment

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

These gst packages should be in buildInputs

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh. I miss interpreted the previous review and instead of moving the last line, I moved the whole highlighted section into nativeBuildInput. I'll fix this.
How do you know which packages should get into buildInputs?

Copy link
Member

@Aleksanaa Aleksanaa Apr 14, 2024

Choose a reason for hiding this comment

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

I should admit that I mostly go by feel. Packages that are used at build time should go in nativeBuildInputs, and packages that are linked or called at runtime should go in buildInputs. If you want a very accurate representation, you can add strictDeps = true to this package (recommended), in this way if you don't do it correctly, you'll fail to build.

gst_all_1.gst-plugins-good
appstream-glib
gtk4
libadwaita
Copy link
Member

Choose a reason for hiding this comment

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

gtk4 and libadwaita should also be in buildInputs

meta = with lib; {
homepage = "https://gitlab.gnome.org/guidog/livi";
description = "A small video player targeting mobile devices (also named 渭Player)";
license = licenses.gpl3;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
license = licenses.gpl3;
license = licenses.gpl3Plus;

Copy link
Contributor Author

@mksafavi mksafavi Apr 14, 2024

Choose a reason for hiding this comment

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

I think it should be changed into gpl3Only based on this https://gitlab.gnome.org/guidog/livi/-/blob/main/COPYING
It seems like gpl3 is deprecated but I didn't get any warnings in the build.
Do you know why my builds don't care about the meta section? It was the same with the maintainer field as well. I didn't notice any issues with it but Borg caught it and failed.

Copy link
Member

Choose a reason for hiding this comment

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

I think it should be changed into gpl3Only based on this https://gitlab.gnome.org/guidog/livi/-/blob/main/COPYING

It's at the top of c source files. Also the flathub uses gpl3Plus.

It seems like gpl3 is deprecated but I didn't get any warnings in the build.
Do you know why my builds don't care about the meta section? It was the same with the maintainer field as well. I didn't notice any issues with it but Borg caught it and failed.

Because It does not use these sections.

Copy link
Member

@Aleksanaa Aleksanaa left a comment

Choose a reason for hiding this comment

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

lgtm. Please squash your commits into livi: init at 0.0.6 (except adding maintainer)

@@ -0,0 +1,55 @@
{ lib
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should I order the args alphabetically here?
I noticed that some packages put builtins like lib, stdenv,... first and then order the deps alphabetically.

Copy link
Member

Choose a reason for hiding this comment

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

Not needed. You can also sort them in the order they are used. It's up to you. But lib should be placed in first order.

@Aleksanaa Aleksanaa merged commit 7e60fe6 into NixOS:master Apr 14, 2024
24 of 25 checks passed
@mksafavi mksafavi deleted the pkg/livi branch May 4, 2024 16:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Package request: livi
3 participants