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

lookPath should accept an optional path override #3141

Closed
arran4 opened this issue Aug 2, 2023 · 9 comments · Fixed by #3260 · May be fixed by arran4/chezmoi#4
Closed

lookPath should accept an optional path override #3141

arran4 opened this issue Aug 2, 2023 · 9 comments · Fixed by #3260 · May be fixed by arran4/chezmoi#4
Labels
enhancement New feature or request

Comments

@arran4
Copy link
Contributor

arran4 commented Aug 2, 2023

Is your feature request related to a problem? Please describe.

As part of my dotfiles script I'm building up a list of paths that will replace the existing paths as a result I want not just the "current state" of paths, but if something is found in the paths that I'm building up. One way to do this would be to provide an optional argument to lookPath where I could specify the value of $PATH that I'm building up

Describe the solution you'd like

{{ if lookPath "diff-so-fancy" }}
# diff-so-fancy is in $PATH
{{ end }}

Would work along with:

{{ if lookPath "diff-so-fancy" data.futurepath }}
# diff-so-fancy is in future $PATH
{{ end }}

Describe alternatives you've considered

I guess I could loop through it and use stat to build up a map of what's in there for a quick lookup later. But it seems slower.

@arran4 arran4 added the enhancement New feature or request label Aug 2, 2023
@twpayne
Copy link
Owner

twpayne commented Aug 2, 2023

Two threads, maybe not solving this issue:

  1. Can template logic work? For example, here's how I add paths.
  2. chezmoi caches path look-ups. Maybe looking up a path is not expensive?

@arran4
Copy link
Contributor Author

arran4 commented Aug 2, 2023

I am doing something similar to:
https://github.com/twpayne/dotfiles/blob/f25b14407ec63b79b8c84a3a3159431fc4c02c24/home/dot_zshrc.tmpl#L9-L19

That's exactly what I imagined it to look like:

func LookPath(file string) (string, error) {
lookPathCacheMutex.Lock()
defer lookPathCacheMutex.Unlock()
if path, ok := lookPathCache[file]; ok {
return path, nil
}
path, err := exec.LookPath(file)
if err == nil {
lookPathCache[file] = path
}
return path, err

My issue is one step further; I would like to pick tools based on what's now available in paths. For instance installing on Solaris often puts tools in /opt/ which wouldn't be picked up with the current lookPath once I have modified path..

So:


{{- $paths := list }}
{{- $homeDir := .chezmoi.homeDir }}
{{- range $_, $relPath := list "bin" "go/bin" ".cargo/bin" ".local/bin" }}
{{    $path := joinPath $homeDir $relPath }}
{{-   if stat $path }}
{{-     $paths = mustAppend $paths $path }}
{{-   end }}
{{- end }}
{{- if $paths }}
export PATH={{ toStrings $paths | join ":" }}:$PATH
{{- end }}

{{ if lookPath "diff-so-fancy" $paths }}
# diff-so-fancy is in future $PATH
export DIFFTOOL=diff-so-fancy
{{ end }}

Is more to the goal.

So perhaps:

package chezmoi

import (
        "filepath"
        "os" 
	"os/exec"
	"sync"
)

var (
	lookPathCacheMutex sync.Mutex
	lookPathCache      = make(map[string]string)
	foundExecutableCacheMutex sync.Mutex
	foundExecutableCache      = make(map[string]struct{})
)

// LookPath is like os/exec.LookPath except that the first positive result is
// cached.
func LookPath(file string) (string, error) {
	lookPathCacheMutex.Lock()
	defer lookPathCacheMutex.Unlock()

	if path, ok := lookPathCache[file]; ok {
		return path, nil
	}

	path, err := exec.LookPath(file)
	if err == nil {
		lookPathCache[file] = path
	}

	return path, err
}

func LookPathIn(file string, paths string) (string, error) {

	foundExecutableCacheMutex.Lock()
	defer foundExecutableCacheMutex.Unlock()

	// stolen from: /usr/lib/go-1.20/src/os/exec/lp_unix.go:52
	for _, dir := range filepath.SplitList(paths) {
		if dir == "" {
                       continue
		}
		path := filepath.Join(dir, file)
		if err := findExecutable(path); err == nil {
			if !filepath.IsAbs(path) && execerrdot.Value() != "0" {
				return path, &Error{file, os.ErrDot}
			}
			return path, nil
		}
	}

	return path, err
}

// stolen from: /usr/lib/go-1.20/src/os/exec/lp_unix.go:52
func findExecutable(file string) error {

	if path, ok := foundExecutableCache[file]; ok {
		return file, nil
	}
	d, err := os.Stat(file)
	if err != nil {
		return err
	}
	m := d.Mode()
	if m.IsDir() {
		return syscall.EISDIR
	}
	err = unix.Eaccess(file, unix.X_OK)
	// ENOSYS means Eaccess is not available or not implemented.
	// EPERM can be returned by Linux containers employing seccomp.
	// In both cases, fall back to checking the permission bits.
	if err == nil || (err != syscall.ENOSYS && err != syscall.EPERM) {
		return err
	}
	if m&0111 != 0 {
		return nil
	}
	return fs.ErrPermission
}

Sorry I did this in the comments there are probably major issues with this example

@halostatue
Copy link
Collaborator

I think that, as a separate function, this could be quite useful. Without this, I would need to do something like:

$ # run the commands to install bun, which installs in ~/.bun/bin/bun
$ chezmoi apply
# things apply without knowing that bun is present
$ exec $SHELL
# my shell config has bun file detection, and this is a fast way to replace the current shell with an updated shell
$ chezmoi apply
# things apply knowing that bun is present

With this, it would be more like:

$ # run the commands to install bun
$ chezmoi apply
# things apply knowing that bun will be present on next shell start
$ exec $SHELL

@twpayne
Copy link
Owner

twpayne commented Aug 2, 2023

@arran4, thanks, this makes sense :)

