Add `Sys.which()`, use that to find `curl` in `download()` #26559

staticfloat · 2018-03-21T17:29:50Z

It's better to invoke curl directly (e.g. via curl --help) in order to see if it exists than to use which curl and see if that successfully passes. This fixes test errors on e.g. our CentOS 7.3 buildbots, which do not have which available by default.

iamed2 · 2018-03-21T17:47:43Z

Maybe someone with FreeBSD can confirm, but I don't think fetch supports --help or any other no-op argument.

ararslan · 2018-03-21T18:48:22Z

Can confirm, it does not.

stevengj · 2018-03-21T18:57:25Z

Might be nice to have a portable which, analogous to Python's distutils.spawn.find_executable or shutils.which, if we don't have one already.

ararslan · 2018-03-21T19:02:29Z

@stevengj, Elliot and I were just talking about that offline. He intends to add such a function based off of https://github.com/ararslan/BuildUtils.jl/blob/master/src/BuildUtils.jl#L46-L74. 🙂

staticfloat · 2018-03-21T19:14:07Z

Yep, I'll tack it onto this PR, just writing up the tests now. It'll be called Sys.which(), and I expect a full course of bikeshedding. Don't let me down guys.

ararslan · 2018-03-21T19:45:41Z

base/sysinfo.jl

+    # If prog has a slash, we know the user wants to determine whether the given
+    # file exists and is executable, and to not search the `PATH`.  Note that
+    # Windows can have either `\\` or `/` in its paths:
+    dirseps = @static if is_windows() ["/", "\\"] else ["/"] end


is_windows is deprecated to iswindows

ararslan · 2018-03-21T19:45:54Z

base/sysinfo.jl

+    # Windows can have either `\\` or `/` in its paths:
+    dirseps = @static if is_windows() ["/", "\\"] else ["/"] end
+
+    if any(contains.(program_name, dirseps))


contains(a, b) is deprecated to occursin(b, a)

ararslan · 2018-03-21T19:46:28Z

base/sysinfo.jl

+    # We use `access()` and `X_OK` to determine if a given path is executable
+    # by the current user.  `X_OK` comes from `unistd.h`.
+    X_OK = 1 << 0
+    can_execute(program_name) = @static if is_windows()


This could be moved out of the function

stevengj · 2018-03-21T21:22:12Z

base/sysinfo.jl

+        ["/", "\\"]
+    else
+        ["/"]
+    end


Probably a tuple of chars here rather than an array of strings?

Are tuples preferred here for performance considerations?

stevengj · 2018-03-21T21:24:06Z

base/sysinfo.jl

+    # If we have been given just a program name (not a relative or absolute
+    # path) then we should search `PATH` for it here:
+    path = get(ENV, "PATH", "")
+    pathsep = @static if iswindows()


pathsep = @static iswindows() ? ';' : ':' would be shorter and probably more readable for conditionals like this.

Ah, for some reason I thought @static didn't work with the ternary operator. Nice.

The @static isn't entirely necessary

stevengj · 2018-03-21T21:26:12Z

base/sysinfo.jl

+
+    # On windows, we need to check both $(program_name) and $(program_name).exe
+    program_matches(file, program_name) = @static if iswindows()
+        file == program_name || file == "$(program_name).exe"


Don't you need to check everything in get(ENV, "PATHEXT", "exe")?

Also, you presumably only want to check appending an extension if program_name doesn't already have an extension (i.e. if it doesn't contain .).

