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.

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

Added expand to toplevel. #864

Merged
merged 18 commits into from Nov 7, 2016

Conversation

Projects
None yet
2 participants
@bschommer
Contributor

bschommer commented Oct 19, 2016

The toplevel now also accepts -args and -args0. In order to avoid
problems with the override_sys_argv hack no script file is allowed in
expand options.

@gasche

This comment has been minimized.

Show comment
Hide comment
@gasche

gasche Oct 19, 2016

Member

Thanks, this is very nice. I'm in favor of merging.

Would it be possible to have a testsuite check for this functionality, testing in particular the failure case with a file in the expansion? tests/tool-toplevel does not allow to pass command-line arguments, but tests/tool-ocamldoc-* do; you could probably copy the latter organization in a new tests/tool-toplevel-invocation directory.

Member

gasche commented Oct 19, 2016

Thanks, this is very nice. I'm in favor of merging.

Would it be possible to have a testsuite check for this functionality, testing in particular the failure case with a file in the expansion? tests/tool-toplevel does not allow to pass command-line arguments, but tests/tool-ocamldoc-* do; you could probably copy the latter organization in a new tests/tool-toplevel-invocation directory.

@gasche gasche added the approved label Oct 19, 2016

@bschommer

This comment has been minimized.

Show comment
Hide comment
@bschommer

bschommer Oct 25, 2016

Contributor

I added a test for the failing case.

Contributor

bschommer commented Oct 25, 2016

I added a test for the failing case.

@bschommer

This comment has been minimized.

Show comment
Hide comment
@bschommer

bschommer Nov 2, 2016

Contributor

I added also a working test case. Also the using a .ml file in an response file while now raise Arg.Bad and thus result in printing the help.

Contributor

bschommer commented Nov 2, 2016

I added also a working test case. Also the using a .ml file in an response file while now raise Arg.Bad and thus result in printing the help.

Show outdated Hide outdated testsuite/tests/tool-toplevel-invocation/Makefile
@for file in *.txt; do \
TERM=dumb $(OCAML) -args $$file < test.ml 2>&1 \
| grep -v '^ OCaml version' \
| grep -v ' Script file is not allowed in expanded option.' > $$file.result; \

This comment has been minimized.

@gasche

gasche Nov 2, 2016

Member

could you turn this into >> $$file.result, with an echo "Script file is not allowed in expanded option." > $$file.result; \ before that? It is a bit silly, but it help users make sense of what the test is testing by looking at the result file, without having to look at the Makefile.

@gasche

gasche Nov 2, 2016

Member

could you turn this into >> $$file.result, with an echo "Script file is not allowed in expanded option." > $$file.result; \ before that? It is a bit silly, but it help users make sense of what the test is testing by looking at the result file, without having to look at the Makefile.

This comment has been minimized.

@bschommer

bschommer Nov 2, 2016

Contributor

I used sed instead to just remove the prefix from the error message.

@bschommer

bschommer Nov 2, 2016

Contributor

I used sed instead to just remove the prefix from the error message.

Show outdated Hide outdated testsuite/tests/tool-toplevel-invocation/test.ml
@@ -0,0 +1 @@
printf "Test succeds\n";;

This comment has been minimized.

@gasche

gasche Nov 2, 2016

Member

succeeds?

@gasche

gasche Nov 2, 2016

Member

succeeds?

@gasche gasche removed the approved label Nov 4, 2016

@gasche

This comment has been minimized.

Show comment
Hide comment
@gasche

gasche Nov 4, 2016

Member

I think I (or someone else) need to review the patch again (and in more details), it's not so clear to me anymore how !expand_offset works.

Member

gasche commented Nov 4, 2016

I think I (or someone else) need to review the patch again (and in more details), it's not so clear to me anymore how !expand_offset works.

@bschommer

This comment has been minimized.

Show comment
Hide comment
@bschommer

bschommer Nov 5, 2016

Contributor

I think I (or someone else) need to review the patch again (and in more details), it's not so clear to me anymore how !expand_offset works.