Would you mind converting your comment in to a pull request?

https://github.com/twpayne/chezmoi/pull/2833/files should give you a complete picture of which files need to be modified when adding a new template function.

arran4 added a commit to arran4/chezmoi that referenced this issue Aug 3, 2023
arran4 added a commit to arran4/chezmoi that referenced this issue Aug 3, 2023
BREAKING CHANGE: `isExecutable` has major change for windows. It now does something
@arran4 arran4 mentioned this issue Aug 3, 2023
@arran4
Copy link
Contributor Author

arran4 commented Aug 3, 2023

@twpayne Done: #3148 It was a bit more extensive than I expected.

arran4 added a commit to arran4/chezmoi that referenced this issue Aug 3, 2023
BREAKING CHANGE: `isExecutable` has major change for windows. It now does something
arran4 added a commit to arran4/chezmoi that referenced this issue Aug 3, 2023
BREAKING CHANGE: `isExecutable` has major change for windows. It now does something
arran4 added a commit to arran4/chezmoi that referenced this issue Aug 4, 2023
BREAKING CHANGE: `isExecutable` has major change for windows. It now does something
@twpayne
Copy link
Owner

twpayne commented Aug 6, 2023

Fixed by #3157 (and also #3152, #3153, and #3156 on the way).

@twpayne twpayne closed this as completed Aug 6, 2023
@arran4
Copy link
Contributor Author

arran4 commented Aug 6, 2023

@twpayne Should I continue with this?

@twpayne
Copy link
Owner

twpayne commented Aug 6, 2023

If you need the functionality, then please do (and re-open this issue). I think it's possible to do within the current templating functionality, something like (warning: untested):

{{ $haveEcho := false }}
{{ $paths := list "/my/path1" "/my/path2" }}
{{ range $_, $path := $paths }}
{{   if (joinPath $path "echo" | isExecutable) }}
{{     $haveEcho = true }}
{{   end }}
{{ end }}
{{ if $haveEcho }}
# echo already found in path
{{ end }}

@arran4
Copy link
Contributor Author

arran4 commented Aug 6, 2023

