LookPath like impelementations for OS specific executable searches #1131

arixmkii · 2022-08-25T14:26:28Z

Additionally tries exec.LookPath over Absolute path, when searching in predefined directories. exec.LookPath could handle absolute paths - verified on Windows and macOS.

On Windows was tested with my.bat file in C:\Temp, which is obviously not referenced by PATH variable.

Test program (my.go):

package main

import (
        "fmt"
        "os/exec"
)

func main() {
        lp, err := exec.LookPath("C:\\Temp\\my")
        fmt.Printf("R: %q, E: %v", lp, err)
}

Run as go run my.go and outputs.

R: "C:\\Temp\\my.bat", E: <nil>

Signed-off-by: Arthur Sengileyev [email protected]

arixmkii · 2022-08-25T14:33:04Z

Error: undeclared name: `lookPathExplicit` (typecheck)

I don't really understand this. Probably unix + windows implementations are not enough and it needs _unsupported fallback as well. 🤔

pkg/config/lpe_unix.go

pkg/config/lpe_windows.go

pkg/config/lpe_unix.go

rhatdan · 2022-09-02T11:24:52Z

LGTM
@containers/podman-maintainers PTAL

giuseppe · 2022-09-02T13:01:12Z

could we directly use LookPath from os/exec? What would be different?

TomSweeneyRedHat · 2022-09-02T15:45:38Z

The changes LGTM, but would like to hear the answer to @giuseppe 's question before merge.

Luap99

I don't understand why this adds so much complexity? It should be enough to check for the windows extensions inside the dirList loop in FindHelperBinary. Just add a small helper function which does the check on windows and make it a noop on unix.

pkg/config/lpe_unix.go

arixmkii · 2022-09-06T11:02:18Z

I don't understand why this adds so much complexity? It should be enough to check for the windows extensions inside the dirList loop in FindHelperBinary. Just add a small helper function which does the check on windows and make it a noop on unix.

Just for consistency with how LookPath works - I considered this as the most complete reference.

could we directly use LookPath from os/exec? What would be different?

I would like to do so. But language package doesn't provide this. LookPath only provides searching in environment defined PATH, but here we are trying to lookup a binary in specific non system provided path. For me it looks like this should be a part of os/exec and then LookPath (like it is now) should be implemented by the means of this function just provided to it the environment variable expanded. I'm open trying to make it a suggestion for Go lang, but this looks like a whole level more complex process, though this still looks to me as correct path to take.

This is not a blocker for any known workload (workarounds are mentioned in issue linked). This was just to make things more consistent (subject to personal opinion). The added amount of code probably doesn't justify that improved consistency.

I'm open for discarding/archiving this PR, keeping the original issue open for reference purposes and trying getting the support at os/exec level.

mtrmac

removed copyright for now, they have BSD attribution header, I don't know what is the right way to preserve it the right way

Just so that it isn’t forgotten, this is a blocker.

mtrmac · 2022-09-13T17:55:15Z

I don't understand why this adds so much complexity? It should be enough to check for the windows extensions inside the dirList loop in FindHelperBinary. Just add a small helper function which does the check on windows and make it a noop on unix.

I agree with this. (Or not even a function, just a global variable of suffixes to iterate over, set to {""} on Unix.)

It would be nice not to sign up to the perpetual responsibility to keep up with the upstream LookPath changes.

And doing the simple thing, although not entirely correct about things like PATHEXT — or about those future LookPath changes — would avoid the need to worry about licenses, which is a pretty large benefit.

removed copyright for now, they have BSD attribution header, I don't know what is the right way to preserve it the right way

Just so that it isn’t forgotten, this is a blocker.

Also, please read https://github.com/containers/common/blob/main/CONTRIBUTING.md#sign-your-prs . The DCO is a serious matter.

arixmkii · 2022-09-14T07:21:25Z

Just so that it isn’t forgotten, this is a blocker.

@mtrmac Could you give me the heads ups on how one could use BSD sources in APL2 project? It will not happen for this MR, probably, but just to not step in the same pitfall once again. Thanks in advance!

arixmkii · 2022-09-14T07:22:53Z

Thank you all for your input! I will move it into a draft and will rework into a simpler implementation, because now I can clearly see you reasoning and it is up to maintainers to tell what they are fine to support longterm. Might take a week or smth, because I'm a bit overloaded now.

mtrmac · 2022-09-14T15:00:56Z

@mtrmac Could you give me the heads ups on how one could use BSD sources in APL2 project?

I don’t immediately know. It might well be possible, or it might be a problem. I don’t think I’m qualified to make a recommendation or decide whether a particular licensing arrangement is acceptable.

I think it’s just easier to work around this so that we don’t rely on the answer on this question.

