Skip to content

cosmic-player: init at unstable-2025-01-14#373897

Merged
RossComputerGuy merged 1 commit intoNixOS:masterfrom
ahoneybun:init-cosmic-player
Jan 20, 2025
Merged

cosmic-player: init at unstable-2025-01-14#373897
RossComputerGuy merged 1 commit intoNixOS:masterfrom
ahoneybun:init-cosmic-player

Conversation

@ahoneybun
Copy link
Copy Markdown
Contributor

@ahoneybun ahoneybun commented Jan 15, 2025

Things done

This is based on @lilyinstarlight work in the nixos-cosmic repo linked below.

https://github.com/lilyinstarlight/nixos-cosmic/blob/main/pkgs/cosmic-player/package.nix

This is based on the latest tag for COSMIC Player which is epoch-1.0.0-alpha.5.1

  • 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: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. labels Jan 15, 2025
@ahoneybun ahoneybun requested a review from a-kenji January 15, 2025 13:31
Copy link
Copy Markdown
Member

@a-kenji a-kenji left a comment

Choose a reason for hiding this comment

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

Thank you for packaging this package.

This package is not upstreamable in its current form.

Please look at the cosmic-edit package that you maintain: https://github.com/NixOS/nixpkgs/blob/master/pkgs/by-name/co/cosmic-edit/package.nix#L32.

It uses the useFetchCargoVendor = true; functionality, which makes the Cargo.lock file obsolete.

I haven't looked at the source you got it from, but those packages don't seem upstreamable.

@ahoneybun
Copy link
Copy Markdown
Contributor Author

Thanks for the review @a-kenji ! I was mainly worried about the gst plugins (legal can be a funny thing) and was worried that I included something that I not have haha.

@ahoneybun ahoneybun requested a review from a-kenji January 15, 2025 14:08
@ahoneybun
Copy link
Copy Markdown
Contributor Author

ahoneybun commented Jan 15, 2025

@a-kenji I think that's what you wanted correct?

@a-kenji
Copy link
Copy Markdown
Member

a-kenji commented Jan 15, 2025

nixpkgs-review result

Generated using nixpkgs-review.

Command: nixpkgs-review pr 373897


x86_64-linux

✅ 3 packages built:
  • cosmic-player
  • parlay
  • terraform-providers.keycloak

I get the following error when trying to run:

thread 'main' panicked at /build/cosmic-player-1.0.0-alpha.5.1-vendor/iced_winit-0.12.0/src/application.rs:211:10:
Create event loop: Os(OsError { line: 81, file: "/build/cosmic-player-1.0.0-alpha.5.1-vendor/winit-0.29.10/src/platform_impl/linux/wayland/event_loop/mod.rs", error: WaylandError(Connection(NoWaylandLib)) })
stack backtrace:
   0: rust_begin_unwind
   1: core::panicking::panic_fmt
   2: core::result::unwrap_failed
   3: iced_winit::application::run
   4: cosmic_player::main
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.

@a-kenji
Copy link
Copy Markdown
Member

a-kenji commented Jan 15, 2025

Yes, that is what I wanted. It builds fine. But I can't get it to run.

@ahoneybun
Copy link
Copy Markdown
Contributor Author

Yea I noticed that as well and I'll work on it later today. I still want to get Lily's blessing to use her work and make her I credit her before merging this though.

@lilyinstarlight
Copy link
Copy Markdown
Member

lilyinstarlight commented Jan 15, 2025

Yes, that is what I wanted. It builds fine. But I can't get it to run.

The force-linking libwayland-client that upstream nixos-cosmic does for all libcosmic-based apps is load-bearing and would be needed here too for it to run (it appears to have been excluded when copy-pasting)

I still want to get Lily's blessing to use her work and make her I credit her before merging this though.

I unfortunately will not be blessing this work, but I also won't stop you from following license terms if you copy-paste substantial portions, and I don't otherwise mind the work! (most relevantly source attribution is required for substantially copy-pasted portions -- a courtesy link in code comment to nixos-cosmic so people know where to update the derivation from when it needs updating would be helpful too, but is not strictly required)

@ahoneybun
Copy link
Copy Markdown
Contributor Author

@lilyinstarlight I'd be happy to include that link (once I have it as I'm not sure where that would be) in a code comment and anything else to give the credit to you.

@ahoneybun
Copy link
Copy Markdown
Contributor Author

@a-kenji I suspect that should work now.

@a-kenji
Copy link
Copy Markdown
Member

a-kenji commented Jan 15, 2025

Yes, it does work for me now.

Copy link
Copy Markdown
Member

@RossComputerGuy RossComputerGuy left a comment

Choose a reason for hiding this comment

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

Little nitpicks, also should squash the commits to 1.

Comment on lines 102 to 109
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

No with lib please.

Copy link
Copy Markdown
Contributor Author

@ahoneybun ahoneybun Jan 16, 2025

Choose a reason for hiding this comment

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

So like this?:

  meta = {
    maintainers = with lib.maintainers; [ ahoneybun ];
    license = licenses.gpl3Only;
  };

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Otherwise it errors out:

aaronh@pop-os:~/Projects/NixOS/nixpkgs$ nix-build -A cosmic-player --max-jobs 1
error:
       … while evaluating a branch condition
         at /home/aaronh/Projects/NixOS/nixpkgs/lib/customisation.nix:303:5:
          302|     in
          303|     if missingArgs == { } then
             |     ^
          304|       makeOverridable f allArgs

       … while calling the 'removeAttrs' builtin
         at /home/aaronh/Projects/NixOS/nixpkgs/lib/attrsets.nix:647:5:
          646|     set:
          647|     removeAttrs set (filter (name: ! pred name set.${name}) (attrNames set));
             |     ^
          648|

       (stack trace truncated; use '--show-trace' to show the full, detailed trace)

       error: syntax error, unexpected '['
       at /home/aaronh/Projects/NixOS/nixpkgs/pkgs/by-name/co/cosmic-player/package.nix:106:36:
          105|     license = lib.licenses.gpl3Only;
          106|     maintainers = lib.maintainers; [ ahoneybun ];
             |                                    ^
          107|     platforms = platforms.linux;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

maintainers = with lib.maintainers; [ ahoneybun ];

Closely scoped is acceptable I believe.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Should be good to go in that I believe.

@RossComputerGuy RossComputerGuy added the 12.approvals: 1 This PR was reviewed and approved by one person. label Jan 17, 2025
@RossComputerGuy RossComputerGuy merged commit fcbaac0 into NixOS:master Jan 20, 2025
@ahoneybun ahoneybun deleted the init-cosmic-player branch January 20, 2025 15:08
@ghost ghost mentioned this pull request Jan 22, 2025
13 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. 12.approvals: 1 This PR was reviewed and approved by one person.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants