nmattia / niv

Easy dependency management for Nix projects
https://github.com/nmattia/niv
MIT License
1.62k stars 79 forks source link

niv local <pkg> [--path /path/to/file] #31

Closed nmattia closed 3 years ago

nmattia commented 5 years ago

I'd like to implement a simple way to switch between GitHub packages and local checkouts. Ideally it'd be as simple as:

$ niv local my-package

which would toggle between local checkout and upstream URL. I think the nicest would be for niv to check out the repo (using the sources.json's owner and repo fields) in a temporary directory and printing out the path. If --path is set, use that path (and skip the checkout). Finally if local has ever be used on a package, just reuse the specified path.

@erictapen would this solve the issue you mentioned here https://github.com/nmattia/niv/issues/19#issuecomment-469910993 ?

erictapen commented 5 years ago

Well I don't see the appeal of having a dedicated subcommand for this, but yes, that would solve my problem.

domenkozar commented 5 years ago

@roberth has implement a home-grown version of this using git work trees and clones to a known location.

roberth commented 5 years ago

It's based on git worktrees, so it's super fast and it shares branches and config across checkouts. It's probably a less often used feature of git, but it seems to be far better integrated than (dare I bring this up?) submodules.

Let me share our implementation, because perhaps it's useful to you. It's a bash prototype, so let's start with the nix part.

sources.nix

# A record, from name to path, of the third-party packages
let

  bootPkgs = import sourcesNoOverrides.nixpkgs {};
  inherit (bootPkgs.callPackage sourcesNoOverrides.nix-gitignore {}) gitignoreRecursiveSource;

  versions = builtins.fromJSON (builtins.readFile ./sources.json);
  sourcesDir =  ../sources;
  sourcesDirNodes =
    if builtins.pathExists ../sources
      then builtins.readDir ../sources // {
             # Ignore checkout of nix-gitignore because of cyclic dependency
             nix-gitignore = null;
           }
      else {};

  # fetchTarball version that is compatible between all the versions of Nix
  fetchTarball =
    { url, sha256 }:
      if builtins.lessThan builtins.nixVersion "1.12" then
        builtins.fetchTarball { inherit url; }
      else
        builtins.fetchTarball { inherit url sha256; };

  loadSpec = spec:
    if builtins.hasAttr "url" spec && builtins.hasAttr "sha256" spec
    then
      spec //
    { outPath = fetchTarball { inherit (spec) url sha256; }; }
    else spec;

  loadDir = name: spec:
    spec //
    { outPath = gitignoreRecursiveSource "" (sourcesDir + "/${name}"); };

  # NOTE: spec must _not_ have an "outPath" attribute
  sources = builtins.mapAttrs (name: spec:
      if builtins.hasAttr "outPath" spec
      then abort
        "The values in versions.json must not have an 'outPath' attribute"
      else
        if sourcesDirNodes."${name}" or null == null
        then loadSpec spec
        else loadDir name spec
    ) versions;

  sourcesNoOverrides = builtins.mapAttrs (name: spec:
      if builtins.hasAttr "outPath" spec
      then abort
        "The values in versions.json must not have an 'outPath' attribute"
      else
        loadSpec spec
    ) versions;

in sources

And to perform the checkout, we have a script called open-source. Please don't use that name, thank you :)

open-source -> niv open

#!/usr/bin/env bash
set -eu -o pipefail

PATH="$(nix-build --no-out-link -A devTools.jq)/bin:$PATH"

sourceName="$1"

echo Opening source $sourceName

DOTGIT_STORAGE_DIR=~/git
NIX_SOURCES_JSON=nix/sources.json

owner="$(jq -r <"$NIX_SOURCES_JSON" '.[$name].owner' --arg name "$sourceName")"
repo="$(jq -r <"$NIX_SOURCES_JSON" '.[$name].repo' --arg name "$sourceName")"
rev="$(jq -r <"$NIX_SOURCES_JSON" '.[$name].rev' --arg name "$sourceName")"
branch="$(jq -r <"$NIX_SOURCES_JSON" '.[$name].branch' --arg name "$sourceName")"
# TODO: map url_template to service type and use that when mapping to local directory

