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

unimatrix: init at unstable-2023-04-25 #264716

Merged
merged 1 commit into from
Nov 21, 2023
Merged

unimatrix: init at unstable-2023-04-25 #264716

merged 1 commit into from
Nov 21, 2023

Conversation

Anomalocaridid
Copy link
Contributor

Description of changes

Adds unimatrix, a program that displays the falling code from "The Matrix" in a terminal. It is similar to cmatrix, but uses Unicode for Japanese characters so that they can be used out of the box.

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/)
  • 23.11 Release Notes (or backporting 23.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.

h7x4
h7x4 reviewed Nov 1, 2023
View reviewed changes
Copy link
Member

@h7x4 h7x4 left a comment

Choose a reason for hiding this comment

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

Here are some initial comments. I'm still a bit unsure about the nativeBuildInputs, so I hope some other reviewers that know more about scm python applications could have a closer look. Cool stuff!

pkgs/by-name/un/unimatrix/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/un/unimatrix/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/un/unimatrix/package.nix Outdated Show resolved Hide resolved
@Anomalocaridid
Copy link
Contributor Author

Here are some initial comments. I'm still a bit unsure about the nativeBuildInputs, so I hope some other reviewers that know more about scm python applications could have a closer look. Cool stuff!

I do not know much about scm python applications either, but if it helps, the reason I had pip and git as nativeBuildInputs was because unimatrix would not be built without an error otherwise. To my understanding, it required the pip python module and needed to run git to get version information.

Although after implementing your recommendations, I was able to remove pip, which I suspect may have something to do with using python3Packages instead of python3.pkgs.

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

onemoresuza
onemoresuza reviewed Nov 9, 2023
View reviewed changes
Copy link
Contributor

@onemoresuza onemoresuza 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.

Reviewed points

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

2 packages built:
  • unimatrix
  • unimatrix.dist
  • package name fits guidelines
  • package version fits guidelines
  • package build on x86_64-linux
  • executables tested on x86_64-linux
  • all depending packages build

@onemoresuza
Copy link
Contributor

@robert-manchester is right; leaveDotGit should be avoided. Here's a discourse post discussing:

https://discourse.nixos.org/t/including-git-directory-in-build-environment/19204

@onemoresuza
Copy link
Contributor

The package does not have a valid python version, so env.SETUPTOOLS_SCM_PRETEND_VERSION = version; cannot be used --- see guake.

So format = "setuptools";, the default, is a no go --- see the nixpkgs manual on python.

But since the program is just a python file, format = "other"; can be used and a custom installPhase can be given like in pdfdiff.

@onemoresuza
Copy link
Contributor

@Anomalocaridid, ping me when ofborg finishes its ofborg-eval check.

@Anomalocaridid
Copy link
Contributor Author

@onemoresuza It looks like the ofborg-eval check is done.

@delroth delroth added the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Nov 12, 2023
pbsds
pbsds reviewed Nov 19, 2023
View reviewed changes
Copy link
Member

@pbsds pbsds 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 264716 run on x86_64-linux 1

1 package built:
  • unimatrix

Runs fine and LGTM, just a minor nit

pkgs/by-name/un/unimatrix/package.nix Outdated Show resolved Hide resolved
Anomalocaridid added a commit to Anomalocaridid/dotfiles that referenced this pull request Nov 20, 2023
@Anomalocaridid
flake.lock: Update
e2195a4 
Flake lock file updates:

• Updated input 'unimatrix-patch':
    'NixOS/nixpkgs#264716'
  → 'NixOS/nixpkgs#264716'
pbsds
pbsds approved these changes Nov 21, 2023
View reviewed changes
Copy link
Member

@pbsds pbsds 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 264716 run on x86_64-linux 1

1 package built:
  • unimatrix
  • package path fits guidelines
  • package name fits guidelines
  • package version fits guidelines
  • package builds on x86_64-linux
  • executables tested on x86_64-linux
  • meta.description is set and fits guidelines
  • meta.license fits upstream license
  • meta.platforms is set
  • meta.maintainers is set
  • build time only dependencies are declared in nativeBuildInputs
  • source is fetched using the appropriate function
  • the list of phases is not overridden
  • when a phase (like installPhase) is overridden it starts with runHook preInstall and ends with runHook postInstall.
  • patches have a comment describing either the upstream URL or a reason why the patch wasn't upstreamed
  • patches that are remotely available are fetched rather than vendored

LGTM, thanks!

@pbsds pbsds merged commit f3bc369 into NixOS:master Nov 21, 2023
22 checks passed
@Anomalocaridid
Copy link
Contributor Author

Thank you!

@Anomalocaridid Anomalocaridid deleted the unimatrix branch November 21, 2023 21:51
Anomalocaridid added a commit to Anomalocaridid/dotfiles that referenced this pull request Nov 25, 2023
@Anomalocaridid
flake.lock: Update
aaf698a 
Flake lock file updates:

• Updated input 'disko':
    'github:nix-community/disko/548962c50b8afad7b8c820c1d6e21dc8394d6e65' (2023-11-04)
  → 'github:nix-community/disko/2d7d77878c5d70f66f3d676ff66708d8d4f9d7df' (2023-11-12)
• Updated input 'home-manager':
    'github:nix-community/home-manager/c067d57fc4552835987fd7611c3a6741ee32ebd5' (2023-11-06)
  → 'github:nix-community/home-manager/8cedd63eede4c22deb192f1721dd67e7460e1ebe' (2023-11-24)
• Updated input 'hyprland-contrib':
    'github:hyprwm/contrib/e2cb3559cf435bd26585405de0c6b23855786337' (2023-11-05)
  → 'github:hyprwm/contrib/11bbb96e44818f67e5259c1788231f30fe5fe2e7' (2023-11-20)
• Updated input 'nix-gaming':
    'github:fufexan/nix-gaming/40bcc678e41285dbe0d9a306baf2c2123fd19027' (2023-11-05)
  → 'github:fufexan/nix-gaming/9ed5f7f0e6f14fb8956ddb8c0ed345d6b4aee2c0' (2023-11-25)
• Updated input 'nix-gaming/nixpkgs':
    'github:NixOS/nixpkgs/9d5d25bbfe8c0297ebe85324addcb5020ed1a454' (2023-11-04)
  → 'github:NixOS/nixpkgs/7414e9ee0b3e9903c24d3379f577a417f0aae5f1' (2023-11-16)
• Updated input 'nix-index-database':
    'github:Mic92/nix-index-database/5388a4002179d6778d212dc2fdcc7ac3fdbd5b65' (2023-11-05)
  → 'github:Mic92/nix-index-database/27920146e671a0d565aaa7452907383be14d8d82' (2023-11-19)
• Updated input 'nixpkgs':
    'github:NixOS/nixpkgs/85f1ba3e51676fa8cc604a3d863d729026a6b8eb' (2023-11-04)
  → 'github:NixOS/nixpkgs/19cbff58383a4ae384dea4d1d0c823d72b49d614' (2023-11-22)
• Updated input 'spicetify-nix':
    'github:the-argus/spicetify-nix/652aac2be378a6955c207b91ace8b0c9d687e743' (2023-11-06)
  → 'github:the-argus/spicetify-nix/3c702af1f717b7876fac08809201b1368d0d956c' (2023-11-14)
• Updated input 'stylix':
    'github:danth/stylix/07795247c2db08711bbd9503e01752c315be0805' (2023-11-01)
  → 'github:danth/stylix/a15c3196c1d620c18cbee8229092598384a89fef' (2023-11-18)
• Updated input 'unimatrix-patch':
    'NixOS/nixpkgs#264716'
  → 'NixOS/nixpkgs#264716'
• Updated input 'unison-nix':
    'github:ceedubs/unison-nix/8dd8294dff43b98d2cb46a3c5d99f42eae9ac74a' (2023-10-01)
  → 'github:ceedubs/unison-nix/378814b43702c78b8daca0e6f9444e73cb6c9955' (2023-11-15)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@h7x4 h7x4 h7x4 left review comments

@onemoresuza onemoresuza onemoresuza approved these changes

@pbsds pbsds pbsds approved these changes

Assignees
No one assigned
Labels
8.has: package (new) This PR adds a new package 10.rebuild-darwin: 1-10 10.rebuild-darwin: 1 10.rebuild-linux: 1-10 10.rebuild-linux: 1 11.by: package-maintainer This PR was created by the maintainer of the package it changes 12.approvals: 1 This PR was reviewed and approved by one reputable person
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@Anomalocaridid @nixos-discourse @onemoresuza @pbsds @h7x4 @delroth