The expand_offset variable is used to take track of the last part of argv was inserted via an Expand argument. So if Expand is parsed the current position is stored as well as the length of the inserted array. If this Expand itself comes from a responsfile the length of the current expand_offset is just increased by the length of the new array.

Contributor

bschommer commented Nov 5, 2016

I think I (or someone else) need to review the patch again (and in more details), it's not so clear to me anymore how !expand_offset works.

The expand_offset variable is used to take track of the last part of argv was inserted via an Expand argument. So if Expand is parsed the current position is stored as well as the length of the inserted array. If this Expand itself comes from a responsfile the length of the current expand_offset is just increased by the length of the new array.

@bschommer

This comment has been minimized.

Show comment
Hide comment
@bschommer

bschommer Nov 5, 2016

Contributor

I need to rework the tests again, the current test would require updates to the test every time the help of the toplevel is changed.

Contributor

bschommer commented Nov 5, 2016

I need to rework the tests again, the current test would require updates to the test every time the help of the toplevel is changed.

@gasche

This comment has been minimized.

Show comment
Hide comment
@gasche

gasche Nov 5, 2016

Member

One way to achieve this would be to have this failure mode not behave as Arg.Bad. I think Arg.Bad makes perfect sense, but on the other hand the error that is being complained about is not really about an ill-formatted command-line parameter, and showing the --help output does not really clarify it. (This behavior is fine if it's simpler or more natural to implement, but given that you had to add your own exception-catching logic in the patch I'm not sure without looking at the details.)

Member

gasche commented Nov 5, 2016

One way to achieve this would be to have this failure mode not behave as Arg.Bad. I think Arg.Bad makes perfect sense, but on the other hand the error that is being complained about is not really about an ill-formatted command-line parameter, and showing the --help output does not really clarify it. (This behavior is fine if it's simpler or more natural to implement, but given that you had to add your own exception-catching logic in the patch I'm not sure without looking at the details.)

bschommer added some commits Oct 19, 2016

Added expand to toplevel.
The toplevel now also accepts -args and -args0. In order to avoid
problems with the overide_args hack now script file is allowed in
expand options.
Added some test for broken -args for toplevel.
The test checks whether the toplevel fails if the script file is passed
via args option.
Updated error case.
Instead of printing the error string, Arg.Bad is raised and a wrapper is
added around the parse_and_expand_dynamic_argv.
Added new tests and updated documentation.
A script file in a responsefile now only prints an error message instead
of the help.
@bschommer

This comment has been minimized.

Show comment
Hide comment
@bschommer

bschommer Nov 6, 2016

Contributor

I rebased everything and adopted the documentation. The error string now does not print the help. Also there is a test for the indirection case.

Contributor

bschommer commented Nov 6, 2016

I rebased everything and adopted the documentation. The error string now does not print the help. Also there is a test for the indirection case.

