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

mcfly: init at v0.3.1 #52940

Merged
merged 1 commit into from Feb 25, 2019
Merged

mcfly: init at v0.3.1 #52940

merged 1 commit into from Feb 25, 2019

Conversation

@Melkor333
Copy link
Contributor

Melkor333 commented Dec 26, 2018

Motivation for this change

mcfly is a very nice ctrl+r replacement that takes into account your working directory and the context of recently executed commands.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • 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 nox --run "nox-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)
  • Assured whether relevant documentation is up to date
  • Fits CONTRIBUTING.md.

# integration scripts are living.
echo $out/share/mcfly
SCRIPT
chmod +x $out/bin/mcfly-share

This comment has been minimized.

Copy link
@Infinisil

Infinisil Dec 29, 2018

Member

I don't see any point in this. Unless you need this there's no point in copying it from other packages.

This comment has been minimized.

Copy link
@Melkor333

Melkor333 Dec 30, 2018

Author Contributor

This is necessary. The comment is just a hint to know where I have it from. Mcfly and fzf are both ctrl+r replacements and thus a file needs to be sourced by bash to use them. This additional command shows the path to this bash script.

I can delete the comment if you think it is confusing.

This comment has been minimized.

Copy link
@Infinisil

Infinisil Dec 30, 2018

Member

I was just about to complain how you can just discover these files by just inspecting the derivations (tree $(nix-build '<nixpkgs>' -A fzf)), but then I found #27709 which introduced this. And it's in the wiki too.

I personally really dislike this approach. Using binaries to find paths is really weird. How I'd think it should work without such a -share binary is to have packages install these sourceable scripts into a standard location (say $out/share/bash/sourceables) and then do

for p in $NIX_PROFILES; do
  source "$p/share/bash/sourceables/fzf"
  source "$p/share/bash/sourceables/autojump"
done

This is also how bash sources its completion scripts:

for p in $NIX_PROFILES; do
for m in "$p/etc/bash_completion.d/"*; do
. $m
done
done

Alternatively (as just mentioned by @samueldr in #nixos-dev) I can imagine a nix-share binary usable like this (also requires standard locations for these files):

source $(nix-share fzf)
source $(nix-share autojump)

Maybe also combine these approaches and have a standard bash setup script that creates a function for the for p in $NIX_PROFILES thing.

This comment has been minimized.

Copy link
@Melkor333

Melkor333 Jan 1, 2019

Author Contributor

There is also an entry in the nixpkgs manual. And weirdly enough autojump doesn't have this script anymore, 47aceb0 removed it.

Maybe also combine these approaches and have a standard bash setup script that creates a function for the for p in $NIX_PROFILES thing.

Creating a shell setup script makes sense. I just don't get exactly what you mean with ..that creates a function.... What kind of function do you mean, what should it do?

What I could think of is a script called something like nix-source which looks similar to this autojump script, except it also takes a $package argument like nix-source fzf to know which sourceable it should load and goes through a for p in $NIX_PROFILES loop to find the file.

This comment has been minimized.

Copy link
@Infinisil

Infinisil Jan 23, 2019

Member

I think it's best to just remove that convenience script for now, we can always make it more convenient later (but you can never take something convenient away without people complaining).

This comment has been minimized.

Copy link
@Melkor333

Melkor333 Jan 29, 2019

Author Contributor

I've removed the part in question and as (ugly) workaround the following works for now:

source $(dirname $(which mcfly))/../share/mcfly/mcfly.bash
@Melkor333 Melkor333 force-pushed the Melkor333:mcfly-init branch from f4b13a9 to 3322479 Jan 27, 2019
@tilpner

This comment has been minimized.

Copy link
Member

tilpner commented Jan 27, 2019

@GrahamcOfBorg build mcfly
@GrahamcOfBorg eval

@grahamc

This comment has been minimized.

Copy link
Member

grahamc commented Jan 27, 2019

@Melkor333 Melkor333 force-pushed the Melkor333:mcfly-init branch from 3322479 to 1c03692 Feb 9, 2019
@Melkor333

This comment has been minimized.

Copy link
Contributor Author

Melkor333 commented Feb 25, 2019

@Infinisil is it possible for you to merge this PR? I removed the part in question and it should be OK to merge now.

@Infinisil

This comment has been minimized.

Copy link
Member

Infinisil commented Feb 25, 2019

Yeah, can you squash the commits first?

@Melkor333 Melkor333 force-pushed the Melkor333:mcfly-init branch from ec94104 to a75e25b Feb 25, 2019
@Infinisil Infinisil merged commit 22f9405 into NixOS:master Feb 25, 2019
10 checks passed
10 checks passed
grahamcofborg-eval ^.^!
Details
grahamcofborg-eval-check-maintainers matching changed paths to changed attrs...
Details
grahamcofborg-eval-check-meta config.nix: checkMeta = true
Details
grahamcofborg-eval-nixos-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A manual
Details
grahamcofborg-eval-nixos-options nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A options
Details
grahamcofborg-eval-nixpkgs-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A manual
Details
grahamcofborg-eval-nixpkgs-tarball nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A tarball
Details
grahamcofborg-eval-nixpkgs-unstable-jobset nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A unstable
Details
grahamcofborg-eval-package-list nix-env -qa --json --file .
Details
grahamcofborg-eval-package-list-no-aliases nix-env -qa --json --file . --arg config { allowAliases = false; }
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.