(But I do feel rather confident in saying that removing copyright notices is basically never OK.)

arixmkii · 2023-01-11T22:03:53Z

Hello @Luap99 @mtrmac Sorry for wasting your time before. I figured out a lot more simple solution w/o any code borrowing. I hope now it will go easier. Please take a look, when you have time.

arixmkii · 2023-01-11T22:06:55Z

could we directly use LookPath from os/exec? What would be different?

@giuseppe @TomSweeneyRedHat yes, after all we actually could, I just didn't know how that time.

arixmkii · 2023-01-12T12:50:34Z

pkg/config/config.go

 if fi, err := os.Stat(fullpath); err == nil && fi.Mode().IsRegular() {
 return fullpath, nil
 }
+ abspath, err := filepath.Abs(fullpath)
+ if err == nil {
+ if lp, err := exec.LookPath(abspath); err == nil {
+ return lp, nil
+ }
+ }


Thinking about this. If we have absolute path then use builtin machinery, but as a fallback use old implementation.

Suggested change

if fi, err := os.Stat(fullpath); err == nil && fi.Mode().IsRegular() {

return fullpath, nil

}

abspath, err := filepath.Abs(fullpath)

if err == nil {

if lp, err := exec.LookPath(abspath); err == nil {

return lp, nil

}

}

if filepath.IsAbs(fullpath) {

if lp, err := exec.LookPath(fullpath); err == nil {

return lp, nil

}

} else {

if fi, err := os.Stat(fullpath); err == nil && fi.Mode().IsRegular() {

return fullpath, nil

}

}

I don't understand either solution, this function by definition should not lookup in $PATH unless searchPATH was set to true.

From experiments, LookPath doesn't traverse the PATH records if absolute path is given as input (or even if it does, then it is still using given absolute instead of filepath.Join). For Windows OS it still extends the search from C:\something\executablename to C:\something\executablename.exe, C:\something\executablename.bat, etc. And then return the updated version (with the extension) as an output, if one is found.

I can put it behind if runtime.GOOS = "windows" as it seems this doesn't really change the behavior of the current implementation for non-Windows OSes.

The end goal is to make it understand that "C:\Program Files\Red Hat\Podman\gvproxy" should resolve to "C:\Program Files\Red Hat\Podman\gvproxy.exe". So, that it will be possible to use the same binary name gvproxy (which is now hardcoded, and it looks fine to have it this way) w/o any conditional code in Podman, because this method will behave more like a LookPath, which is capable of understanding that for example ssh is actually ssh.exe on Windows.

But can't you just remove

