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.

Sign up for GitHub

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

Jump to bottom

create-dmg: init 1.2.2 #319628

Merged
merged 1 commit into from
Aug 9, 2024
Merged

create-dmg: init 1.2.2 #319628

merged 1 commit into from
Aug 9, 2024

Conversation

heywoodlh
Copy link
Contributor

@heywoodlh heywoodlh commented Jun 13, 2024 •
edited
Loading

Description of changes

Adds create-dmg, a DMG creator for MacOS

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.

@github-actions github-actions bot added the 8.has: maintainer-list (update) This PR changes `maintainers/maintainer-list.nix` label Jun 13, 2024
@ofborg ofborg bot added 8.has: package (new) This PR adds a new package 11.by: package-maintainer This PR was created by the maintainer of the package it changes 10.rebuild-darwin: 1-10 10.rebuild-darwin: 1 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux labels Jun 13, 2024
clebs
clebs reviewed Jun 13, 2024
View reviewed changes
Copy link
Member

@clebs clebs left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for the contribution! 🚀
Also your commits need to be reversed (first maintainers and then create-dmg init)

pkgs/by-name/cr/create-dmg/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/cr/create-dmg/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/cr/create-dmg/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/cr/create-dmg/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/cr/create-dmg/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/cr/create-dmg/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/cr/create-dmg/package.nix Outdated Show resolved Hide resolved
@heywoodlh heywoodlh mentioned this pull request Jun 13, 2024
Closed
@heywoodlh
Copy link
Contributor Author

@clebs thank you for the thoughtful review! I believe I addressed all your concerns in my most recent commit: f8ada611c660eb83d6611558081dbdbbfb50fbc8

(Sorry if the notifications I generated resolving the conversations were obnoxious)

@heywoodlh
Copy link
Contributor Author

Also, here's how I tested functionality (it seems to work as expected):

/tmp/test
❯ mkdir source_folder; echo stuff > source_folder/file.txt; echo other-stuff > source_folder/file2.txt

/tmp/test
❯ create-dmg \
        --volname "Application Installer" \
        --window-pos 200 120 \
        --window-size 800 400 \
        --icon-size 100 \
        --icon "Application.app" 200 190 \
        --hide-extension "Application.app" \
        --app-drop-link 600 185 \
        "test.dmg" \
        "source_folder/"

And the resulting Volume in Finder with the contents:

image

AndersonTorres
AndersonTorres suggested changes Jun 13, 2024
View reviewed changes
pkgs/by-name/cr/create-dmg/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/cr/create-dmg/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/cr/create-dmg/package.nix Outdated Show resolved Hide resolved
@heywoodlh
Copy link
Contributor Author

@AndersonTorres thanks for the feedback, I believe I've addressed your feedback on the most recent commit

AndersonTorres
AndersonTorres reviewed Jun 14, 2024
View reviewed changes
pkgs/by-name/cr/create-dmg/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/cr/create-dmg/package.nix Outdated Show resolved Hide resolved
@heywoodlh heywoodlh force-pushed the create-dmg branch 2 times, most recently from c586e55 to 492c59e Compare June 16, 2024 18:05
clebs
clebs approved these changes Jun 16, 2024
View reviewed changes
Copy link
Member

@clebs clebs left a comment

Choose a reason for hiding this comment

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

LGTM!

AndersonTorres
AndersonTorres suggested changes Jun 17, 2024
View reviewed changes
Copy link
Member

@AndersonTorres AndersonTorres left a comment

Choose a reason for hiding this comment

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

Reword the commit as

create-dmg: init at 1.2.2

@heywoodlh
Copy link
Contributor Author

@AndersonTorres done

Out of curiosity and for my future reference, is the phrasing of the commit that important before being merged into master? If so, why?

@AndersonTorres
Copy link
Member

  1. This is pontificated at Contributing docs: https://github.com/NixOS/nixpkgs/blob/master/pkgs%2FREADME.md

  2. The formatting allows grepping the commit logs more easily, in a similar fashion to the "conventional commits" standard (which I think it is ugly).

XKCD 1296

@superherointj superherointj changed the title create-dmg init: v1.2.2 create-dmg: init 1.2.2 Jun 18, 2024
@superherointj superherointj added the 6.topic: darwin Running or building packages on Darwin label Jun 18, 2024
afh
afh suggested changes Jun 28, 2024
View reviewed changes
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, @heywoodlh, much appreciated. Please find below a minor suggestion for improvement.

pkgs/by-name/cr/create-dmg/package.nix Outdated
installFlags = [ "prefix=$(out)" ];