Show outdated Hide outdated manual/manual/cmds/unified-options.etex
Read additional newline-terminated command line arguments from \var{filename}.
Read additional newline-terminated command line arguments from \var{filename}.
\top{Read additional newline-terminated command line arguments from \var{filename}.

This comment has been minimized.

@gasche

gasche Nov 6, 2016

Member

If I read correctly, the line Read additional... will be repeated twice in the toplevel section, as it is present both without any condition and in the toplevelconditional. I think that only It is not possible... should be there, and similarly for Read additional... / It is not possible... below.

@gasche

gasche Nov 6, 2016

Member

If I read correctly, the line Read additional... will be repeated twice in the toplevel section, as it is present both without any condition and in the toplevelconditional. I think that only It is not possible... should be there, and similarly for Read additional... / It is not possible... below.

Show outdated Hide outdated toplevel/opttopmain.ml
@@ -20,6 +20,26 @@ let usage =
let preload_objects = ref []
(* Position plus length of last expand argument *)
let expand_offset: (int * int) option ref = ref None

This comment has been minimized.

@gasche

gasche Nov 7, 2016

Member

I re-reviewed the code and it seems correct to me, but a bit complicated. We could add more comments to explain, but I wonder if there is not a simpler way. For example, why would it not be enough to only keep the limit expanded arguments (what you call "position + length")? I'm not sure what the position is for; it would be useful if positions could decrease over time, but is that possible?

@gasche

gasche Nov 7, 2016

Member

I re-reviewed the code and it seems correct to me, but a bit complicated. We could add more comments to explain, but I wonder if there is not a simpler way. For example, why would it not be enough to only keep the limit expanded arguments (what you call "position + length")? I'm not sure what the position is for; it would be useful if positions could decrease over time, but is that possible?

This comment has been minimized.

@bschommer

bschommer Nov 7, 2016

Contributor

I updated the code. It is actually simpler to just store the last position of the current expanded array.

@bschommer

bschommer Nov 7, 2016

Contributor

I updated the code. It is actually simpler to just store the last position of the current expanded array.

Simplified expand logic.
We only remember where the current last expanded option is.
Show outdated Hide outdated toplevel/opttopmain.ml
let pos = match !last_expand_pos with
| Some opos when pos < opos ->
opos+len (* Already expand option just shift the position *)
| _ -> pos + len + 1 in (* New last position *)

This comment has been minimized.

@gasche

gasche Nov 7, 2016

Member

Should that not be pos + len - 1 instead? I'm not sure what len + 1 means in this context.

I think that you could store the outer limit of all expansions, that is the first position that is not part of an expansion (that would be pos + len, if I understand correctly). In that case, you would not need to use a int option, because you could initialize first_nonexpanded_pos with ref 0, and the code would be even more straightforward.

@gasche

gasche Nov 7, 2016

Member

Should that not be pos + len - 1 instead? I'm not sure what len + 1 means in this context.

I think that you could store the outer limit of all expansions, that is the first position that is not part of an expansion (that would be pos + len, if I understand correctly). In that case, you would not need to use a int option, because you could initialize first_nonexpanded_pos with ref 0, and the code would be even more straightforward.

This comment has been minimized.

@bschommer

bschommer Nov 7, 2016

Contributor

The +1 is because the current index is at this point the file name. Like all other spec functions consume_arg is only called after the function is called. So for the first_non_expanded_pos it would be pos + len + 2.

@bschommer

bschommer Nov 7, 2016

Contributor

The +1 is because the current index is at this point the file name. Like all other spec functions consume_arg is only called after the function is called. So for the first_non_expanded_pos it would be pos + len + 2.

Arg.parse_and_expand_argv_dynamic current argv list file_argument usage;
with
| Arg.Bad msg -> Format.fprintf Format.err_formatter "%s%!" msg; exit 2
| Arg.Help msg -> Format.fprintf Format.std_formatter "%s%!" msg; exit 0

This comment has been minimized.

@gasche

gasche Nov 7, 2016

Member

Apologies for being very slow in going through my review, but: why do we have this new exception-handling code here? The new failure mode you added does exit 2 directly, if I understand correctly, so is there still a reason for this?

@gasche

gasche Nov 7, 2016

Member

Apologies for being very slow in going through my review, but: why do we have this new exception-handling code here? The new failure mode you added does exit 2 directly, if I understand correctly, so is there still a reason for this?

This comment has been minimized.

@bschommer

bschommer Nov 7, 2016

Contributor

It is the same as in Arg.parse which was called before. For the other expand options it is possible to use Arg.parse_expand which does make the array and the current visible to the outside.

@bschommer

bschommer Nov 7, 2016

Contributor

It is the same as in Arg.parse which was called before. For the other expand options it is possible to use Arg.parse_expand which does make the array and the current visible to the outside.

This comment has been minimized.

@gasche

gasche Nov 7, 2016

Member

So if I understand correctly, whether an Arg.parse*-family function will handle errors internally or not depends on whether argv appears in the name of the function. This is not a very clear API... Is there anything we can do to fix it, besides adding a clear warning about this in args.mli?

@gasche

gasche Nov 7, 2016

Member

So if I understand correctly, whether an Arg.parse*-family function will handle errors internally or not depends on whether argv appears in the name of the function. This is not a very clear API... Is there anything we can do to fix it, besides adding a clear warning about this in args.mli?

This comment has been minimized.

@bschommer

bschommer Nov 7, 2016

Contributor

I think all functions working on Sys.argv contain the error handling, all other functions not.

@bschommer

bschommer Nov 7, 2016

Contributor

I think all functions working on Sys.argv contain the error handling, all other functions not.

Show outdated Hide outdated toplevel/opttopmain.ml
check in override arguments may fail since the new argv can be larger
than the original argv.
*)
Printf.eprintf "Script file is not allowed in expand option";