# TODO: follow forks (which also breaks the assumption of a single "origin" remote)

dir="$DOTGIT_STORAGE_DIR/$owner/$repo"

if ! test -d "$dir"; then
    mkdir -p "$dir"
    git clone --bare "git@github.com:$owner/$repo.git" "$dir"
else
    (cd "$dir"; git fetch --all)
fi

targetDir="$PWD/sources/$sourceName"

if test -d "$targetDir"; then
  # Update (checkout)
  ( cd "$targetDir"; git checkout "$rev" )
else
  # Open (worktree add)
  ( cd "$dir";
    if git worktree list --porcelain grep "worktree $targetDir"; then
      git worktree remove "$targetDir"
    fi
    git worktree add "$targetDir" "$rev"
  )
fi
roberth commented 5 years ago

niv update should attempt to checkout the intended version, because it turns out having a local worktree is easy to forget. It's the kind of mistake that takes a while to figure out, but the good news is it's preventable in niv update.

roberth commented 5 years ago

Some tools (among which VSCode and Atom) don't really like nested git repo's behind gitignore. It doesn't break that bad, but they will be greyed out and not available in search.

We chose to keep the worktrees in the project so they are clearly local to the project and that no accidental sharing of edits will occur. Even worse, keeping the worktrees in a user-global namespace will not work because we need simultaneous checkouts of multiple versions.

Some location schemes:

  1. $project/sources/$dep
    • our current scheme
    • exotic .gitignore use case doesn't work well with tools
  2. $project-source-$dep (= $project/../$(basename $project)-source-$dep)
    • clutters directory of projects
    • assumes $project/.. is not in a git repo
      • no nested niv open either
    • $project/.. is still a self-contained workspace
  3. ~/worktrees/$(dirname $project)-$dep
    • still collides for instances of a project with same name
    • harder to discover, navigate, no autojump either

(2) seems like the best candidate

roberth commented 5 years ago

Use of git hooks seems to be a prerequisite to using this without a lot of human error.

michaelpj commented 5 years ago

Here's a trick we use at work for something like this:

  # iohk-nix can be overridden for debugging purposes by setting
  # NIX_PATH=iohk-nix=/path/to/iohk-nix
  iohk-nix = import (
    let try = builtins.tryEval <iohk-nix>;
    in if try.success
    then builtins.trace "using host <iohk-nix>" try.value
    else
      let
        spec = builtins.fromJSON (builtins.readFile ./iohk-nix.json);
      in builtins.fetchTarball {
        url = "${spec.url}/archive/${spec.rev}.tar.gz";
        inherit (spec) sha256;
      }) { inherit config system; };

What this does is that it tries to get the dependency iohk_nix from NIX_PATH, and if that fails it imports it via the JSON spec. In practice, this means you can override a source by adding something to NIX_PATH on an ad-hoc basis. It would be relatively easy to just generate this for every dependency based on its name, I think.

This is probably useful in slightly different circumstances: it's nice when you're testing your changes to a dep before opening a PR, for example, but you can't check in the change if you want to persist it.

roberth commented 5 years ago

@michaelpj I think your snippet might benefit from https://github.com/hercules-ci/gitignore around try.value.

What I like about your solution is that it is easy to make generic for all sources with builtins.nixPath. Some differences:

I suppose these solutions can coexist for different scenarios. @michaelpj's solution is certainly easier to implement.

michaelpj commented 5 years ago

NIX_PATH applies to all sources, also nested sources which happen to have the same name

Yes, this is pretty scary! I don't have a good suggestion for doing this other than somehow mangling the name based on the project or something.

I think I would personally find it equally easy if there was a single command that just switched the local dep, though.

domenkozar commented 5 years ago