meta = {
description = "A shell script to build fancy DMGs";
Copy link
Member

Choose a reason for hiding this comment

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

Please remove the indefinite article from the beginning of the description as recommended in section "Meta attributes" in pkgs/README.md

Suggested change
description = "A shell script to build fancy DMGs";
description = "Shell script to build fancy DMGs";

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added on most recent commit, thanks!

Copy link
Contributor

Choose a reason for hiding this comment

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

@afh I can confirm that change. Can we get an approval?

Copy link
Member

Choose a reason for hiding this comment

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

The changes look good to me, yet having all checks pass would be helpful, @sarahec.

@afh
Copy link
Member

afh commented Jul 10, 2024

Something seems off with the checks, not sure if the following is going to work to re-trigger the checks…

@ofborg test

@sarahec
Copy link
Contributor

sarahec commented Jul 10, 2024 •
edited
Loading

@afh the merge check timed out two weeks ago. Do we ask @heywoodlh to rebase atop master and force push, knowing it'll lose the one approval (@clebs)?

@heywoodlh
Copy link
Contributor Author

Let me know what you need from me and I'll be happy to make those changes

@afh
Copy link
Member

afh commented Jul 11, 2024

This PRs branch is ~12k commits behind master, possibly that is the reason why checks time out. @heywoodlh, could you rebase this PRs branch onto master, please?

@DontEatOreo
Copy link
Member

@heywoodlh Are you still interested in pursuing this PR?

@heywoodlh
Copy link
Contributor Author

Oops, sorry, I missed the other messages! Will rebase off of updated master branch later today.

@DontEatOreo
Copy link
Member

Thank you!

@github-actions github-actions bot removed the 8.has: maintainer-list (update) This PR changes `maintainers/maintainer-list.nix` label Aug 8, 2024
@heywoodlh
Copy link
Contributor Author

Rebased onto master, let me know if there's any other follow up that is needed.

@afh
Copy link
Member

afh commented Aug 8, 2024

Thanks for rebase, @heywoodlh, mind formatting the package using:
nix run nixpkgs#nixfmt-rfc-style -- pkgs/by-name/cr/create-dmg/package.nix, please? This should address the failed nixfmt check..

@heywoodlh
Copy link
Contributor Author

@afh done! Also, added meta.changelog in there while I was at it.

@afh
Copy link
Member

afh commented Aug 8, 2024

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

1 package built:
  • create-dmg

afh
afh approved these changes Aug 8, 2024
View reviewed changes
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 the latest updates on this PR, much appreciated 👍

@afh afh added the 12.approvals: 2 This PR was reviewed and approved by two reputable people label Aug 8, 2024
@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/1892

@DontEatOreo DontEatOreo requested a review from JohnRTitor August 8, 2024 22:47
@DontEatOreo
Copy link
Member

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

1 package built:
  • create-dmg

@DontEatOreo
Copy link
Member

Result of nixpkgs-review pr 319628 run on x86_64-darwin 1

1 package built:
  • create-dmg

@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/1895

@JohnRTitor
Copy link
Contributor

Result of nixpkgs-review pr 319628 run on x86_64-linux 1

@AndersonTorres
Copy link
Member

Result of nixpkgs-review pr 319628 run on x86_64-linux 1

Not conclusive, since it is Darwin-only.

@JohnRTitor
Copy link
Contributor

Yeah, tried on the nix-community darwin builder and it succeeded.

johnrtitor@darwin01 ~ % nix build "github:nixos/nixpkgs/refs/pull/319628/merge#create-dmg" --print-out-paths
/nix/store/ajjyja98l3xa05w6rvrzi14arj23g4sd-create-dmg-1.2.2
nix build "github:nixos/nixpkgs/refs/pull/319628/merge#create-dmg"   1.70s user 12.60s system 60% cpu 23.516 total
johnrtitor@darwin01 ~ % ./result/bin/create-dmg
Not enough arguments. Run 'create-dmg --help' for help.
1 johnrtitor@darwin01 ~ % ./result/bin/create-dmg --help
create-dmg 1.2.1

Creates a fancy DMG file.

Usage:  create-dmg [options] <output_name.dmg> <source_folder>

All contents of <source_folder> will be copied into the disk image.

@JohnRTitor JohnRTitor merged commit 8c8426d into NixOS:master Aug 9, 2024
29 checks passed
@github-actions github-actions bot mentioned this pull request Aug 9, 2024
Merged
1 task
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Aug 9, 2024

Successfully created backport PR for release-24.05:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sarahec sarahec sarahec left review comments

@wolfgangwalther wolfgangwalther wolfgangwalther left review comments

@DontEatOreo DontEatOreo DontEatOreo approved these changes

@JohnRTitor JohnRTitor JohnRTitor approved these changes

@AndersonTorres AndersonTorres AndersonTorres approved these changes

@clebs clebs clebs approved these changes

@afh afh afh approved these changes

Assignees
No one assigned
Labels
6.topic: darwin Running or building packages on Darwin 8.has: package (new) This PR adds a new package 10.rebuild-darwin: 1-10 10.rebuild-darwin: 1 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux 11.by: package-maintainer This PR was created by the maintainer of the package it changes 12.approvals: 2 This PR was reviewed and approved by two reputable people
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
10 participants
@heywoodlh @AndersonTorres @afh @sarahec @DontEatOreo @nixos-discourse @JohnRTitor @clebs @wolfgangwalther @superherointj