No to the first (also .cmd), yes to the second (with qualifications). We should probably try to return the same answer as our underlying spawn API (https://github.com/libuv/libuv/blob/edf05b97f03472def2837ceb2d6bf61a0d0dc248/src/win/process.c#L299)

stevengj · 2018-03-21T21:28:42Z

base/sysinfo.jl

+        file == program_name
+    end
+
+    for dir in split(path, pathsep), file in readdir(dir)


Why walk the directory tree? This seems very inefficient if the directories contain lots of entries, as will be common. Instead, you could do something like:

for dir in split(path, pathsep) file = joinpath(path, program_name) isfile(file) && _can_execute(file) && return abspath(file) .... check extensions on windows .... end

right?

stevengj · 2018-03-21T21:29:13Z

base/sysinfo.jl

+
+Returns `true` if the given `path` has executable permissions.
+"""
+function _can_execute(path::AbstractString)


isexecutable seems more idiomatic.

stevengj · 2018-03-21T21:37:15Z

base/sysinfo.jl

+        ["/"]
+    end
+
+    if any(occursin.(dirseps, program_name))


or just

if occursin(@static(iswindows() ? r"[/\\]" : '/'), program_name)

vtjnash · 2018-03-21T22:51:46Z

base/sysinfo.jl

+    # by the current user.  `X_OK` comes from `unistd.h`.
+    X_OK = 1 << 0
+    @static if iswindows()
+        ccall(:_access, Cint, (Ptr{UInt8}, Cint), path, X_OK) == 0


access on windows doesn't have an X_OK parameter

Ah, that's an excellent point. I just spent half an hour trying to parse MSDN and figure out how GetNamedSecurityInfo can help me figure out the right ACL values to see if something is executable by the current user, but it defeated me. I'm just going to return true on Windows.

Yes, the which function on Windows probably shares fairly little with the same function on posix. I expect they likely should just be completely independent implementations

.........busybox-w32 uses access(path, X_OK). Sigh.

I’m not too surprised. It’s a posix emulation later, probably intended to approximate the result you’d get on busybox sh or cygwin (similar to how the real which program on posix approximates the likely answer)

vtjnash · 2018-03-22T00:30:57Z

base/sysinfo.jl

+    pathsep = @static iswindows() ? ';' : ':'
+
+    # On windows, if the program name does not already have an extension,
+    # we need to tack all the values in PATHEXT onto the end and check for


Nope, see how libuv implements this function. IIRC, the distinction is that PATHEXT is for DOS, not the Shell32.

Which function are you referring to? What should I look for in libuv?

I'm not sure what you mean in stating that PATHEXT is only for DOS; when I try to run() a command, I expect it to work like I've typed it into cmd.exe, and cmd.exe does pay attention to PATHEXT.

See my libuv link in the other comment thread

@vtjnash means that we want to match the semantics of search_path in libuv's src/win/process.c, I guess, since this is what run uses.

Unfortunately, the search_path function is not exported. But since we use our own libuv fork anyway, maybe we should just modify it to export a uv_win_search_path function on Windows? But it doesn't look that complicated to implement libuv's semantics — it's actually somewhat simpler than what you are doing now.

Libuv says:

Furthermore, it tries to follow the semantics that cmd.exe, with this exception that PATHEXT environment variable isn't used. Since CreateProcess can start only .com and .exe files, only those extensions are tried. This behavior equals that of msvcrt's spawn functions.

Note also some differences with what you are doing now:

If there's really only a filename, check the current directory for file, then search all path directories.

If filename specified has any extension, search for the file with the specified extension first.

If the literal filename is not found in a directory (even if the file already had an extension), try appending (not replacing) .com first and then .exe.

Note also that libuv writes CMD does not trim leading/trailing whitespace from path/pathex entries

stevengj · 2018-03-22T02:12:31Z

base/sysinfo.jl

+    program_names = [program_name]
+    @static if iswindows()
+        if !isempty(splitext(program_name)[2])
+            pathexts = get(ENV, "PATHEXT", ".com; .exe; .bat; .cmd")


no spaces in the default

stevengj · 2018-03-22T02:13:21Z

base/sysinfo.jl

+    @static if iswindows()
+        if !isempty(splitext(program_name)[2])
+            pathexts = get(ENV, "PATHEXT", ".com; .exe; .bat; .cmd")
+            pathexts = strip.(split(pathexts, ';'))


Is the strip necessary? Can PATHEXT contain whitespace?

staticfloat · 2018-03-23T07:24:59Z

So looking through this, I guess we have to make the decision; do we want to behave like cmd here, or do something else? Since the most logical use of this function is to be used with run(), which I assume is going to eventually sub off to CreateProcess, we should probably ignore PATHEXT completely just like libuv does, and only search for .exe and .com files.

stevengj · 2018-03-23T11:07:30Z

I think which("foo") should return the same program that run(`foo`) would execute, which means it needs to follow libuv.

stevengj · 2018-03-26T12:06:58Z

base/sysinfo.jl

+"""
+function isexecutable(path::AbstractString)
+    @static if iswindows()
+        return true


isfile(path)?

stevengj · 2018-03-26T12:07:33Z

base/sysinfo.jl

+        for program_name in program_names
+            program_path = joinpath(path_dir, program_name)
+            # If we find something that matches our name and we can execute
+            if isfile(program_path) && isexecutable(program_path)


If you change isexecutable to isfile on Windows as suggested above, then you can omit the isfile check here.

staticfloat · 2018-03-30T23:34:51Z

Okay, I think I've addressed all the comments so far.

ararslan · 2018-03-30T23:43:25Z

base/sysinfo.jl

+"""
+function isexecutable(path::AbstractString)
+    @static if iswindows()
+        return isfile(path)


Can't you determine whether it has executable permissions using _access on Windows?

Nope. Windows doesn't have an X_OK bit. :(

https://msdn.microsoft.com/en-us/library/1w06ktdy.aspx

Ah okay, sorry for the noise then.

stevengj · 2018-03-31T12:03:00Z

base/sysinfo.jl

+        # If this file does not even exist, fail out
+        if !isfile(program_name)
+            throw(ArgumentError("$program_name does not exist"))
+        end


this check is now redundant with isexecutable

ararslan · 2018-05-09T21:30:58Z

base/sysinfo.jl

+    end
+
+    # If we couldn't find anything, complain
+    error("$program_name not found")


This should probably be an ArgumentError.

Ah, I see it originally was but Jameson said it shouldn't be. Just using error here seems unfortunately general.

Yeah, but I don't have a good feel for what errors should be what, so in the absence of a better idea, I'm going to keep it as error() for now.

ararslan · 2018-05-09T21:32:01Z

test/spawn.jl

+end
+
+mktempdir() do dir
+    pathsep = @static if Sys.iswindows() ";" else ":" end


A ternary would be less weird here

pathsep = Sys.iswindows() ? ";" : ":" is too much punctuation for me.

ararslan · 2018-05-09T21:32:32Z

test/spawn.jl

+            bar_path = joinpath(dir, "bin1", "bar.exe")
+            touch(bar_path)
+            chmod(bar_path, 0o777)
+            cd("$(dir)") do


dir is already a String; it doesn't need to be interpolated.

ararslan · 2018-05-09T21:33:03Z

test/spawn.jl

+
+        @static if Sys.iswindows()
+            # On windows, check that pwd() takes precedence, except when we provide a path
+            cd("$(dir)/bin2") do


joinpath(dir, "bin2")

ararslan · 2018-05-09T21:34:23Z

base/sysinfo.jl

+"""
+    Sys.isexecutable(path::String)
+
+Returns `true` if the given `path` has executable permissions.


We word our docs in the imperative, i.e. this should be "return" rather than "returns." Same for the which docstring below.

Hah, I was basing this off of the Sys.windows_version() docstring above it. I'll fix that too.

ararslan · 2018-05-09T21:35:12Z

base/sysinfo.jl

@@ -23,7 +23,8 @@ export BINDIR,
       isbsd,
       islinux,
       isunix,
-       iswindows
+       iswindows,
+       which


Maybe export isexecutable as well?

#26559 (comment)

This acts as a julia-native version of the `which` command found on most *nix systems; it searches the path for an executable of the given name, returning the absolute path if it exists, and throwing an `ArgumentError()` if it does not.

This fixes issues on systems that don't have `which` available

#26559 (comment)

staticfloat · 2018-05-22T03:13:17Z

This passes all CI now. Win32 build failure is unrelated, the spawn test passes, which is the only thing that matters for this PR.

Absent any other comments, I'm going to merge this tomorrow.

ararslan · 2018-05-22T23:25:12Z

Probably should have been squash-merged.

staticfloat · 2018-05-22T23:31:16Z

I intentionally didn't; each commit is intended to stand alone.

JuliaLang#26559 (comment)

As [discussed recently on discourse](https://discourse.julialang.org/t/weird-sys-which-function/58070), some programs require a certain executable name to function correctly, and behave badly if you call `realpath` to expand symbolic links (potentially changing the program name). c.f. discussion in #26559, implemented `Sys.which`

As [discussed recently on discourse](https://discourse.julialang.org/t/weird-sys-which-function/58070), some programs require a certain executable name to function correctly, and behave badly if you call `realpath` to expand symbolic links (potentially changing the program name). c.f. discussion in JuliaLang#26559, implemented `Sys.which`

staticfloat force-pushed the sf/very_small_rocks branch from 0d0a0b3 to e908eb0 Compare March 21, 2018 19:39

ararslan reviewed Mar 21, 2018

View reviewed changes

stevengj reviewed Mar 21, 2018

View reviewed changes

vtjnash reviewed Mar 21, 2018

View reviewed changes

staticfloat force-pushed the sf/very_small_rocks branch from 42c1909 to 5239a75 Compare March 22, 2018 00:22

vtjnash reviewed Mar 22, 2018

View reviewed changes

stevengj reviewed Mar 22, 2018

View reviewed changes

staticfloat force-pushed the sf/very_small_rocks branch from 5239a75 to 5b500a6 Compare March 23, 2018 07:34

stevengj reviewed Mar 26, 2018

View reviewed changes

base/sysinfo.jl Outdated

"""

function isexecutable(path::AbstractString)

@static if iswindows()

return true

Copy link

Member

stevengj Mar 26, 2018

Choose a reason for hiding this comment

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

isfile(path)?

stevengj reviewed Mar 26, 2018

View reviewed changes

staticfloat force-pushed the sf/very_small_rocks branch from 5b500a6 to d79c028 Compare March 30, 2018 23:22

staticfloat changed the title ~~Don't use which, just invoke curl directly~~ Add Sys.which(), use that to find curl in download() Mar 30, 2018

ararslan reviewed Mar 30, 2018

View reviewed changes

stevengj reviewed Mar 31, 2018

View reviewed changes

simonbyrne mentioned this pull request May 9, 2018

Future of Base.download #27043

Closed

ararslan reviewed May 9, 2018

View reviewed changes

staticfloat force-pushed the sf/very_small_rocks branch from e8b511b to db9e7f2 Compare May 18, 2018 22:16

staticfloat added a commit that referenced this pull request May 18, 2018

Fix wording of Sys.windows_version() to be in the imperative

db9e7f2

#26559 (comment)

staticfloat force-pushed the sf/very_small_rocks branch from 690f47b to 9bc1aa4 Compare May 19, 2018 22:51

staticfloat added a commit that referenced this pull request May 19, 2018

Fix wording of Sys.windows_version() to be in the imperative

9bc1aa4

#26559 (comment)

staticfloat force-pushed the sf/very_small_rocks branch from 9bc1aa4 to 3592250 Compare May 20, 2018 22:52

staticfloat added a commit that referenced this pull request May 20, 2018

Fix wording of Sys.windows_version() to be in the imperative

3592250

#26559 (comment)

staticfloat force-pushed the sf/very_small_rocks branch from 3592250 to dff0be4 Compare May 20, 2018 23:04

staticfloat added a commit that referenced this pull request May 20, 2018

Fix wording of Sys.windows_version() to be in the imperative

dff0be4

#26559 (comment)

staticfloat force-pushed the sf/very_small_rocks branch from dff0be4 to 3cf7dfc Compare May 21, 2018 20:34

staticfloat added a commit that referenced this pull request May 21, 2018

Fix wording of Sys.windows_version() to be in the imperative

3cf7dfc

#26559 (comment)

staticfloat force-pushed the sf/very_small_rocks branch from 3cf7dfc to 4c7f2cf Compare May 21, 2018 20:39

staticfloat added 4 commits May 21, 2018 13:39

Add Sys.which(program_name::AbstractString)

11d6f37

This acts as a julia-native version of the `which` command found on most *nix systems; it searches the path for an executable of the given name, returning the absolute path if it exists, and throwing an `ArgumentError()` if it does not.

Don't use which curl, use Sys.which("curl")

f76833f

This fixes issues on systems that don't have `which` available

Add NEWS.md entry for Sys.which()

f547a86

Fix wording of Sys.windows_version() to be in the imperative

4c7f2cf

#26559 (comment)

staticfloat merged commit 33fd977 into master May 22, 2018

ararslan deleted the sf/very_small_rocks branch May 22, 2018 23:22

Liozou pushed a commit to Liozou/julia that referenced this pull request May 24, 2018

Fix wording of Sys.windows_version() to be in the imperative

c201ff5

JuliaLang#26559 (comment)

ararslan mentioned this pull request May 27, 2018

Change the error behavior of Sys.which #27284

Closed

stevengj mentioned this pull request Jul 25, 2018

WIP: make which(str) return the path to the command #13197

Closed

4 tasks

stevengj mentioned this pull request Mar 27, 2021

don't use realpath in Sys.which #40233

Merged

Add Sys.which(), use that to find curl in download() #26559

Add Sys.which(), use that to find curl in download() #26559

Conversation

staticfloat commented Mar 21, 2018

iamed2 commented Mar 21, 2018

ararslan commented Mar 21, 2018

stevengj commented Mar 21, 2018

ararslan commented Mar 21, 2018

staticfloat commented Mar 21, 2018

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

stevengj Mar 21, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stevengj Mar 21, 2018 • edited Loading

Choose a reason for hiding this comment

stevengj Mar 21, 2018 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stevengj Mar 22, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stevengj Mar 22, 2018 • 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

staticfloat commented Mar 23, 2018

stevengj commented Mar 23, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

staticfloat commented Mar 30, 2018

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

staticfloat commented May 22, 2018

ararslan commented May 22, 2018

staticfloat commented May 22, 2018

Add `Sys.which()`, use that to find `curl` in `download()` #26559

Add `Sys.which()`, use that to find `curl` in `download()` #26559

stevengj Mar 21, 2018 •

edited

Loading

stevengj Mar 21, 2018 •

edited

Loading

stevengj Mar 21, 2018 •

edited

Loading

stevengj Mar 22, 2018 •

edited

Loading

stevengj Mar 22, 2018 •

edited

Loading