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

Add Exec2, the "better version of Exec" #5103

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

Add Exec2, the "better version of Exec" #5103

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
53 changes: 29 additions & 24 deletions lib/helpview.gi
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@
winfilename:=MakeExternalFilename( SplitString( filename, "#" )[1] );
fi;
Print( "Opening help page ", winfilename, " in default windows browser ... \c" );
Exec( Concatenation("start ", winfilename ) );
Exec2( "start", winfilename );

Check warning on line 83 in lib/helpview.gi

View check run for this annotation

Codecov / codecov/patch

lib/helpview.gi#L83 

Added line #L83 was not covered by tests
Print( "done! \n" );
end
);
Expand Down Expand Up @@ -138,7 +138,7 @@
fi;
file := file.file;
fi;
Exec(Concatenation("open -a Preview ", file));
Exec2("open", "-a", "Preview", file);

Check warning on line 141 in lib/helpview.gi

View check run for this annotation

Codecov / codecov/patch

lib/helpview.gi#L141 

Added line #L141 was not covered by tests
Print("# see page ", page, " in the Preview window.\n");
end
);
Expand All @@ -155,7 +155,7 @@
fi;
file := file.file;
fi;
Exec(Concatenation("open -a \"Adobe Reader\" ", file));
Exec2("open", "-a", "Adobe Reader", file);

Check warning on line 158 in lib/helpview.gi

View check run for this annotation

Codecov / codecov/patch

lib/helpview.gi#L158 

Added line #L158 was not covered by tests
Print("# see page ", page, " in the Adobe Reader window.\n");
end
);
Expand All @@ -172,7 +172,7 @@
fi;
file := file.file;
fi;
Exec(Concatenation("open ", file));
Exec2("open ", file);

Check warning on line 175 in lib/helpview.gi

View check run for this annotation

Codecov / codecov/patch

lib/helpview.gi#L175 

Added line #L175 was not covered by tests
Print("# see page ", page, " in the pdf viewer window.\n");
end
);
Expand All @@ -187,16 +187,15 @@
fi;
file := file.file;
fi;
Exec( Concatenation(
"osascript <<ENDSCRIPT\n",
"tell application \"Skim\"\n",
"activate\n",
"open \"", file, "\"\n",
"set theDoc to document of front window\n",
"go theDoc to page ",String(page)," of theDoc\n",
"end tell\n",
"ENDSCRIPT\n" ) );
return;
Exec2("osascript",
InputTextString(Concatenation("""

Check warning on line 191 in lib/helpview.gi

View check run for this annotation

Codecov / codecov/patch

lib/helpview.gi#L190-L191 

Added lines #L190 - L191 were not covered by tests
tell application "Skim"
activate
open """, ViewString(file), """

Check warning on line 194 in lib/helpview.gi

View check run for this annotation

Codecov / codecov/patch

lib/helpview.gi#L194 

Added line #L194 was not covered by tests
set theDoc to document of front window
go theDoc to page """,String(page),""" of theDoc

Check warning on line 196 in lib/helpview.gi

View check run for this annotation

Codecov / codecov/patch

lib/helpview.gi#L196 

Added line #L196 was not covered by tests
end tell
""")));

Check warning on line 198 in lib/helpview.gi

View check run for this annotation

Codecov / codecov/patch

lib/helpview.gi#L198 

Added line #L198 was not covered by tests
end
);

Expand All @@ -207,17 +206,20 @@
HELP_VIEWER_INFO.browser := rec(
type := "url",
show := function( url )
local str;
str := "";
Exec2("wslpath", "-a", "-w", url, OutputTextString(str, false));

Check warning on line 211 in lib/helpview.gi

View check run for this annotation

Codecov / codecov/patch

lib/helpview.gi#L210-L211 

Added lines #L210 - L211 were not covered by tests
# Ignoring part of the URL after '#' since we are unable
# to navigate to the precise location on Windows
url := SplitString( url, "#" )[1];
Exec(Concatenation("explorer.exe \"$(wslpath -a -w \"",url, "\")\""));
Exec2("explorer.exe", str);

Check warning on line 215 in lib/helpview.gi

View check run for this annotation

Codecov / codecov/patch

lib/helpview.gi#L215 

Added line #L215 was not covered by tests
end
);

HELP_VIEWER_INFO.("pdf viewer") := rec(
type := "pdf",
show := function(file)
local page;
local page, str;
# unfortunately one cannot (yet?) give a start page to windows
page := 1;
if IsRecord(file) then
Expand All @@ -226,7 +228,10 @@
fi;
file := file.file;
fi;
Exec(Concatenation("explorer.exe \"$(wslpath -a -w \"",file, "\")\""));

str := "";
Exec2("wslpath", "-a", "-w", url, OutputTextString(str, false));
Exec2("explorer.exe", str);

Check warning on line 234 in lib/helpview.gi

View check run for this annotation

Codecov / codecov/patch

lib/helpview.gi#L232-L234 

Added lines #L232 - L234 were not covered by tests
Print("# see page ", page, " in PDF.\n");
end
);
Expand All @@ -235,15 +240,15 @@
HELP_VIEWER_INFO.netscape := rec(
type := "url",
show := function(url)
Exec(Concatenation("netscape -remote \"openURL(file:", url, ")\""));
Exec2("netscape", "-remote", Concatenation("openURL(file:", url, ")"));

Check warning on line 243 in lib/helpview.gi

View check run for this annotation

Codecov / codecov/patch

lib/helpview.gi#L243 

Added line #L243 was not covered by tests
end
);

# html version with mozilla
HELP_VIEWER_INFO.mozilla := rec(
type := "url",
show := function(url)
Exec(Concatenation("mozilla -remote \"openURL(file:", url, ")\""));
Exec2("mozilla", "-remote", Concatenation("openURL(file:", url, ")"));

Check warning on line 251 in lib/helpview.gi

View check run for this annotation

Codecov / codecov/patch

lib/helpview.gi#L251 

Added line #L251 was not covered by tests
end
);

Expand Down Expand Up @@ -276,36 +281,36 @@
HELP_VIEWER_INFO.lynx := rec(
type := "url",
show := function(url)
Exec(Concatenation("lynx \"", url, "\""));
Exec2("lynx", url);

Check warning on line 284 in lib/helpview.gi

View check run for this annotation

Codecov / codecov/patch

lib/helpview.gi#L284 

Added line #L284 was not covered by tests
end
);

# html version with w3m
HELP_VIEWER_INFO.w3m := rec(
type := "url",
show := function(url)
Exec(Concatenation("w3m \"", url, "\""));
Exec2("w3m", url);

Check warning on line 292 in lib/helpview.gi

View check run for this annotation

Codecov / codecov/patch

lib/helpview.gi#L292 

Added line #L292 was not covered by tests
end
);

HELP_VIEWER_INFO.elinks := rec(
type := "url",
show := function(url)
Exec(Concatenation("elinks \"", url, "\""));
Exec2("elinks", url);

Check warning on line 299 in lib/helpview.gi

View check run for this annotation

Codecov / codecov/patch

lib/helpview.gi#L299 

Added line #L299 was not covered by tests
end
);

HELP_VIEWER_INFO.links2ng := rec(
type := "url",
show := function(url)
Exec(Concatenation("links2 \"", url, "\""));
Exec2("links2", url);

Check warning on line 306 in lib/helpview.gi

View check run for this annotation

Codecov / codecov/patch

lib/helpview.gi#L306 

Added line #L306 was not covered by tests
end
);

HELP_VIEWER_INFO.links2 := rec(
type := "url",
show := function(url)
Exec(Concatenation("links2 -g \"", url, "\""));
Exec2("links2", "-g", url);

Check warning on line 313 in lib/helpview.gi

View check run for this annotation

Codecov / codecov/patch

lib/helpview.gi#L313 

Added line #L313 was not covered by tests
end
);
fi;
Expand Down
2 changes: 2 additions & 0 deletions lib/process.gd
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -186,3 +186,5 @@ DeclareOperation( "Process",
## <#/GAPDoc>
##
DeclareGlobalFunction( "Exec" );

DeclareGlobalName( "Exec2" );
74 changes: 74 additions & 0 deletions lib/process.gi
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -261,3 +261,77 @@
Process( dir, shell, InputTextUser(), OutputTextUser(), [ cs, cmd ] );

end );

# TODO: document this, come up with a better name, write some tests...
BindGlobal( "Exec2", function( arg )
local args, result, a, input, output, dir, cmd;

args := [];
result := rec();

Check warning on line 270 in lib/process.gi

View check run for this annotation

Codecov / codecov/patch

lib/process.gi#L269-L270 

Added lines #L269 - L270 were not covered by tests

# parse the inputs
for a in arg do
if IsDirectory(a) then
if IsBound(dir) then
Error("must specify at most one working directory");
fi;
dir := a;
elif IsInputStream(a) then
Copy link
Member Author

Choose a reason for hiding this comment

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

Right now input and output stream can come at any position and in any order. Perhaps I should make this code stricter, and require that they come in a specific place (canonical places: 1. as the first two arguments; 2. after the command name; 3. as the last two arguments)

Copy link
Contributor

Choose a reason for hiding this comment

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

While it would be a bigger change, I wonder if the input should be a record, with keys like input :=, output := .. this would mean in future it would be easier to do things like add stderr support, and make it clear which output stream is stdout and which is stderr, things like that.

Copy link
Member Author

Choose a reason for hiding this comment

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

I like that idea. It could then also be made backwards compatible: if there is a single argument which is a record, then use the new code; otherwise call the old code.

Copy link
Member Author

Choose a reason for hiding this comment

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

What I don't like about it is that for simple uses, it is much more verbose. Compare: e.g.:

old code:

Exec( Concatenation("start ", winfilename ) );

this PR:

Exec2( "start", winfilename );

with a record (record entries subject to change, of course):

Exec(rec(cmd:= "start", args := [ winfilename ] );

A compromise would be to only use a rec for the "extra" arguments like input, output, etc., like so:

Exec2( "start", winfilename, rec( input := BLAH, output := BLEH ) );

but then of course I can't use this to differentiate old and new callers.

if IsBound(input) then
Error("must specify at most one input stream");
fi;
input := a;
elif IsOutputStream(a) then
if IsBound(output) then
Error("must specify at most one output stream");
fi;
output := a;
elif IsString(a) then
ConvertToStringRep(a);
if not IsBound(cmd) then
cmd := a;

Check warning on line 292 in lib/process.gi

View check run for this annotation

Codecov / codecov/patch

lib/process.gi#L274-L292 

Added lines #L274 - L292 were not covered by tests
else
Add(args, a);
fi;
else
Error("unsupported argument type");
fi;
od;

Check warning on line 299 in lib/process.gi

View check run for this annotation

Codecov / codecov/patch

lib/process.gi#L294-L299 

Added lines #L294 - L299 were not covered by tests

if not IsBound(cmd) then
Error("must specify a command to execute");
fi;

Check warning on line 303 in lib/process.gi

View check run for this annotation

Codecov / codecov/patch

lib/process.gi#L301-L303 

Added lines #L301 - L303 were not covered by tests

# determine full executable path if it is not already a path
if not '/' in cmd then
a := Filename( DirectoriesSystemPrograms(), cmd );
if a = fail and ARCH_IS_WINDOWS() then
a := Filename( DirectoriesSystemPrograms(), Concatenation( cmd, ".exe" ) );
fi;
if a = fail then
Error("could not locate executable for '", cmd, "'");
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps, here, just fallback to the previousExec? Then you can rename Exec2 into Exec since it extends the previous command

Copy link
Member Author

Choose a reason for hiding this comment

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

No, that would not be backwards compatible at all. E.g. this would change its behavior: Exec("ls", "-l", filename, ">tmpfile");

fi;
cmd := a;
fi;

Check warning on line 315 in lib/process.gi

View check run for this annotation

Codecov / codecov/patch

lib/process.gi#L306-L315 

Added lines #L306 - L315 were not covered by tests

# set default working directory if necessary
if not IsBound(dir) then
dir := DirectoryCurrent();
fi;

Check warning on line 320 in lib/process.gi

View check run for this annotation

Codecov / codecov/patch

lib/process.gi#L318-L320 

Added lines #L318 - L320 were not covered by tests

# if no input stream was specified, pass no input to the command
if not IsBound(input) then
input := InputTextNone();
fi;

Check warning on line 325 in lib/process.gi

View check run for this annotation

Codecov / codecov/patch

lib/process.gi#L323-L325 

Added lines #L323 - L325 were not covered by tests

# if no output stream was specified, put output into the returned record
if not IsBound(output) then
result.output := "";
output := OutputTextString(result.output, false);
fi;

Check warning on line 331 in lib/process.gi

View check run for this annotation

Codecov / codecov/patch

lib/process.gi#L328-L331 

Added lines #L328 - L331 were not covered by tests

# execute the command
result.status := Process( dir, cmd, input, output, args );

Check warning on line 334 in lib/process.gi

View check run for this annotation

Codecov / codecov/patch

lib/process.gi#L334 

Added line #L334 was not covered by tests

return result;
end );

Check warning on line 337 in lib/process.gi

View check run for this annotation

Codecov / codecov/patch

lib/process.gi#L336-L337 

Added lines #L336 - L337 were not covered by tests
11 changes: 6 additions & 5 deletions lib/streams.gi
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1311,19 +1311,20 @@
InstallGlobalFunction( OpenExternal, function(filename)
local file;
if ARCH_IS_MAC_OS_X() then
Exec(Concatenation("open \"",filename,"\""));
Exec2("open", filename);

Check warning on line 1314 in lib/streams.gi

View check run for this annotation

Codecov / codecov/patch

lib/streams.gi#L1314 

Added line #L1314 was not covered by tests
elif ARCH_IS_WINDOWS() then
Exec(Concatenation("cmd /c start \"",filename,"\""));
Exec2("cmd", "/c", "start", filename);

Check warning on line 1316 in lib/streams.gi

View check run for this annotation

Codecov / codecov/patch

lib/streams.gi#L1316 

Added line #L1316 was not covered by tests
elif ARCH_IS_WSL() then
# If users pass a URL, make sure if does not get mangled.
if ForAny(["https://", "http://"], {pre} -> StartsWith(filename, pre)) then
file := filename;
else
file := Concatenation("$(wslpath -a -w \"",filename,"\")");
file := "";
Exec2("wslpath", "-a", "-w", filename, OutputTextString(file, false));

Check warning on line 1323 in lib/streams.gi

View check run for this annotation

Codecov / codecov/patch

lib/streams.gi#L1322-L1323 

Added lines #L1322 - L1323 were not covered by tests
fi;
Exec(Concatenation("explorer.exe \"", file, "\""));
Exec2("explorer.exe", file);

Check warning on line 1325 in lib/streams.gi

View check run for this annotation

Codecov / codecov/patch

lib/streams.gi#L1325 

Added line #L1325 was not covered by tests
else
Exec(Concatenation("xdg-open \"",filename,"\""));
Exec2("xdg-open", filename);

Check warning on line 1327 in lib/streams.gi

View check run for this annotation

Codecov / codecov/patch

lib/streams.gi#L1327 

Added line #L1327 was not covered by tests
fi;
end );

Expand Down
Loading