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

itsycal: init at 0.15.3 #299294

Merged
1 commit merged into from
Mar 27, 2024
Merged

itsycal: init at 0.15.3 #299294

1 commit merged into from
Mar 27, 2024

Conversation

DontEatOreo
Copy link
Member

Description of changes

Continuation of #299257

I'm really sorry I had to open a new issue, but OfBorg gave errors for a build that works perfectly fine.

OfBorg log: https://gist.github.com/GrahamcOfBorg/bfaae351d6dc61a08283686ab23a0e80

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.05 Release Notes (or backporting 23.05 and 23.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
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux labels Mar 26, 2024
@DontEatOreo DontEatOreo requested a review from stepbrobd March 26, 2024 21:44
@stepbrobd
Copy link
Member

The eval bug was fixed in #299288

pname = "itsycal";
version = "0.15.3";

src = fetchurl {
Copy link
Member

Choose a reason for hiding this comment

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

Any specific reason why fetchurl used but not fetchzip?

Copy link
Member Author

Choose a reason for hiding this comment

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

Nope, I didn't realize there was fetchzip will change it

Copy link
Member Author

@DontEatOreo DontEatOreo Mar 26, 2024

Choose a reason for hiding this comment

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

On a second note maybe not... fetchzip is getting different hash and it seems like it's not downloading stuff properly either?

@nix { "action": "setPhase", "phase": "unpackPhase" }
unpacking sources
unpacking source archive /nix/store/ryja8i37q8kci47blxdcmjr6sm51lr9k-source
source root is .
@nix { "action": "setPhase", "phase": "patchPhase" }
patching sources
@nix { "action": "setPhase", "phase": "updateAutotoolsGnuConfigScriptsPhase" }
updateAutotoolsGnuConfigScriptsPhase
@nix { "action": "setPhase", "phase": "configurePhase" }
configuring
no configure script, doing nothing
@nix { "action": "setPhase", "phase": "buildPhase" }
building
no Makefile or custom buildPhase, doing nothing
@nix { "action": "setPhase", "phase": "installPhase" }
installing
total 12
-rw-r--r-- 1 _nixbld1 nixbld  158 Mar 26 21:58 .sandbox.sb
-rw-r--r-- 1 _nixbld1 nixbld 5104 Mar 26 21:58 env-vars
drwxr-xr-x 3 _nixbld1 nixbld   96 Jan  1  1970 source
mv: cannot stat 'Itsycal.app': No such file or directory

I suggest it stays fetchurl unless fetchzip requires extra stuff I'm not aware of that would fix this? 🤔

The diff for the output above:

diff --git a/pkgs/os-specific/darwin/itsycal/default.nix b/pkgs/os-specific/darwin/itsycal/default.nix
index 4ce7d3ae084f..85c07d6eeb9d 100644
--- a/pkgs/os-specific/darwin/itsycal/default.nix
+++ b/pkgs/os-specific/darwin/itsycal/default.nix
@@ -1,5 +1,5 @@
 { lib
-, fetchurl
+, fetchzip
 , stdenvNoCC
 , unzip
 }:
@@ -8,9 +8,9 @@ stdenvNoCC.mkDerivation (finalAttrs: {
   pname = "itsycal";
   version = "0.15.3";
 
-  src = fetchurl {
+  src = fetchzip {
     url = "https://itsycal.s3.amazonaws.com/Itsycal-${finalAttrs.version}.zip";
-    hash = "sha256-5aJzSuqq31B33jW4lV8vuU3eurpZBoyIW/AOC9/pxng=";
+    hash = "sha256-jpTlJY7yAARrkHzreQKbFaKj0sYp950R0qPPcDeY6AE=";
   };
 
   sourceRoot = ".";
@@ -20,6 +20,7 @@ stdenvNoCC.mkDerivation (finalAttrs: {
   installPhase = ''
     runHook preInstall
 
+    ls -lA
     mkdir -p $out/Applications
     mv Itsycal.app $out/Applications

Copy link
Member

Choose a reason for hiding this comment

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

This worked for me (haven't tested the app tho):

diff --git a/pkgs/os-specific/darwin/itsycal/default.nix b/pkgs/os-specific/darwin/itsycal/default.nix
index 4ce7d3ae084f..8d727e703650 100644
--- a/pkgs/os-specific/darwin/itsycal/default.nix
+++ b/pkgs/os-specific/darwin/itsycal/default.nix
@@ -1,5 +1,5 @@
 { lib
-, fetchurl
+, fetchzip
 , stdenvNoCC
 , unzip
 }:
@@ -8,20 +8,16 @@ stdenvNoCC.mkDerivation (finalAttrs: {
   pname = "itsycal";
   version = "0.15.3";

-  src = fetchurl {
+  src = fetchzip {
     url = "https://itsycal.s3.amazonaws.com/Itsycal-${finalAttrs.version}.zip";
-    hash = "sha256-5aJzSuqq31B33jW4lV8vuU3eurpZBoyIW/AOC9/pxng=";
+    hash = "sha256-jpTlJY7yAARrkHzreQKbFaKj0sYp950R0qPPcDeY6AE=";
   };

-  sourceRoot = ".";
-
-  nativeBuildInputs = [ unzip ];
-
   installPhase = ''
     runHook preInstall

-    mkdir -p $out/Applications
-    mv Itsycal.app $out/Applications
+    mkdir -p $out/Applications/Itsycal.app
+    cp -R . $out/Applications/Itsycal.app

     runHook postInstall
   '';
diff --git a/pkgs/top-level/all-packages.nix b/pkgs/top-level/all-packages.nix
index c014c268962a..973238d5c8e3 100644
--- a/pkgs/top-level/all-packages.nix
+++ b/pkgs/top-level/all-packages.nix
@@ -38276,6 +38276,8 @@ with pkgs;

   itsx = callPackage ../applications/science/biology/itsx { };

+  itsycal = callPackage ../os-specific/darwin/itsycal { };
+
   iv = callPackage ../applications/science/biology/iv {
     neuron-version = neuron.version;
   };

Copy link
Member

Choose a reason for hiding this comment

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

I guess the problem was due to

  • sourceRoot = "."; (wrong location?)
  • mv Itsycal.app $out/Applications (nix store is read-only).

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for the help!

@DontEatOreo
Copy link
Member Author

The eval bug was fixed in #299288

Oh, nice I thought I did something wrong glad it was fix this fast as well! I guess you could ping OfBerg to eval it again, good to know for the future!

@stepbrobd
Copy link
Member

Seems like you'll have to migrate to pkgs/by-name:

  • remove the entry from all-packages.nix
  • create corresponding new directories in the pkgs/by-name folder
  • default.nix -> package.nix

@DontEatOreo
Copy link
Member Author

Wait... I already have some another packages merged (#299032) without it being declared in all-packages.nix would I need to move that too?

Or shouldn't this package and #299032 stay in os-specific/darwin since that's the only platform they support?

@stepbrobd
Copy link
Member

Sounds good, let's keep it as is.

Btw, seems like the package introduced in #299032 doesn't have an entry in all-packages.nix? Also, it's using fetchurl.

@DontEatOreo
Copy link
Member Author

Sounds good, let's keep it as is.

I'm not sure what you mean here? Do you want me to move over to pkgs/by-name OR stay in os-specific/darwin and remove the entry from all-packages.nix?

Btw, seems like the package introduced in #299032 doesn't have an entry in all-packages.nix? Also, it's using fetchurl.

That's what I was saying, because if it had an entry, then it would have to be moved to pkgs/by-name.

I see some other packages like raycast for example, which is in os-specific/darwin/ that is also in the all-packages.nix

description = "Itsycal is a tiny menu bar calendar";
homepage = "https://www.mowglii.com/itsycal/";
license = lib.licenses.mit;
maintainers = with lib.maintainers; [ DontEatOreo ];
Copy link
Member

@stepbrobd stepbrobd Mar 26, 2024

Choose a reason for hiding this comment

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

$ nix run nixpkgs#nixpkgs-review -- pr --print-result 299294
...
error: undefined variable 'DontEatOreo'

       at /Users/ysun/.cache/nixpkgs-review/pr-299294/nixpkgs/pkgs/os-specific/darwin/itsycal/default.nix:29:43:

           28|     license = lib.licenses.mit;
           29|     maintainers = with lib.maintainers; [ DontEatOreo ];

Please add an entry to https://github.com/NixOS/nixpkgs/blob/master/maintainers/maintainer-list.nix and backport it to your earliest contribution in nixpkgs where lib.maintainers.DontEatOreo is referenced.

I think it'd better if we involve a nixpkgs committer to see how we can resolve this, since #299032 was merged without lib.maintainers.DontEatOreo being defined.

Relevant: https://github.com/NixOS/nixpkgs/tree/master/maintainers

@drupol

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm already there

donteatoreo = {
name = "DontEatOreo";
github = "DontEatOreo";
githubId = 57304299;
keys = [{
fingerprint = "33CD 5C0A 673C C54D 661E 5E4C 0DB5 361B EEE5 30AB";
}];
};

I guess it was a bit stupid of me, but didn't realize it was case-sensitive

Copy link
Member

Choose a reason for hiding this comment

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

Ah, nice! I guess we would need a new PR to have the package introduced in #299032 to reference the correct maintainer (correct casing), and backport it

Copy link
Member Author

Choose a reason for hiding this comment

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

Will do tomorrow

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for letting me know !

@stepbrobd
Copy link
Member

I'm not sure what you mean here? Do you want me to move over to pkgs/by-name OR stay in os-specific/darwin and remove the entry from all-packages.nix?

Sorry about my wording. Please keep pkgs/os-specific/darwin/itsycal/default.nix as is and leave the itsycal entry in pkgs/top-level/all-packages.nix.

The only thing we need now is to add your maintainer metadata to maintainers/maintainer-list.nix

@ghost
Copy link

ghost commented Mar 26, 2024

I'm not sure what you mean here? Do you want me to move over to pkgs/by-name OR stay in os-specific/darwin and remove the entry from all-packages.nix?

Sorry about my wording. Please keep pkgs/os-specific/darwin/itsycal/default.nix as is and leave the itsycal entry in pkgs/top-level/all-packages.nix.

The only thing we need now is to add your maintainer metadata to maintainers/maintainer-list.nix

i thought that new packages all go under by-name

@reckenrode
Copy link
Contributor

i thought that new packages all go under by-name

That was my understanding too. I’d expect packages in os-specific/darwin to be added to darwin-packages.nix not all-packages.nix.

@stepbrobd
Copy link
Member

i thought that new packages all go under by-name

I think it'd be better to keep itsycal in pkgs/os-specific/darwin/ and wait until #293498 merges, then migrate over to by-name with darwin.installBinaryPackage.

If the consensus here is to init itsycal in by-name or something else, we should follow that.

@ghost
Copy link

ghost commented Mar 27, 2024

i thought that new packages all go under by-name

I think it'd be better to keep itsycal in pkgs/os-specific/darwin/ and wait until #293498 merges, then migrate over to by-name with darwin.installBinaryPackage.

If the consensus here is to init itsycal in by-name or something else, we should follow that.

package guidelines are pretty clear where new apps are. let's keep this area clean. also install-binary-package is not clear it belongs here either.

@ghost
Copy link

ghost commented Mar 27, 2024

another benefit is being able to take advantage of the merge-bot in by-name https://github.com/NixOS/nixpkgs-merge-bot

@DontEatOreo
Copy link
Member Author

I guess I don't need to define itsycal in all-packages.nix?

https://github.com/NixOS/nixpkgs/actions/runs/8453647586/job/23156800994?pr=299294#step:7:31

Such a definition is provided automatically and therefore not necessary. Please remove it.

@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 and removed 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin labels Mar 27, 2024
@DontEatOreo DontEatOreo requested a review from stepbrobd March 27, 2024 16:19
@stepbrobd
Copy link
Member

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

1 package built:
  • itsycal

@ghost ghost merged commit 9806daf into NixOS:master Mar 27, 2024
26 of 27 checks passed
Copy link
Contributor

Successfully created backport PR for release-23.11:

@DontEatOreo DontEatOreo deleted the pkgs-itsycal branch May 3, 2024 10:08
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants