Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
many: abstract path to /bin/{true,false} #3096
Conversation
zyga
self-requested a review
Mar 28, 2017
| +} | ||
| + | ||
| +func (s *findInPathSuite) SetUpSuite(c *C) { | ||
| + p, err := ioutil.TempDir("", "find-in-path") |
zyga
Mar 31, 2017
Contributor
Maybe just c.MkDir for a temporary directory you don't have to manage?
morphis
Mar 31, 2017
Contributor
Isn't that what ioutil.TempDir does internally for me with the advantage of having a unique dir for every test run?
zyga
Mar 31, 2017
Contributor
Yep, it's just more integrated as I think it gets removed automatically.
| + c.Assert(err, Equals, nil) | ||
| + s.basePath = p | ||
| + s.envPath = os.Getenv("PATH") | ||
| + os.Setenv("PATH", fmt.Sprintf("%s/bin:%s/usr/bin", s.basePath, s.basePath)) |
zyga
Mar 31, 2017
Contributor
Os.Setenv is problematic. Not sure if we should call it or should just mock something like osPath instead.
https://rachelbythebay.com/w/2017/01/30/env/
I'm not sure if golang calls the C library or just buffers the operation.
| + } | ||
| +} | ||
| + | ||
| +func (s *findInPathSuite) TearDownSuite(c *C) { |
added some commits
Mar 28, 2017
Some checks were not successful
| @@ -49,23 +49,31 @@ type ErrtrackerTestSuite struct { | ||
| var _ = Suite(&ErrtrackerTestSuite{}) | ||
| +var truePath string |
mvo5
Apr 3, 2017
Collaborator
You can write this as:
var truePath = osutil.FindInPathOrDefault("true", "/bin/true")
var falsePath = osutil.FindInPathOrDefault("false", "/bin/false")
to safe some lines of code :) (i.e. no need for the extra init() AFAICS).
| +var gccPath string | ||
| + | ||
| +func init() { | ||
| + truePath = osutil.FindInPathOrDefault("true", "/bin/true") |
mvo5
Apr 3, 2017
Collaborator
Same as above, this can go directly to the "var" bits to safe some lines.
| +// listed in the environment variable PATH and returns the found path or the | ||
| +// provided default path. | ||
| +func FindInPathOrDefault(name string, defaultPath string) string { | ||
| + paths := strings.Split(Getenv("PATH"), ":") |
mvo5
Apr 3, 2017
Collaborator
There is a exec.LookupPath() which is doing what this function is doing except the "default" part. So maybe use that and when LookupPath returns an ErrNotFound return the provided default?
| +func FindInPathOrDefault(name string, defaultPath string) string { | ||
| + paths := strings.Split(Getenv("PATH"), ":") | ||
| + for _, p := range paths { | ||
| + candidate := fmt.Sprintf("%s/%s", p, name) |
mvo5
Apr 3, 2017
Collaborator
I slightly prefer candidate := filepath.Join(p, name) to join the dir and the filename (but its to a certain extend nitpick :)
| + paths := strings.Split(Getenv("PATH"), ":") | ||
| + for _, p := range paths { | ||
| + candidate := fmt.Sprintf("%s/%s", p, name) | ||
| + _, err := os.Stat(candidate) |
mvo5
Apr 3, 2017
Collaborator
You could use if FileExists(candidate) { return candidate } here which is slightly shorter.
added some commits
Apr 3, 2017
Some checks were not successful
| @@ -38,13 +36,9 @@ func FindInPath(name string) string { | ||
| // listed in the environment variable PATH and returns the found path or the | ||
| // provided default path. | ||
| func FindInPathOrDefault(name string, defaultPath string) string { |
mvo5
Apr 3, 2017
Collaborator
Sorry for being complicated, I wonder if maybe LookupPathWithDefault() so that its closer to the exec.LookupPath (i.e. less cognitive load by having "FindInPath" vs "LookupPath" depending on what is needed).
mvo5
approved these changes
Apr 3, 2017
Looks good, thanks for doing this work! One final nitpick but feel free to ignore (or merge if you want as is).
Some checks were not successful
mvo5
added this to the
2.24 milestone
Apr 4, 2017
zyga
added some commits
Apr 4, 2017
Some checks were not successful
All checks have passed
niemeyer
requested changes
Apr 4, 2017
A few changes requested to make the amount of changes more proportional to the problem being solved. Please feel free to merge when you're happy about the handling of these problems.
| + | ||
| +var truePath = osutil.LookupPathWithDefault("true", "/bin/true") | ||
| +var falsePath = osutil.LookupPathWithDefault("false", "/bin/false") | ||
| +var gccPath = osutil.LookupPathWithDefault("gcc", "/usr/bin/gcc") |
niemeyer
Apr 4, 2017
Contributor
These should be close to where they are used instead of in a file by themselves. If they are used in more than one file, it's okay to pick the one which uses them the most, or the one you like the most. :)
| +// LookupPathWithDefault searches for a given command name in all directories | ||
| +// listed in the environment variable PATH and returns the found path or the | ||
| +// provided default path. | ||
| +func LookupPathWithDefault(name string, defaultPath string) string { |
niemeyer
Apr 4, 2017
•
Edited 1 time
-
niemeyer
Apr 4, 2017
Contributor
-
Apr 4, 2017
This feels a little like overshooting a bit given the real needs we have in this PR, as this is the only function we really needed, and their names don't actually give any hint about why we have the three (LookupPath vs. LookPath).
Instead, I suggest adding just the following implementation with this name:
func LookPathDefault(name, defpath string) string {
If we need LookPath itself, we can use the exec package directly, with the familiar name and signature.
In tests, it should be easy to manipulate the environment to test the real function without using mocks.
morphis
Apr 5, 2017
•
Edited 1 time
-
morphis
Apr 5, 2017
Contributor
-
Apr 5, 2017
@niemeyer I was asked by @zyga to not use os.Setenv in the unit tests. Changing the code to just have LookPathDefault
| + | ||
| +var _ = Suite(&lookupPathSuite{}) | ||
| + | ||
| +func (s *lookupPathSuite) TestGivesCorrectPath(c *C) { |
niemeyer
Apr 4, 2017
Contributor
Similarly, let's integrate these into stat_test.go.
For the record, this package is overbroken, so I apologize for the bad example. We should eventually merge some of those tiny one-function files into proper chunks by similarity. A bit like stat.go.
morphis commentedMar 28, 2017
The true and false command line utilities are not on all systems in /bin. Therefor this PR implements a tiny helper method which searches for commands in PATH and returns the found path. This allows us now to abstract the path to relevant binaries instead of specifying ones specific for a single OS. On Fedora and openSUSE true and false are in /usr/bin.