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

kakoune.cr: init at unstable-2021-04-30 #115011

Closed
wants to merge 1 commit into from

Conversation

loewenheim
Copy link
Contributor

Motivation for this change

I would like to have kakoune.cr in nixpkgs.

This is still a draft; it doesn't build for reasons that are a mystery to me. To be more concrete, when it gets to the building phase, this happens:

Resolving dependencies
Could not find commit a5df8025fd3ab182ae2511d525f708cbbe0ed43a for shard "fifo" in the repository https://github.com/alexherbo2/fifo.cr.git

I don't understand what "could not find commit" is supposed to mean. As far as I can tell it's set up exactly like crystal2nix, which builds without issue.

@alexherbo2 @manveru @peterhoeg

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.

Copy link
Member

@SuperSandro2000 SuperSandro2000 left a comment

Choose a reason for hiding this comment

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

I don't understand what "could not find commit" is supposed to mean. As far as I can tell it's set up exactly like crystal2nix, which builds without issue.

Does it try to access git information? They are not available by default. You can either patch it or add leaveDotGit = true; to the github fetcher.

pkgs/applications/editors/kakoune.cr/default.nix Outdated Show resolved Hide resolved
@loewenheim
Copy link
Contributor Author

Hm, at the very least I don't see where this might happen in crystal2nix. This is the entirety of its default.nix:

{ lib, fetchFromGitHub, fetchgit, crystal, makeWrapper, nix-prefetch-git }:

crystal.buildCrystalPackage rec {
  pname = "crystal2nix";
  version = "0.1.0";

  src = fetchFromGitHub {
    owner = "peterhoeg";
    repo = "crystal2nix";
    rev = "v${version}";
    sha256 = "sha256-K1ElG8VC/D0axmSRaufH3cE50xNQisAmFucDkV+5O0s=";
  };

  format = "shards";

  shardsFile = ./shards.nix;

  nativeBuildInputs = [ makeWrapper ];

  postInstall = ''
    wrapProgram $out/bin/crystal2nix \
      --prefix PATH : ${lib.makeBinPath [ nix-prefetch-git ]}
  '';

  # temporarily off. We need the checks to execute the wrapped binary
  doCheck = false;

  # it requires an internet connection when run
  doInstallCheck = false;

  meta = with lib; {
    description = "Utility to convert Crystal's shard.lock files to a Nix file";
    license = licenses.mit;
    maintainers = with maintainers; [ manveru peterhoeg ];
  };
}

@peterhoeg
Copy link
Member

peterhoeg commented Mar 4, 2021 via email

@loewenheim
Copy link
Contributor Author

Thank you!

@loewenheim
Copy link
Contributor Author

@peterhoeg any news?

