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

snowsql: Add support for darwin platforms #320328

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

andehen
Copy link
Contributor

@andehen andehen commented Jun 16, 2024

Description of changes

Add support for darwin platforms (x86-64 and arm64)

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.11 Release Notes (or backporting 23.11 and 24.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.

@afh
Copy link
Member

afh commented Jun 28, 2024

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

1 package built:
  • snowsql

Copy link
Member

@afh afh left a comment

Choose a reason for hiding this comment

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

Thanks for this, @andehen, much appreciated 👍

Please find below a few comments and suggestions for improvement.
Hopefully I've been able to explain well what the intent of the suggestions are and provide helpful context, if not please ask me to clarify 🙂

pkgs/applications/misc/snowsql/darwin.nix Outdated Show resolved Hide resolved
pkgs/applications/misc/snowsql/darwin.nix Outdated Show resolved Hide resolved
pkgs/applications/misc/snowsql/darwin.nix Outdated Show resolved Hide resolved
pkgs/applications/misc/snowsql/darwin.nix Outdated Show resolved Hide resolved
pkgs/applications/misc/snowsql/darwin.nix Outdated Show resolved Hide resolved
pkgs/applications/misc/snowsql/default.nix Outdated Show resolved Hide resolved
pkgs/applications/misc/snowsql/linux.nix Show resolved Hide resolved
pkgs/applications/misc/snowsql/linux.nix Outdated Show resolved Hide resolved
pkgs/applications/misc/snowsql/linux.nix Outdated Show resolved Hide resolved
pkgs/applications/misc/snowsql/linux.nix Outdated Show resolved Hide resolved
@andehen
Copy link
Contributor Author

andehen commented Jul 6, 2024

Thanks for this, @andehen, much appreciated 👍

Please find below a few comments and suggestions for improvement. Hopefully I've been able to explain well what the intent of the suggestions are and provide helpful context, if not please ask me to clarify 🙂

Thanks a lot for your review! Really appreciate that you spent time doing this. Writing nix packages is new to me and I learned a lot 👍

Copy link
Member

@afh afh left a comment

Choose a reason for hiding this comment

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

Thanks for considering and accepting the suggestions, @andehen, much appreciated. And glad to hear you found the comments helpful.

pkgs/applications/misc/snowsql/linux.nix Show resolved Hide resolved
@afh
Copy link
Member

afh commented Jul 9, 2024

@AndersonTorres would be willing to lend your expertise on this one too? It seems the changes in this PR might be related to what is being talked about in #325562.

@AndersonTorres
Copy link
Member

Well, this is an unfree, binary-only package, and some of observations I did before for vlc and pcsx2 do not apply.

However, I believe it is not really interesting to "merge" both codes in a same expressions, because they are different. There is few to no benefit on "factoring the common terms" since the common term is too small.

Further, it falls on the previous problem: the Linux and MacOS binaries follow distinct paths, and the maintainers of one have no interest in maintain the other (in a legal sense, something like "I have the tools to test and approve a PR").

Yes, I have noticed the maintainers are the same in this case, but the general principle still applies.

Suppose that a a third person wants to contribute by sending a PR updating only the Apple package, since theey has no Linux to test it. It would be counterproductive to block such a PR.

@afh
Copy link
Member

afh commented Jul 10, 2024

There is few to no benefit on "factoring the common terms" since the common term is too small.

I find this 👆 very helpful as a general guidance, thanks @AndersonTorres.

Regarding maintainers, would setting the maintainers by platforms be useful? E.g.:

meta {
  # …
  maintainers = with lib.maintainers; [
    ] ++ lib.optionals stdenv.isDarwin [
      afh
    ] ++ lib.optionals stdenv.isLinux [
      AndersonTorres
    ];
}

I'm not sure I completely understand your example of a third person wanting to contribute to the Apple package. Let me rephrase in my own words to see if I did:
If a single package that needs to be written very differently per platform, due to the installation method on the platform, it is possible that an update to the Apple part could break the Linux part, yet the Apple contributors/maintainers may not be able to check the Linux part and the Linux contributors/maintainers may not be able to check the Apple part, creating a situation where there are unnecessary complications to move forward with the proposed changes?
Did I get that right?

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

3 participants