This comment has been minimized.

@gasche

gasche Nov 7, 2016

Member

We could clarify this failure message as follows:

"For implementation reasons, the toplevel does not support having script files (here %S) inside expanded arguments passed through the -args{,0} command-line option.\n" name

@gasche

gasche Nov 7, 2016

Member

We could clarify this failure message as follows:

"For implementation reasons, the toplevel does not support having script files (here %S) inside expanded arguments passed through the -args{,0} command-line option.\n" name

This comment has been minimized.

@bschommer

bschommer Nov 7, 2016

Contributor

Fine with me.

@bschommer

bschommer Nov 7, 2016

Contributor

Fine with me.

@gasche

This comment has been minimized.

Show comment
Hide comment
@gasche

gasche Nov 7, 2016

Member

I think we've looked at this PR enough and would be in favor of merging it soon. I plan to squash all your commits in a single merged commit (currently there are many back-and-forth commit), because I think the total changes make sense as a single commit (having the testsuite with the feature itself is fine by me); if you would prefer to rebase the commit set yourself, feel free to do it.

Member

gasche commented Nov 7, 2016

I think we've looked at this PR enough and would be in favor of merging it soon. I plan to squash all your commits in a single merged commit (currently there are many back-and-forth commit), because I think the total changes make sense as a single commit (having the testsuite with the feature itself is fine by me); if you would prefer to rebase the commit set yourself, feel free to do it.

@gasche gasche merged commit bc81c31 into ocaml:trunk Nov 7, 2016

1 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details

@bschommer bschommer deleted the bschommer:toploop-responsefile branch Nov 7, 2016

@gasche

This comment has been minimized.

Show comment
Hide comment
@gasche

gasche Nov 7, 2016

Member

Few! Merged. Thanks a lot, and sorry for the myriad of little comments. I think the result is much better.

Member

gasche commented Nov 7, 2016

Few! Merged. Thanks a lot, and sorry for the myriad of little comments. I think the result is much better.

@bschommer

This comment has been minimized.

Show comment
Hide comment
@bschommer

bschommer Nov 7, 2016

Contributor

No problem. I will have a look at the remaining tools in the next few days.

Contributor

bschommer commented Nov 7, 2016

No problem. I will have a look at the remaining tools in the next few days.

camlspotter pushed a commit to camlspotter/ocaml that referenced this pull request Oct 17, 2017

Added expand to toplevel. (#864)
* Added expand to toplevel.

The toplevel now also accepts -args and -args0. In order to avoid
problems with the overide_args hack now script file is allowed in
expand options.

* Fixed differences between .ml and .mli

* Added missing expand in opttopmain.

* Added some test for broken -args for toplevel.

The test checks whether the toplevel fails if the script file is passed
via args option.

* Corrected test case.

* Updated error case.

Instead of printing the error string, Arg.Bad is raised and a wrapper is
added around the parse_and_expand_dynamic_argv.

* Added begin ... end around try ... with.

* Added working example an strip error path.

* Use sed to remove path and fixed typo.

* Added documentation.

* Also fix typo in reference file.

* Added PR to the corresponding change entry.

* Reworked Changes entry.

* Added new tests and updated documentation.

A script file in a responsefile now only prints an error message instead
of the help.

* Removed duplicated entry.

* Simplified expand logic.

We only remember where the current last expanded option is.

* Use first non_expand position instead.

* Updated error message.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment