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

vlc: add aarch64-darwin and x86_64-darwin to platforms #209086

Closed
wants to merge 1 commit into from

Conversation

willemml
Copy link
Contributor

@willemml willemml commented Jan 4, 2023

Description of changes

Adds a darwin version of VLC.

Files formatted with nixfmt.

nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD" fails because it cannot find ibtool which is bundled with Xcode (it isn't a nix package, but it is on my system even though nix-shell doesn't find it.)

I did not add myself to the maintainers list.

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/)
  • 23.05 Release Notes (or backporting 22.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
    • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
  • Fits CONTRIBUTING.md.

@ofborg ofborg bot added the 6.topic: darwin Running or building packages on Darwin label Jan 4, 2023
@wegank
Copy link
Member

wegank commented Jan 4, 2023

I guess reformatting the files with nixfmt (at least for linux.nix) is not a good idea, as this would clearly mystify the patch. The maintainer of the package also seem to use nixpkgs-fmt instead.

@willemml
Copy link
Contributor Author

willemml commented Jan 4, 2023

I'll reformat it with nixpkgs-fmt, I thought nixfmt was the same thing.

@willemml willemml force-pushed the willemml/vlc-mac branch 3 times, most recently from 20134b3 to 28c5f4e Compare January 4, 2023 21:36
Comment on lines 166 to 167
++ optionals skins2Support
(with xorg; [ freetype libXext libXinerama libXpm ])
Copy link
Member

@wegank wegank Jan 4, 2023

Choose a reason for hiding this comment

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

The original default.nix has

  optionals skins2Support (with xorg; [
    freetype
    libXext
    libXinerama
    libXpm
  ])

so reformatting with nixfmt and then nixpkgs-fmt causes inconsistencies. I suggest leaving the original content untouched instead.

src = fetchurl {
url = "http://get.videolan.org/vlc/${version}/vlc-${version}.tar.xz";
sha256 = "sha256-VwlEOcNl2KqLm0H6MIDMDu8r7+YCW7XO9yKszGJa7ew=";
srcs = rec {
Copy link
Member

Choose a reason for hiding this comment

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

srcs and meta should be declared separately for the two derivations: otherwise, linux.nix would still be evaluable on darwin, and thus ibtool errors.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do I have to declare the entirety of meta separately? Or can I declare it once and then append the platforms field?

I've now declared srcs separately and the platforms for meta (but not descriptions, and etc) separately but I still get the same ibtool error.

Copy link
Member

Choose a reason for hiding this comment

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

Oh, I checked the expressions in nixpkgs, and I think I'm wrong. Sorry for the inconvenience! It should be OK to inherit both of them. Let's see if ofborg evals.

@AndersonTorres
Copy link
Member

AndersonTorres commented Jan 6, 2023

This is ugly as hell.

First, it adds a binary-only package as if it was the same thing as a pure source.
Indeed, when I read the notification I thought "hey, they found some patch for building that pandemonium on Mac!". And what I found? A blob.

Second, it renames files without notice.

And third, I am far from being interested in keeping maintenance of a blob for a platform I have no access. Create something like vlc-bin = callPackage ../applications/video/vlc/bin.nix {}; and put your vlc-mac on it.

@AndersonTorres AndersonTorres marked this pull request as draft January 6, 2023 00:34
@willemml
Copy link
Contributor Author

willemml commented Jan 6, 2023

I realize now that binaries are undesirable, I'll just close this for now. Sorry for wasting the time of everyone who reviewed this.

@willemml willemml closed this Jan 6, 2023
@AndersonTorres
Copy link
Member

AndersonTorres commented Jan 6, 2023

Binaries are not undesirable per se. And, as an organization, NixOS/nixpkgs is not hostile to binary-only packages.

Sometimes binary-only packages are the only known option (nvidia blobs, boostrap compilers) or the source compilations is overly prohibitive (some Java packages).

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

4 participants