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

Add iim #91657

Closed
wants to merge 2 commits into from
Closed

Add iim #91657

wants to merge 2 commits into from

Conversation

monokrome
Copy link

Motivation for this change

I was trying to install iim and noticed that it wasn't in the package index, so decided that I'd add a package for it.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@stale
Copy link

stale bot commented Dec 24, 2020

I marked this as stale due to inactivity. → More info

@stale stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Dec 24, 2020
@stale stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Feb 24, 2021
@tomberek
Copy link
Contributor

@monokrome : cleaned up the derivation, does this meet your intent?

@monokrome
Copy link
Author

@tomberek Yeah, sorry - it was my first package and I didn't know how to access the things lol. Thank you 🙏🏼

@monokrome
Copy link
Author

monokrome commented Mar 2, 2021

What does the "This PR does not cleanly list package outputs after merging." imply? Is there a new function for listing them or something?

@monokrome
Copy link
Author

Also, this isn't meant to be a draft... I don't know how that happened...

@monokrome monokrome marked this pull request as ready for review March 2, 2021 02:29
Copy link
Contributor

@tomberek tomberek left a comment

Choose a reason for hiding this comment

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

Fixed up the fetchFromGitHub, this does a more efficient fetching. Do you want to add yourself as a maintainer?

stdenv.mkDerivation {
name = "iim";

src = builtins.fetchGit {
Copy link
Contributor

Choose a reason for hiding this comment

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

my mistake, this should be fetchFromGithub (and added to the inputs at the top

Copy link
Author

Choose a reason for hiding this comment

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

@tomberek What does "added to the inputs at the top" mean here? Apologies for my confusion

Copy link
Member

Choose a reason for hiding this comment

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

You should use fetchFromGitHub which...

@monokrome
Copy link
Author

@tomberek Should I add myself as maintainer? Is that the normal thing to do?

@tomberek
Copy link
Contributor

tomberek commented Mar 2, 2021

@tomberek Should I add myself as maintainer? Is that the normal thing to do?

Yes. Please make it an independent commit before the current one(same PR is fine).

meta = with lib; {
homepage = "https://github.com/c00kiemon5ter/iim";
license = licenses.mit;
description = "iim is as a complete rewrite of the original ii from suckless.org";
Copy link
Member

Choose a reason for hiding this comment

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

Adding yourself as a maintainer is required.


buildPhase = "make iim CFLAGS='-D_GNU_SOURCE -std=c99 -Os -Wall -Wextra -pedantic'";

installPhase = "install -D iim $out/bin/iim";
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
installPhase = "install -D iim $out/bin/iim";
installPhase = ''
install -D iim $out/bin/iim
'';

rev = "220958e4e00156cbf67bf3cb4d60af25094f5428";
};

buildPhase = "make iim CFLAGS='-D_GNU_SOURCE -std=c99 -Os -Wall -Wextra -pedantic'";
Copy link
Member

Choose a reason for hiding this comment

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

Please use makeFlags.

stdenv.mkDerivation {
name = "iim";

src = builtins.fetchGit {
Copy link
Member

Choose a reason for hiding this comment

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

You should use fetchFromGitHub which...

Comment on lines +1 to +2
{ lib, stdenv }:
stdenv.mkDerivation {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
{ lib, stdenv }:
stdenv.mkDerivation {
{ lib, stdenv, fetchFromGitHub }:
stdenv.mkDerivation {

.. would be added here.

@@ -0,0 +1,20 @@
{ lib, stdenv }:
stdenv.mkDerivation {
name = "iim";
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
name = "iim";
pname = "iim";
version ="?";

@monokrome monokrome closed this Mar 23, 2021
@monokrome
Copy link
Author

Deleting this because multiple reviewers is annoying and @SuperSandro2000 doesn't know how to write complete sentences

@SuperSandro2000
Copy link
Member

Deleting this because multiple reviewers is annoying and @SuperSandro2000 doesn't know how to write complete sentences

It is totally normal to get multiple reviewers if one does not have enough time to take another look at the PR. Normally there are also no contradicting requests/changes which shouldn't make this a problem.

My sentence did start at the one comment and continued over the three dots ... into the other comment. I thought this was clear.

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