If we filter directories, which are not given as absolute paths or use filepath.Abs and ignore error cases (I don't know if here we could satisfy the conditions for this to fail, but we have to take into account the error case). Because now it gives potential escape via relative path from the PWD, but if we pass relative ones into LookPath it is a wired range of path escapes. And not-validated for being absolute path CONTAINERS_HELPER_BINARY_DIR is the first candidate to be a trouble maker:

if dir, found := os.LookupEnv("CONTAINERS_HELPER_BINARY_DIR"); found { dirList = append([]string{dir}, dirList...) }

Not related to this PR, but it now looks to me that CONTAINERS_HELPER_BINARY_DIR deserves some sanity checks.

I will revisit the changes this weekend and will do extended testing on non-Windows hosts.

passing the full path to LookupPath() should also work on linux/unix?

Yes, that seems to be a good approach: Always do filepath.Abs(filepath.Join(…)) + exec.LookPath. There would be not much extra cost on Unix (filepath.Abs is just os.Getwd + some in-memory manipulation).

The returned path from this function could now be absolute in most cases, but that’s hardly a bad thing.

Either way, the LookupEnv call does need a quite verbose comment explaining that this is important to preserve, because on Windows it turns a command name into an executable path (handling $PATHEXT) — perhaps even with an example.

Otherwise someone like me would be pretty likely to remove it in a few months.

I will revisit the changes this weekend and will do extended testing on non-Windows hosts.

Having a unit test (at least a single case combining a path + a fictitious executable t.Tempdir()) would be nice as well. OTOH the test would have to be, annoyingly, platform-specific.

(I have no opinion on whether the unit test should be a required part of this PR.)

Simplified implementation (always use Abs + LookPath) and added comments.

Proofs that LookPath always short circuits for absolute paths:

Windows https://github.com/golang/go/blob/213495a4a67c318a1fab6e76093e6690c2141c0e/src/os/exec/lp_windows.go#L82

Unix https://github.com/golang/go/blob/213495a4a67c318a1fab6e76093e6690c2141c0e/src/os/exec/lp_unix.go#L57

This implementation is slightly superior to original one on Unix in the form that it taxed into account executable bit and on Windows it correctly loops through PATHEXT, when needed.

I'm not sure if this needs unit test then, because golang repo looks like a more appropriate way to place them (we want to validate platform behavior).

arixmkii · 2023-02-03T22:03:22Z

Implementation details:

Unix:

// exec.LookPath from absolute path on Unix is equal to os.Stat + IsNotDir + check for executable bits in FileMode

Short circuit for absolute path https://github.com/golang/go/blob/213495a4a67c318a1fab6e76093e6690c2141c0e/src/os/exec/lp_unix.go#L57
Check applied https://github.com/golang/go/blob/master/src/os/exec/lp_unix.go#L22

Windows:

// exec.LookPath from absolute path on Windows is equal to os.Stat + IsNotDir for `file.ext` or loops through extensions from PATHEXT for `file`

Short circuit https://github.com/golang/go/blob/213495a4a67c318a1fab6e76093e6690c2141c0e/src/os/exec/lp_windows.go#L82
Checking available extensions https://github.com/golang/go/blob/213495a4a67c318a1fab6e76093e6690c2141c0e/src/os/exec/lp_windows.go#L38
Actual code checking binary presence https://github.com/golang/go/blob/213495a4a67c318a1fab6e76093e6690c2141c0e/src/os/exec/lp_windows.go#L19

The biggest overhead here will be os.Getwd(), but it will only be called when path values in dirList are not absolute.

So, the implementation will work in a very optimized way in the majority of cases.

arixmkii · 2023-02-03T22:08:21Z

I rebased to latest main, there are linter failures, but they don't look related to this PR

  Error: SA1019: lockfile.Locker is deprecated: Refer directly to *LockFile, the provided implementation, instead.  (staticcheck)
  Error: SA1019: lockfile.Locker is deprecated: Refer directly to *LockFile, the provided implementation, instead.  (staticcheck)
  Error: SA1019: lockfile.Locker is deprecated: Refer directly to *LockFile, the provided implementation, instead.  (staticcheck)
  Error: func `prepareProbeBinary` is unused (unused)

mtrmac

LGTM.

@Luap99 ?

(The test failures need to be resolved before merging, but probably not in this PR, and quite possibly by someone else.)

pkg/config/config.go

exec.LookPath seems to handle absolute paths gracefully. On Windows this allows to additionally check for all known executable alternatives when only name is provided. Signed-off-by: Arthur Sengileyev <[email protected]>

Luap99

LGTM

openshift-ci · 2023-02-13T13:53:26Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: arixmkii, Luap99

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [Luap99]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

mtrmac · 2023-02-13T15:25:55Z

LGTM .

Now, who can understand and fix the test failure?

Luap99 · 2023-02-13T15:28:07Z

I rerun the test, I don't understand what is wrong but I think this is a flake.

rhatdan · 2023-02-14T02:56:28Z

/lgtm
/hold

Luap99 · 2023-02-14T13:32:58Z

/hold cancel

rhatdan reviewed Aug 26, 2022

View reviewed changes

pkg/config/lpe_unix.go Outdated Show resolved Hide resolved

arixmkii force-pushed the internal_look_path_impl branch from 98e98f3 to b76b4c6 Compare August 29, 2022 08:45

rhatdan reviewed Aug 29, 2022

View reviewed changes

pkg/config/lpe_windows.go Outdated Show resolved Hide resolved

rhatdan reviewed Aug 29, 2022

View reviewed changes

pkg/config/lpe_unix.go Outdated Show resolved Hide resolved

arixmkii force-pushed the internal_look_path_impl branch 4 times, most recently from 1c98dcd to fbebd13 Compare August 29, 2022 15:36

Luap99 reviewed Sep 5, 2022

View reviewed changes

pkg/config/lpe_unix.go Outdated Show resolved Hide resolved

pkg/config/lpe_unix.go Outdated Show resolved Hide resolved

mtrmac requested changes Sep 13, 2022

View reviewed changes

arixmkii marked this pull request as draft September 14, 2022 07:23

openshift-ci bot added the do-not-merge/work-in-progress label Sep 14, 2022

arixmkii force-pushed the internal_look_path_impl branch 2 times, most recently from 8ef712d to e90ecd2 Compare January 11, 2023 21:57

arixmkii marked this pull request as ready for review January 11, 2023 21:57

openshift-ci bot removed the do-not-merge/work-in-progress label Jan 11, 2023

arixmkii requested review from rhatdan and mtrmac and removed request for rhatdan and mtrmac January 11, 2023 22:02

arixmkii force-pushed the internal_look_path_impl branch from e90ecd2 to 0f6f68d Compare January 11, 2023 22:11

arixmkii commented Jan 12, 2023

View reviewed changes

arixmkii force-pushed the internal_look_path_impl branch 2 times, most recently from ca495c3 to ba89abf Compare February 3, 2023 21:58

arixmkii requested review from mtrmac and removed request for rhatdan February 3, 2023 22:09

arixmkii mentioned this pull request Feb 4, 2023

[Feature]: Support Podman Machine provider selection using containers.conf containers/podman#17116

Closed

mtrmac reviewed Feb 6, 2023

View reviewed changes

pkg/config/config.go Outdated Show resolved Hide resolved

arixmkii mentioned this pull request Feb 10, 2023

Support Qemu accelerated podman machine on Windows hosts containers/podman#13006

Open

FindHelperBinary will try LookPath of absolute path

0c2ecb0

exec.LookPath seems to handle absolute paths gracefully. On Windows this allows to additionally check for all known executable alternatives when only name is provided. Signed-off-by: Arthur Sengileyev <[email protected]>

arixmkii force-pushed the internal_look_path_impl branch from ba89abf to 0c2ecb0 Compare February 12, 2023 15:51

arixmkii requested a review from mtrmac February 12, 2023 15:58

Luap99 approved these changes Feb 13, 2023

View reviewed changes

openshift-ci bot added the approved label Feb 13, 2023

openshift-ci bot added the do-not-merge/hold label Feb 14, 2023

openshift-ci bot assigned rhatdan Feb 14, 2023

openshift-ci bot added the lgtm label Feb 14, 2023

openshift-ci bot removed the do-not-merge/hold label Feb 14, 2023

openshift-merge-robot merged commit 3009dfe into containers:main Feb 14, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

LookPath like impelementations for OS specific executable searches #1131

LookPath like impelementations for OS specific executable searches #1131

arixmkii commented Aug 25, 2022 •

edited

Loading

arixmkii commented Aug 25, 2022

rhatdan commented Sep 2, 2022

giuseppe commented Sep 2, 2022

TomSweeneyRedHat commented Sep 2, 2022

Luap99 left a comment

arixmkii commented Sep 6, 2022

mtrmac left a comment

mtrmac commented Sep 13, 2022

arixmkii commented Sep 14, 2022

arixmkii commented Sep 14, 2022

mtrmac commented Sep 14, 2022 •

edited

Loading

arixmkii commented Jan 11, 2023

arixmkii commented Jan 11, 2023

arixmkii Jan 12, 2023

Luap99 Jan 12, 2023

arixmkii Jan 12, 2023 •

edited

Loading

arixmkii Jan 12, 2023

arixmkii Jan 12, 2023

arixmkii Jan 12, 2023

arixmkii Jan 12, 2023

mtrmac Jan 12, 2023

mtrmac Jan 12, 2023

arixmkii Feb 3, 2023

arixmkii commented Feb 3, 2023 •

edited

Loading

arixmkii commented Feb 3, 2023

mtrmac left a comment

Luap99 left a comment

openshift-ci bot commented Feb 13, 2023

mtrmac commented Feb 13, 2023

Luap99 commented Feb 13, 2023

rhatdan commented Feb 14, 2023

Luap99 commented Feb 14, 2023

LookPath like impelementations for OS specific executable searches #1131

LookPath like impelementations for OS specific executable searches #1131

Conversation

arixmkii commented Aug 25, 2022 • edited Loading

arixmkii commented Aug 25, 2022

rhatdan commented Sep 2, 2022

giuseppe commented Sep 2, 2022

TomSweeneyRedHat commented Sep 2, 2022

Luap99 left a comment

Choose a reason for hiding this comment

arixmkii commented Sep 6, 2022

mtrmac left a comment

Choose a reason for hiding this comment

mtrmac commented Sep 13, 2022

arixmkii commented Sep 14, 2022

arixmkii commented Sep 14, 2022

mtrmac commented Sep 14, 2022 • edited Loading

arixmkii commented Jan 11, 2023

arixmkii commented Jan 11, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arixmkii Jan 12, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arixmkii commented Feb 3, 2023 • edited Loading

arixmkii commented Feb 3, 2023

mtrmac left a comment

Choose a reason for hiding this comment

Luap99 left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Feb 13, 2023

mtrmac commented Feb 13, 2023

Luap99 commented Feb 13, 2023

rhatdan commented Feb 14, 2023

Luap99 commented Feb 14, 2023

arixmkii commented Aug 25, 2022 •

edited

Loading

mtrmac commented Sep 14, 2022 •

edited

Loading

arixmkii Jan 12, 2023 •

edited

Loading

arixmkii commented Feb 3, 2023 •

edited

Loading