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.

Sign up for GitHub

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

Fix which behavior when passed an empty string #33150

Merged
merged 6 commits into from
Sep 4, 2019
Merged

Fix which behavior when passed an empty string #33150

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
b47d0b0
Fix behavior of Sys.which when passed an empty String argument
tgflynn Sep 2, 2019
76bf01f
Added test to check for fixed Sys.which behavior with empty string input
tgflynn Sep 3, 2019
2c8ba62
Added test to check that Sys.which returns nothing when passed a blank
tgflynn Sep 3, 2019
31e7d7a
Ensure that Sys.which returns a regular file and never a directory
tgflynn Sep 3, 2019
0198ad9
Moved new Sys.which tests into test/spawn.jl alongside the existing ones
tgflynn Sep 3, 2019
a264f01
Remove new which tests from test/sysinfo.jl (they've moved to
tgflynn Sep 4, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion base/sysinfo.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ;) )

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great. Good to merge from my perspective then.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

return nothing
end
# Build a list of program names that we're going to try
program_names = String[]
base_pname = basename(program_name)
Expand Down Expand Up @@ -501,7 +504,7 @@ function which(program_name::String)
for pname in program_names
program_path = joinpath(path_dir, pname)
# If we find something that matches our name and we can execute
if isexecutable(program_path)
if isfile(program_path) && isexecutable(program_path)
return realpath(program_path)
end
end
Expand Down
11 changes: 11 additions & 0 deletions test/spawn.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -556,6 +556,10 @@ withenv("PATH" => "$(Sys.BINDIR)$(psep)$(ENV["PATH"])") do
@test Sys.which(julia_exe) == realpath(julia_exe)
end

# Check that which behaves correctly when passed an empty string
@test isnothing(Base.Sys.which(""))


mktempdir() do dir
withenv("PATH" => "$(dir)$(psep)$(ENV["PATH"])") do
# Test that files lacking executable permissions fail Sys.which
Expand All @@ -572,8 +576,15 @@ mktempdir() do dir
@test Sys.which(foo_path) === nothing
end

end

# Ensure these tests are done only with a PATH of known contents
withenv("PATH" => "$(dir)") do
# 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(" "))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

end
end

Expand Down