One of the things that @roberth solution gives up is that sources.json is not single source of truth anymore, so I'd suggest whatever is implement also denotes local package usage as part of sources.json.

roberth commented 5 years ago

:+1:, without destroying the original value, preferably. This can be done by adding a boolean attribute to the json, like useLocalWorktree. I guess you might as well put the path to the tree there instead of a boolean. We probably won't implement it in our poc/hack, but for niv it should be easy.

jbgi commented 5 years ago

FYI, I have added what @michaelpj described in this fork (so that we can switch easily to niv without breaking to much users workflow): https://github.com/input-output-hk/niv/commit/209b7a1ef671ea265efcdbd8d1a1468ccb86603a (still need to add hercules-ci/gitignore)

michaelpj commented 5 years ago

@jbgi TBH I think that style of overriding is just too dangerous. Once you start getting two projects A and B both using niv where A depends on B there's a serious risk of accidentally overriding B's dependencies as well when you try and override A's. Unless you adopt some policy of naming all your deps <project-name>-<dep-name>.

nmattia commented 4 years ago

I'm gonna split this in two. First, add a type "local" which contains a path. Then I'll experiment with the UX of checking out a dependency.

nomeata commented 4 years ago

I am not quite sure I like the UX provided by

> $ niv local my-package

if it changes sources.json.

My use-case is: I want to test a change to some other, niv’ed dependency locally. So I only want to temporarily point to a local path. I think do not want to change niv/sources.json, which after all is shared with co-workers, and where I don't want to lose the settings there.

I have independently of @michaelpj implemented something based on environment variables (not using NIXPATH, but simply FOO_SRC for dependency foo, https://github.com/nmattia/niv/issues/197#issuecomment-603153526). Doing it like this nix-wide would of course share the problem discussed above, i.e. that it would affect nested uses of niv

Maybe there is a place for sources.local.json that would be affected by niv local, override settings in sources.json, but (usually) never be committed?

michaelpj commented 4 years ago

Maybe there is a place for sources.local.json that would be affected by niv local, override settings in sources.json, but (usually) never be committed?

I actually really like this idea. At work we already have a sourcesOverride value that we use for overriding niv sources in Nix code, but having it come from another sources.json is... somewhat obvious in retrospect.

I think this solves quite a few of the issues above:

That said, I do think that https://github.com/nmattia/niv/issues/219 is independently useful. If nothing else, you'd probably use it in sources.local.json!

domenkozar commented 4 years ago

The big downside of separate file is that it's easy to get confused. Looking at sources.json one would expect to get a source as it's defined there, forgetting about sources.local.json.

@nomeata If you'd like to temporarily test something locally, why is having that in the git diff so it can be removed or put into a branch a problem?

nomeata commented 4 years ago

@nomeata If you'd like to temporarily test something locally, why is having that in the git diff so it can be removed or put into a branch a problem?

Not a huge problem; it just feels dirty (heh) to edit a checked-in file when I don’t want to publish that. It’s the same princpile as cabal.project.local, or .git/info/exclude etc.

michaelpj commented 4 years ago

I agree: adding x.local files that override the behaviour of x is pretty common. It can definitely be confusing (I've wasted quite a bit of time due to forgetting about cabal.project.local...), but I think the advantages outweigh the disadvantages.

nmattia commented 4 years ago

Any brave souls willing to try out https://github.com/nmattia/niv/pull/220 ?

symphorien commented 4 years ago

I tried it:

srid commented 4 years ago

You may want to checkout obelisk's ob thunk unpack ... for some ideas ...

andir commented 3 years ago

Given that #220 exists we can close this. It might not be perfect but perhaps we can have a more targeted discussion about how to improve it instead.

adrian-gierakowski commented 3 years ago

@andir I don’t think closing this issue is the best course of action. As @nmattia mentioned here, #220 was merely the first step towards resolving this issue. And the comments on this issue contain quite a bit of useful context.