@@ -25,16 +25,11 @@ crystal.buildCrystalPackage rec {
lockFile = ./shard.lock;

installPhase = ''
Copy link
Contributor

Choose a reason for hiding this comment

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

You should copy bin and share to $out.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@loewenheim It is important to copy the whole bin and share directories to $out, as kcr will look in share.

Do the various scripts in share still need to be linked into bin?

Copy link
Contributor

Choose a reason for hiding this comment

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

If you want nix-shell -p kakoune.cr and have the shipped commands in your path yes.

Otherwise, Nix users will need to enter the following commands:

kcr install commands
kcr install desktop

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The idea is definitely that you don't have to run these commands after installing with nix.

@alexherbo2
Copy link
Contributor

@loewenheim It is important to copy the whole bin and share directories to $out, as kcr will look in share.

cp $out/share/kcr/commands/fzf/kcr-fzf-* $out/bin
mkdir -p $out/bin
cp bin/kcr $out/bin
cp share/kcr/commands/edit/kcr-edit-search $out/bin
Copy link
Contributor

Choose a reason for hiding this comment

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

You can replace with cp share/kcr/commands/*/kcr-* $out/bin.

src = fetchFromGitHub {
repo = "kakoune.cr";
owner = "alexherbo2";
rev = "c1e0807e3334f9f2e7d61e22fa1fe38c208cc6a4";
Copy link
Contributor

Choose a reason for hiding this comment

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

I updated again. :(

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No problem :)

@peterhoeg
Copy link
Member

@peterhoeg any news?

#116981

@peterhoeg
Copy link
Member

I suggest you use format = "shards"; and then do the copying in the postInstall hook. You can use install instead of first creating a directory and then cping the file afterwards. Please also use a recent shards that supports the v2 format (crystal2nix now handles this correctly in v0.1.1).

@loewenheim
Copy link
Contributor Author

@pteterhoeg What do you mean by “use install instead of first creating a directory and then cping the file”?

@peterhoeg
Copy link
Member

Instead of doing this:

mkdir -p $out/share/icons/hicolor/scalable/apps
cp $icon $out/share/icons/hicolor/scalable/apps/kakoune.svg

mkdir -p $out/share/applications
cp share/kcr/applications/kakoune.desktop $out/share/applications

You can do:

install -Dm444 $icon $out/share/icons/hicolor/scalable/apps/${icon.name}
install -Dm444 t $out/share/applications share/kcr/applications/kakoune.desktop 

It's just less code and easier to read.

@loewenheim
Copy link
Contributor Author

Instead of doing this:

mkdir -p $out/share/icons/hicolor/scalable/apps
cp $icon $out/share/icons/hicolor/scalable/apps/kakoune.svg

mkdir -p $out/share/applications
cp share/kcr/applications/kakoune.desktop $out/share/applications

You can do:

install -Dm444 $icon $out/share/icons/hicolor/scalable/apps/${icon.name}
install -Dm444 t $out/share/applications share/kcr/applications/kakoune.desktop 

It's just less code and easier to read.

TIL. Thank you very much for the explanation.

@SuperSandro2000
Copy link
Member

Please squash the commits when appending new review feedback.

@alexherbo2
Copy link
Contributor

@loewenheim bin/kcr should be able to find ../share/kcr.

@loewenheim
Copy link
Contributor Author

Building on my machine fails with

In src/version.cr:3:8

 3 | {{ `git describe --tags --always`.chomp.stringify }}
        ^
Error: error executing command: git describe --tags --always, got exit status 128

error: builder for '/nix/store/aszwsjwcvnpglh60379dn8s85kwxyaxy-kakoune-cr-0.1.0.drv' failed with exit code 1;
       last 10 log lines:
       > Error target kcr failed to compile:
       > fatal: not a git repository (or any of the parent directories): .git
       > Showing last frame. Use --error-trace for full trace.
       >
       > In src/version.cr:3:8
       >
       >  3 | {{ `git describe --tags --always`.chomp.stringify }}
       >         ^
       > Error: error executing command: git describe --tags --always, got exit status 128
       >
       For full logs, run 'nix log /nix/store/aszwsjwcvnpglh60379dn8s85kwxyaxy-kakoune-cr-0.1.0.drv'.

i suppose the problem is the not a git repository: .git part, but I don't know what that's supposed to mean.

@SuperSandro2000
Copy link
Member

i suppose the problem is the not a git repository: .git part, but I don't know what that's supposed to mean.

Nix strips the .git directory to make the tarballs reproducible. Can you overwrite the version detection with the version number? Maybe we need to patch this so that the version number is hardcoded or read from an environment variable. Last option would be to leave the git directory in the download.

@peterhoeg
Copy link
Member

peterhoeg commented Apr 8, 2021 via email

Copy link
Contributor

@fufexan fufexan left a comment

Choose a reason for hiding this comment

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

I got it building with these changes. It doesn't work as expected, but it's closer to what we need (hopefully).
For example, running kcr-fzf gives:

/home/mihai/.nix-profile/bin/kcr-fzf: line 3: kcr-fzf-: command not found

and kcr-fzf-buffers complains it can't find fzf.

pkgs/applications/editors/kakoune.cr/default.nix Outdated Show resolved Hide resolved
pkgs/applications/editors/kakoune.cr/default.nix Outdated Show resolved Hide resolved
pkgs/applications/editors/kakoune.cr/default.nix Outdated Show resolved Hide resolved
pkgs/applications/editors/kakoune.cr/default.nix Outdated Show resolved Hide resolved
@fufexan
Copy link
Contributor

fufexan commented May 9, 2021

This concludes where I've gotten with it. It works fine, tested with auto-pairs.kak and standalone.
@loewenheim can you apply the above suggestions?

@loewenheim loewenheim changed the title kakoune.cr: init at 0.1.0 kakoune.cr: init at unstable-2021-04-30 May 10, 2021
@loewenheim
Copy link
Contributor Author

I applied your suggested changes. Sorry for the slow response, I've been rather busy.

@SuperSandro2000
Copy link
Member

This is a semi-automatic executed nixpkgs-review with nixpkgs-review-checks extension. It is checked by a human on a best effort basis and does not build all packages (e.g. lumo, tensorflow or pytorch).
If you have any questions or problems please reach out to SuperSandro2000 on IRC.

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

1 package failed to build and are new build failure:

@fufexan
Copy link
Contributor

fufexan commented Jun 1, 2021

It seems like adding kakoune to the buildInputs doesn't fix the tests failing. I suggest removing it and also disabling tests. I've been using this package as an overlay for a month and had no problems.

@loewenheim
Copy link
Contributor Author

What do you mean by "disabling tests"?

@fufexan
Copy link
Contributor

fufexan commented Jun 2, 2021

doInstallCheck = false;

@stale
Copy link

stale bot commented Jan 9, 2022

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 Jan 9, 2022
@peterhoeg
Copy link
Member

@loewenheim - I appreciate that this has been a bit of a struggle, but are you still interested in getting this in?

@stale stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Mar 1, 2022
@loewenheim
Copy link
Contributor Author

Interest yes, but I'm afraid I don't have the time right now :/

@malte-v
Copy link
Contributor

malte-v commented Mar 2, 2022

Updated PR here: #162522

@peterhoeg
Copy link
Member

Closing as the linked PR supersedes this.

@peterhoeg peterhoeg closed this Mar 3, 2022
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

6 participants