Fix which behavior when passed an empty string #33150

tgflynn · 2019-09-03T20:27:28Z

Currently when passed an argument that is the empty string Sys.which() returns the path of a directory.

For example:

julia> Sys.which("")
"/usr/local/sbin"

This is not correct since the empty string is not a valid path component on any OS of which I am aware.

This PR fixes this issue so that Sys.which("") returns nothing in accord with its documentation.

staticfloat · 2019-09-03T20:34:36Z

base/sysinfo.jl

@@ -459,6 +459,9 @@ for executable permissions only (with `.exe` and `.com` extensions added on
 Windows platforms); no searching of `PATH` is performed.
 """
 function which(program_name::String)
+    if isempty(program_name)


Let's nip things like " " in the bud as well; do isempty(strip(program_name))

So it turns out that this already works with the current code (at least on Linux).

The reason is that isexecutable("/usr/bin/ ") == false while isexecutable("/usr/bin/") == true.

It makes sense to add the test though.

In that case, I think it makes more sense to change isexecutable() to isfile() && isexecutable(). What do you think?

Yes, I agree that makes sense.

I think we might as well keep the early exit for an empty string though. There's no point in searching a bunch of directories for something they can't possibly contain.

While "" isn't a valid file name, " " is perfectly legal (if a bit insane) as an executable name.

Yep, though I think it's OK to include a test for that because the odds of there being such a file on a test system is very low.

There are currently no other tests for Sys.which and in general it's a bit complicated to write them because it would require controlling environment variables as well as the filesystem. However testing that Sys.which(" ") == nothing is easy and almost certain to be right.

FWIW, it's not so hard to make bulletproof tests for this:

mktempdir() do temp_dir my_bin = joinpath(temp_dir, "my_bin") touch(my_bin) chmod(my_bin, 0o755) withenv("PATH" => temp_dir) do @test abspath(Sys.which("my_bin")) == abspath(my_bin) @test Sys.which(" ") == nothing @test Sys.which("") == nothing end end

Something like that should work, modulo chmod() issues on Windows. (E.g. don't try to do test creating non-executable files on windows.... everything is executable on windows from Julia's perspective ;) )

We already have tests for that (in test/spawn.jl), we just need tests for the new code (empty string case).

Great. Good to merge from my perspective then.

@vtjnash

I'm not clear on why the tests are there. I thought the convention was that functions in base/foo.jl were tested in test/foo.jl.

But since that's where the other which tests are I should probably move the new ones into test/spawn.jl which will also allow using the existing testing framework to ensure that the blank filename test is correct.

staticfloat · 2019-09-03T20:34:51Z

test/sysinfo.jl

@@ -6,3 +6,6 @@
 sprint(Base.Sys.cpu_summary)
 @test Base.Sys.uptime() > 0
 Base.Sys.loadavg()
+
+# Check that which behaves correctly when passed an empty string
+@test isnothing(Base.Sys.which(""))


Let's test isnothing(Base.Sys.which(" ")) as well

string

tgflynn · 2019-09-03T22:30:05Z

I think commit a264f01 should complete this PR. Should be OK to merge if the tests pass.

test/spawn.jl)

tgflynn · 2019-09-04T15:03:36Z

It looks like the tests are failing due to download problems.

staticfloat · 2019-09-04T16:21:51Z

Thanks for your contribution @tgflynn!

* Fix behavior of Sys.which when passed an empty String argument * Added test to check for fixed Sys.which behavior with empty string input * Added test to check that Sys.which returns nothing when passed a blank string * Ensure that Sys.which returns a regular file and never a directory * Moved new Sys.which tests into test/spawn.jl alongside the existing ones * Remove new which tests from test/sysinfo.jl (they've moved to test/spawn.jl) (cherry picked from commit fa235cc)

StefanKarpinski · 2019-09-05T15:09:36Z

test/spawn.jl

        # Test that completely missing files also return nothing
        @test Sys.which("this_is_not_a_command") === nothing
+
+        # Check that which behaves correctly when passed a blank string
+        @test isnothing(Base.Sys.which(" "))


I'm a bit late to the party, but should we also test that when there actually is an executable named in the path we can find it?

There are existing tests for that. I just added a test for a blank string and made sure that the tests that should return nothing are independent of what's on the filesystem.

* Fix behavior of Sys.which when passed an empty String argument * Added test to check for fixed Sys.which behavior with empty string input * Added test to check that Sys.which returns nothing when passed a blank string * Ensure that Sys.which returns a regular file and never a directory * Moved new Sys.which tests into test/spawn.jl alongside the existing ones * Remove new which tests from test/sysinfo.jl (they've moved to test/spawn.jl) (cherry picked from commit fa235cc)

tgflynn added 2 commits September 3, 2019 08:43

Fix behavior of Sys.which when passed an empty String argument

b47d0b0

Added test to check for fixed Sys.which behavior with empty string input

76bf01f

fredrikekre requested a review from staticfloat September 3, 2019 20:28

staticfloat requested changes Sep 3, 2019

View reviewed changes

fredrikekre added backport 1.0 labels Sep 3, 2019

tgflynn added 2 commits September 3, 2019 17:04

Added test to check that Sys.which returns nothing when passed a blank

2c8ba62

string

Ensure that Sys.which returns a regular file and never a directory

31e7d7a

staticfloat approved these changes Sep 3, 2019

View reviewed changes

Moved new Sys.which tests into test/spawn.jl alongside the existing ones

0198ad9

Remove new which tests from test/sysinfo.jl (they've moved to

a264f01

test/spawn.jl)

staticfloat merged commit fa235cc into JuliaLang:master Sep 4, 2019

KristofferC mentioned this pull request Sep 5, 2019

WIP: Backports for 1.3-RC2 #32973

Merged

36 tasks

StefanKarpinski reviewed Sep 5, 2019

View reviewed changes

KristofferC removed the backport 1.3 label Sep 8, 2019

KristofferC mentioned this pull request Dec 3, 2019

WIP: Backports release 1.0.6 #34011

Closed

56 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix which behavior when passed an empty string #33150

Fix which behavior when passed an empty string #33150

tgflynn commented Sep 3, 2019 •

edited

Loading

staticfloat Sep 3, 2019

tgflynn Sep 3, 2019

staticfloat Sep 3, 2019

tgflynn Sep 3, 2019

vtjnash Sep 3, 2019

tgflynn Sep 3, 2019

staticfloat Sep 3, 2019

vtjnash Sep 3, 2019

staticfloat Sep 3, 2019

tgflynn Sep 3, 2019

staticfloat Sep 3, 2019

tgflynn commented Sep 3, 2019 •

edited

Loading

tgflynn commented Sep 4, 2019

staticfloat commented Sep 4, 2019

StefanKarpinski Sep 5, 2019

tgflynn Sep 5, 2019

Fix which behavior when passed an empty string #33150

Fix which behavior when passed an empty string #33150

Conversation

tgflynn commented Sep 3, 2019 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tgflynn commented Sep 3, 2019 • edited Loading

tgflynn commented Sep 4, 2019

staticfloat commented Sep 4, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tgflynn commented Sep 3, 2019 •

edited

Loading

tgflynn commented Sep 3, 2019 •

edited

Loading