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

spotify-unwrapped: add macOS builds #195473

Merged
merged 3 commits into from Apr 18, 2023
Merged

Conversation

Enzime
Copy link
Member

@Enzime Enzime commented Oct 11, 2022

Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • 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/)
  • 22.11 Release Notes (or backporting 22.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
    • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
  • Fits CONTRIBUTING.md.

@nixos-discourse
Copy link

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

https://discourse.nixos.org/t/prs-ready-for-review/3032/1334

pkgs/top-level/all-packages.nix Outdated Show resolved Hide resolved
pkgs/applications/audio/spotify/linux.nix Show resolved Hide resolved
pkgs/applications/audio/spotify/linux.nix Show resolved Hide resolved
pkgs/applications/audio/spotify/linux.nix Show resolved Hide resolved
pkgs/applications/audio/spotify/darwin.nix Outdated Show resolved Hide resolved
@Enzime
Copy link
Member Author

Enzime commented Nov 1, 2022

@SuperSandro2000 all I did as part of this PR was move the contents from default.nix to linux.nix, what do you think about fixing these issues in a different PR to keep this one focused?

@laMudri
Copy link
Contributor

laMudri commented Nov 1, 2022

I haven't used this package for years. Please could I be removed from the maintainers list?

Copy link
Member

@Atemu Atemu left a comment

Choose a reason for hiding this comment

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

Result of nixpkgs-review pr 195473 run on aarch64-darwin 1

1 package built:
  • spotify (spotify-unwrapped)

Opened it and it works as expected. I'm on Monterey though, so idk whether it'd work with Ventura's new Gatekeeper but we're not modifying it, so it should.

@nixos-discourse
Copy link

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

https://discourse.nixos.org/t/prs-already-reviewed/2617/666

@SuperSandro2000
Copy link
Member

@SuperSandro2000 all I did as part of this PR was move the contents from default.nix to linux.nix, what do you think about fixing these issues in a different PR to keep this one focused?

GitHub has no support for displaying moved hunks like the git cli so unless I compare the lines one by one I do not know that. Fine for me but the wrapper condition needs to be less awkward between Linux and Darwin. Let me create a PR that removes the wrapper.

@SuperSandro2000
Copy link
Member

#199779

@SuperSandro2000
Copy link
Member

I just merged the consolidation of the wrapper. Now we don't need to think about the differences between darwin and linux and can make the package decision in top-level.nix or in the package. I have a preference to do it inside of the package but the other way is also fine.

@Enzime
Copy link
Member Author

Enzime commented Nov 6, 2022

@SuperSandro2000 rebased on top of master 👍

Enzime added a commit to Enzime/nixpkgs that referenced this pull request Mar 27, 2023
@nixos-discourse
Copy link

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

https://discourse.nixos.org/t/prs-already-reviewed/2617/883

@yamashitax
Copy link
Contributor

Reviewed points
  • package name fits guidelines
  • package version fits guidelines
  • package build on aarch64-darwin
  • executables tested on aarch64-darwin
  • all depending packages build
Possible improvements

Unable to build due to a merge conflict.

Comments

Result of nixpkgs-review

➜ nix-shell -p nixpkgs-review --run "nixpkgs-review pr 195473"
$ git -c fetch.prune=false fetch --no-tags --force https://github.com/NixOS/nixpkgs master:refs/nixpkgs-review/0 pull/195473/head:refs/nixpkgs-review/1
remote: Enumerating objects: 24, done.
remote: Counting objects: 100% (22/22), done.
remote: Compressing objects: 100% (6/6), done.
remote: Total 12 (delta 9), reused 6 (delta 5), pack-reused 0
Unpacking objects: 100% (12/12), 1.54 KiB | 142.00 KiB/s, done.
From https://github.com/NixOS/nixpkgs
   8f8bb3bb7ed..ed7e3a25a9d  master     -> refs/nixpkgs-review/0
$ git worktree add /Users/yamashita/.cache/nixpkgs-review/pr-195473-1/nixpkgs ed7e3a25a9d964177bd52ff31238bd950dca6807
Preparing worktree (detached HEAD ed7e3a25a9d)
Updating files: 100% (34527/34527), done.
HEAD is now at ed7e3a25a9d Merge pull request #226285 from marsam/update-flexget
$ git merge --no-commit --no-ff 5f78d39caa98d86f1b29caee5733ff4febe0141e
Auto-merging pkgs/applications/audio/spotify/default.nix
CONFLICT (content): Merge conflict in pkgs/applications/audio/spotify/default.nix
Auto-merging pkgs/applications/audio/spotify/update.sh
Automatic merge failed; fix conflicts and then commit the result.
https://github.com/NixOS/nixpkgs/pull/195473 failed to build
$ git worktree prune

@Enzime
Copy link
Member Author

Enzime commented Apr 17, 2023

@yamashitax updated 👍

@Enzime
Copy link
Member Author

Enzime commented Apr 17, 2023

@yamashitax updated 👍 if you could review again that would be much appreciated

Enzime added a commit to Enzime/nixpkgs that referenced this pull request Apr 18, 2023
Enzime added a commit to Enzime/nixpkgs that referenced this pull request Apr 18, 2023
Copy link
Contributor

@yamashitax yamashitax left a comment

Choose a reason for hiding this comment

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

Result of nixpkgs-review pr 195473 run on aarch64-darwin 1

1 package built:
  • spotify

Works as intended

@yamashitax
Copy link
Contributor

@SuperSandro2000 Seems fine to merge this now, from what I can tell.

Copy link
Member

@Atemu Atemu left a comment

Choose a reason for hiding this comment

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

Result of nixpkgs-review pr 195473 run on aarch64-darwin 1

1 package built:
  • spotify

@Atemu
Copy link
Member

Atemu commented Apr 18, 2023

(This is on Ventura; works with Gatekeeper.)


stdenv.mkDerivation {
inherit pname;

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

@SuperSandro2000 SuperSandro2000 merged commit 25f40b0 into NixOS:master Apr 18, 2023
20 of 21 checks passed
@Enzime Enzime deleted the spotify-darwin branch April 20, 2023 09:14
SuperSandro2000 pushed a commit to SuperSandro2000/nixpkgs that referenced this pull request Apr 20, 2023
@jtojnar
Copy link
Contributor

jtojnar commented Apr 30, 2023

GitHub has no support for displaying moved hunks like the git cli so unless I compare the lines one by one I do not know that. Fine for me but the wrapper condition needs to be less awkward between Linux and Darwin. Let me create a PR that removes the wrapper.

That is why it is good idea to rename files in a separate commit from any other changes. That way the change commit can be reviewed separately and git blame will continue to work.

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.

None yet

7 participants