It's more of a want, however it would also take time for me to adapt the changes into my disorganized dotfiles. I think the push back warrants a different name at least.

arran4 added a commit to arran4/chezmoi that referenced this issue Aug 8, 2023
…ecutable of a particular name and returns the found path.

As per: twpayne#3141
arran4 added a commit to arran4/chezmoi that referenced this issue Aug 8, 2023
…ecutable of a particular name and returns the found path.

As per: twpayne#3141
arran4 added a commit to arran4/chezmoi that referenced this issue Aug 8, 2023
…ecutable of a particular name and returns the found path.

As per: twpayne#3141
arran4 added a commit to arran4/chezmoi that referenced this issue Aug 8, 2023
…ecutable of a particular name and returns the found path.

As per: twpayne#3141
arran4 added a commit to arran4/chezmoi that referenced this issue Aug 8, 2023
…ecutable of a particular name and returns the found path.

As per: twpayne#3141
arran4 added a commit to arran4/chezmoi that referenced this issue Aug 8, 2023
…ecutable of a particular name and returns the found path.

As per: twpayne#3141
arran4 added a commit to arran4/chezmoi that referenced this issue Aug 8, 2023
…ecutable of a particular name and returns the found path.

As per: twpayne#3141
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 14, 2023
halostatue pushed a commit to halostatue/chezmoi that referenced this issue Sep 21, 2023
Implement `findExecutable` with strong caching. The main change from
twpayne#3162 is that it now also accepts either a `file` or `file-list` for
searching, so that one of several different options can be searched
simultaneously for roughly equivalent implementations. See twpayne#3256 for the
inspiration for this particular change.

We *can* switch this to two template functions, but I think that the
core implementation would best remain the way that it is.

Resolves: twpayne#3141
Closes: twpayne#3162
Co-authored-by: Arran Ubels <arran4@gmail.com>
halostatue pushed a commit to halostatue/chezmoi that referenced this issue Sep 21, 2023
Implement `findExecutable` with strong caching. The main change from
twpayne#3162 is that it now also accepts either a `file` or `file-list` for
searching, so that one of several different options can be searched
simultaneously for roughly equivalent implementations. See twpayne#3256 for the
inspiration for this particular change.

We *can* switch this to two template functions, but I think that the
core implementation would best remain the way that it is.

Resolves: twpayne#3141
Closes: twpayne#3162
Co-authored-by: Arran Ubels <arran4@gmail.com>
halostatue pushed a commit to halostatue/chezmoi that referenced this issue Sep 21, 2023
Implement `findExecutable` with strong caching. The main change from
twpayne#3162 is that it now also accepts either a `file` or `file-list` for
searching, so that one of several different options can be searched
simultaneously for roughly equivalent implementations. See twpayne#3256 for the
inspiration for this particular change.

We *can* switch this to two template functions, but I think that the
core implementation would best remain the way that it is.

Resolves: twpayne#3141
Closes: twpayne#3162
Co-authored-by: Arran Ubels <arran4@gmail.com>
halostatue pushed a commit to halostatue/chezmoi that referenced this issue Sep 22, 2023
Implemented `findExecutable` with strong caching, extended from twpayne#3162.
Also implemented `findOneExecutable` to search for any one executable
from a list.

Resolves: twpayne#3141
Closes: twpayne#3162
Co-authored-by: Arran Ubels <arran4@gmail.com>
halostatue pushed a commit to halostatue/chezmoi that referenced this issue Sep 22, 2023
Implemented `findExecutable` with strong caching, extended from twpayne#3162.
Also implemented `findOneExecutable` to search for any one executable
from a list.

Resolves: twpayne#3141
Closes: twpayne#3162
Co-authored-by: Arran Ubels <arran4@gmail.com>
halostatue pushed a commit that referenced this issue Sep 22, 2023
Implemented `findExecutable` with strong caching, extended from #3162.
Also implemented `findOneExecutable` to search for any one executable
from a list.

Resolves: #3141
Closes: #3162
Co-authored-by: Arran Ubels <arran4@gmail.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.