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

[WIP] Implement responsefiles support PR7050 #843

Merged
merged 6 commits into from Oct 12, 2016

Conversation

Projects
None yet
4 participants
@bschommer
Contributor

bschommer commented Oct 10, 2016

Follow up to PR #748 against trunk.

Added functions to read/write cmdline arguments
The function read_arg, read_arg0, write_arg and write_arg0 read
and write command line arguments to a file which are either
separated by a NUL character or by new lines.
@gasche

This comment has been minimized.

Show comment
Hide comment
@gasche

gasche Oct 10, 2016

Member

This PR implements functions for parsing of argument files. Do you plan to eventually add commits that combine this with the new Expand support to actually provide -args/args0 arguments to the compiler tools, or would that be out of the scope of the current PR?

Member

gasche commented Oct 10, 2016

This PR implements functions for parsing of argument files. Do you plan to eventually add commits that combine this with the new Expand support to actually provide -args/args0 arguments to the compiler tools, or would that be out of the scope of the current PR?

Added -arg/-args0 to compiler cmd arguments
The -arg/-args0 switch allow it to pass additional command line
arguments in files newline/NUL separated.
@bschommer

This comment has been minimized.

Show comment
Hide comment
@bschommer

bschommer Oct 10, 2016

Contributor

I added the functions to the compiler command line arguments.

Contributor

bschommer commented Oct 10, 2016

I added the functions to the compiler command line arguments.

@damiendoligez

I've added some change suggestions.

About the scope: I think it makes sense to add the options to all tools.

Show outdated Hide outdated driver/main_args.ml
@@ -724,6 +724,19 @@ let mk_no_strict_formats f =
\ and instead fix invalid formats.)"
;;
let mk_expand_responsefile f =

This comment has been minimized.

@damiendoligez

damiendoligez Oct 11, 2016

Member

Please respect the format: these functions have the same name as the options they describe, with dashes replaced by underscores, so this should be mk_args and the next one should be mk_args0.

@damiendoligez

damiendoligez Oct 11, 2016

Member

Please respect the format: these functions have the same name as the options they describe, with dashes replaced by underscores, so this should be mk_args and the next one should be mk_args0.

This comment has been minimized.

@bschommer

bschommer Oct 11, 2016

Contributor

Is changed.

@bschommer

bschommer Oct 11, 2016

Contributor

Is changed.

Show outdated Hide outdated driver/main_args.ml
@@ -813,6 +826,9 @@ module type Compiler_options = sig
val _nopervasives : unit -> unit
val _dtimings : unit -> unit
val _expand_responsefile: string -> string array

This comment has been minimized.

@damiendoligez

damiendoligez Oct 11, 2016

Member

Same remark about the function names.

@damiendoligez

damiendoligez Oct 11, 2016

Member

Same remark about the function names.

This comment has been minimized.

@bschommer

bschommer Oct 11, 2016

Contributor

Is changed.

@bschommer

bschommer Oct 11, 2016

Contributor

Is changed.

Show outdated Hide outdated stdlib/arg.ml
try
let c = input_char ic in
if c = sep then begin
stash inw; read true

This comment has been minimized.

@damiendoligez

damiendoligez Oct 11, 2016

Member

This should be read false, right?
In fact you should drop inw altogether because we decided we want to allow empty arguments. Just add a test at EOF to stash only when the buffer is not empty.

@damiendoligez

damiendoligez Oct 11, 2016

Member

This should be read false, right?
In fact you should drop inw altogether because we decided we want to allow empty arguments. Just add a test at EOF to stash only when the buffer is not empty.

This comment has been minimized.

@bschommer

bschommer Oct 11, 2016

Contributor

Is changed.

@bschommer

bschommer Oct 11, 2016

Contributor

Is changed.

Show outdated Hide outdated stdlib/arg.ml
first:=false
else
output_char oc sep in
Array.iter (fun s -> sep (); output_string oc s) args;

This comment has been minimized.

@damiendoligez

damiendoligez Oct 11, 2016

Member

Get rid of the ugly first reference and output a "separator" after each arg. Formally, they are terminators, with the last one optional.

@damiendoligez

damiendoligez Oct 11, 2016

Member

Get rid of the ugly first reference and output a "separator" after each arg. Formally, they are terminators, with the last one optional.

This comment has been minimized.

@bschommer

bschommer Oct 11, 2016

Contributor

Is changed.

@bschommer

bschommer Oct 11, 2016

Contributor

Is changed.

Show outdated Hide outdated stdlib/arg.mli
@@ -173,3 +173,19 @@ val current : int ref
{!Arg.parse} uses the initial value of {!Arg.current} as the index of
argument 0 (the program name) and starts parsing arguments
at the next element. *)
val read_arg: string -> string array
(** [Arg.read_arg file] reads new line separated command line arguments from

This comment has been minimized.

@damiendoligez

damiendoligez Oct 11, 2016

Member

According to your implementation, they are not "new line separated" but "linefeed-separated". What happened to the suggestion of ignoring CR in this case?

Also, this should be terminated rather than separated.

@damiendoligez

damiendoligez Oct 11, 2016

Member

According to your implementation, they are not "new line separated" but "linefeed-separated". What happened to the suggestion of ignoring CR in this case?

Also, this should be terminated rather than separated.

This comment has been minimized.

@bschommer

bschommer Oct 11, 2016

Contributor

Is changed.

@bschommer

bschommer Oct 11, 2016

Contributor

Is changed.

Show outdated Hide outdated stdlib/arg.mli
file [file]. *)
val read_arg0: string -> string array
(** Identical to {!Arg.read_arg} but assumes NUL separated command line

This comment has been minimized.

@damiendoligez

damiendoligez Oct 11, 2016

Member

Same remark about separated vs terminated.

@damiendoligez

damiendoligez Oct 11, 2016

Member

Same remark about separated vs terminated.

This comment has been minimized.

@bschommer

bschommer Oct 11, 2016

Contributor

Is changed.

@bschommer

bschommer Oct 11, 2016

Contributor

Is changed.

Show outdated Hide outdated stdlib/arg.mli
arguments *)
val write_arg: string -> string array -> unit
(** [Arg.write_arg file args] writes the arguments [args] newline separated into

This comment has been minimized.

@damiendoligez

damiendoligez Oct 11, 2016

Member

terminated

@damiendoligez

damiendoligez Oct 11, 2016

Member

terminated

This comment has been minimized.

@bschommer

bschommer Oct 11, 2016

Contributor

Is changed.

@bschommer

bschommer Oct 11, 2016

Contributor

Is changed.

Show outdated Hide outdated stdlib/arg.mli
use the function {!Args.write_arg0} instead *)
val write_arg0: string -> string array -> unit
(** Identical to {!Arg.write_arg} but uses NUL as separator instead *)

This comment has been minimized.

@damiendoligez

damiendoligez Oct 11, 2016

Member

terminator

@damiendoligez

damiendoligez Oct 11, 2016

Member

terminator

This comment has been minimized.

@bschommer

bschommer Oct 11, 2016

Contributor

Is changed.

@bschommer

bschommer Oct 11, 2016

Contributor

Is changed.

;;
test_rw args1;;
test_rw args2;;

This comment has been minimized.

@damiendoligez

damiendoligez Oct 11, 2016

Member

You should also test with an empty argument list and a list that contains empty arguments.

@damiendoligez

damiendoligez Oct 11, 2016

Member

You should also test with an empty argument list and a list that contains empty arguments.

This comment has been minimized.

@bschommer

bschommer Oct 11, 2016

Contributor

Is added.

@bschommer

bschommer Oct 11, 2016

Contributor

Is added.

Trim CR, remove inw parameter and updated docu.
The read_aux function trims the cr character for the new-line
based method and the write functions are simplified without an
additional sep function.

The argument names are changed to match the default naming format
and the documentation is updated to match the implementation
behavior.

Also two new tests for reading and writing empty command lines as
well as a command lines containing empty arguments are added.
@bschommer

This comment has been minimized.

Show comment
Hide comment
@bschommer

bschommer Oct 11, 2016

Contributor

For the other tools: I will have a look but as @diml noted ocamldebug for example needs special care.

Contributor

bschommer commented Oct 11, 2016

For the other tools: I will have a look but as @diml noted ocamldebug for example needs special care.

Added simple parse_expand and support ocamldep
The parse_expand function does the same as parse and parse_dynamic
but allows Expand. However it does not update current to avoid
the problem that the array is modified.

This function is used for the ocamlopt, ocamlc, ocamlcp,
ocamloptp and ocamldep.
@bschommer

This comment has been minimized.

Show comment
Hide comment
@bschommer

bschommer Oct 12, 2016

Contributor

I added ocamldep to the list of supported tools. I also added a simple wrapper around parse_and_expand_argv_dynamic that also prints error/help messages to Arg. In order to avoid the problems with current and the changed array the function does not modify Arg.current is not modified.

Contributor

bschommer commented Oct 12, 2016

I added ocamldep to the list of supported tools. I also added a simple wrapper around parse_and_expand_argv_dynamic that also prints error/help messages to Arg. In order to avoid the problems with current and the changed array the function does not modify Arg.current is not modified.

Added responsefile support for various tools.
Now also the tools dumpobj, objinfo, ocamlprof, primreq and
read_cmt accept response files.
@damiendoligez

This comment has been minimized.

Show comment
Hide comment
@damiendoligez

damiendoligez Oct 12, 2016

Member

For the other tools: I will have a look but as @diml noted ocamldebug for example needs special care.

I did not mean to push hard for supporting all tools, but since you asked about scope (in #748 (comment)) I wanted to give a positive answer. I'd be happy with a partial implementation anyway.

Member

damiendoligez commented Oct 12, 2016

For the other tools: I will have a look but as @diml noted ocamldebug for example needs special care.

I did not mean to push hard for supporting all tools, but since you asked about scope (in #748 (comment)) I wanted to give a positive answer. I'd be happy with a partial implementation anyway.

@damiendoligez

Looks OK to me now.

@bschommer

This comment has been minimized.

Show comment
Hide comment
@bschommer

bschommer Oct 12, 2016

Contributor

List of missing tools/programs:
-ocaml
-ocamldebug
-ocamlyacc
-ocamlex
-ocamldoc
-cvt_emit
-cmpbyt
-ocamlmklib
-ocamlmktop
-stripdebug

Contributor

bschommer commented Oct 12, 2016

List of missing tools/programs:
-ocaml
-ocamldebug
-ocamlyacc
-ocamlex
-ocamldoc
-cvt_emit
-cmpbyt
-ocamlmklib
-ocamlmktop
-stripdebug

@bschommer

This comment has been minimized.

Show comment
Hide comment
@bschommer

bschommer Oct 12, 2016

Contributor

For stripdebug, cvt_emit, cmpbyt and ocamlyacc, ocamllex it does not make sense to add it since they expect a constant number of options.

ocamlmktop only passes the commands on.

ocamldebug, ocaml, ocamldoc and ocamlmklib all have either hand written parse functions or require some preprocessing of the arguments and I'm unsure if it works for them without problem.

Contributor

bschommer commented Oct 12, 2016

For stripdebug, cvt_emit, cmpbyt and ocamlyacc, ocamllex it does not make sense to add it since they expect a constant number of options.

ocamlmktop only passes the commands on.

ocamldebug, ocaml, ocamldoc and ocamlmklib all have either hand written parse functions or require some preprocessing of the arguments and I'm unsure if it works for them without problem.

@bschommer

This comment has been minimized.

Show comment
Hide comment
@bschommer

bschommer Oct 12, 2016

Contributor

I did not mean to push hard for supporting all tools, but since you asked about scope (in #748 (comment)) I wanted to give a positive answer. I'd be happy with a partial implementation anyway.

No problem, most of them are pretty straightforward.

Contributor

bschommer commented Oct 12, 2016

I did not mean to push hard for supporting all tools, but since you asked about scope (in #748 (comment)) I wanted to give a positive answer. I'd be happy with a partial implementation anyway.

No problem, most of them are pretty straightforward.

@gasche

This comment has been minimized.

Show comment
Hide comment
@gasche

gasche Oct 12, 2016

Member

I think that it would be nice to have support in ocaml and ocamlnat as well (you may have noticed that I mistakenly advertised it to Coq developers; I thought that the functorized argument-parsing machinery would provide the feature for the toplevels as well). It could be useful for the Coq use-case, as it allows as a debugging technique to "Drop" to the OCaml toplevel.

(Command-line is in the files toplevel/topmain.ml (for ocaml) and toplevel/opttopmain.ml (for ocamlnat). The native toplevel is not officially supported but it's on the path to redemption these days, so it's nice to keep it in synch.)

Member

gasche commented Oct 12, 2016

I think that it would be nice to have support in ocaml and ocamlnat as well (you may have noticed that I mistakenly advertised it to Coq developers; I thought that the functorized argument-parsing machinery would provide the feature for the toplevels as well). It could be useful for the Coq use-case, as it allows as a debugging technique to "Drop" to the OCaml toplevel.

(Command-line is in the files toplevel/topmain.ml (for ocaml) and toplevel/opttopmain.ml (for ocamlnat). The native toplevel is not officially supported but it's on the path to redemption these days, so it's nice to keep it in synch.)

@bschommer

This comment has been minimized.

Show comment
Hide comment
@bschommer

bschommer Oct 12, 2016

Contributor

I will have a look at ocaml and ocamlnat.

Contributor

bschommer commented Oct 12, 2016

I will have a look at ocaml and ocamlnat.

@bschommer

This comment has been minimized.

Show comment
Hide comment
@bschommer

bschommer Oct 12, 2016

Contributor

It seems like it is not possible without some unsafe operations:

let override_sys_argv args =
  let len = Array.length args in
  if Array.length Sys.argv < len then invalid_arg "Toploop.override_sys_argv";
  Array.blit args 0 Sys.argv 0 len;
  Obj.truncate (Obj.repr Sys.argv) len;
  Arg.current := 0
Contributor

bschommer commented Oct 12, 2016

It seems like it is not possible without some unsafe operations:

let override_sys_argv args =
  let len = Array.length args in
  if Array.length Sys.argv < len then invalid_arg "Toploop.override_sys_argv";
  Array.blit args 0 Sys.argv 0 len;
  Obj.truncate (Obj.repr Sys.argv) len;
  Arg.current := 0
@gasche

This comment has been minimized.

Show comment
Hide comment
@gasche

gasche Oct 12, 2016

Member

The toplevel uses a horrible hack so that the first argument is the name of the executed file instead of the toplevel binary (I suppose). But how does this prevent handling Expand nodes in the argument array after this hack has been applied? I don't understand why there is a conflict here.

Member

gasche commented Oct 12, 2016

The toplevel uses a horrible hack so that the first argument is the name of the executed file instead of the toplevel binary (I suppose). But how does this prevent handling Expand nodes in the argument array after this hack has been applied? I don't understand why there is a conflict here.

@bschommer

This comment has been minimized.

Show comment
Hide comment
@bschommer

bschommer Oct 12, 2016

Contributor

override_sys_argv actually copies the rest of the un-parsed arguments into Sys.argv and then truncates Sys.argv. However Expand may increase the array beyond the length of Sys.argv and the copy would only work if the size is increased instead of making it smaller.

Contributor

bschommer commented Oct 12, 2016

override_sys_argv actually copies the rest of the un-parsed arguments into Sys.argv and then truncates Sys.argv. However Expand may increase the array beyond the length of Sys.argv and the copy would only work if the size is increased instead of making it smaller.

@gasche

This comment has been minimized.

Show comment
Hide comment
@gasche

gasche Oct 12, 2016

Member

So if I understand correctly, the toplevel parses the provided argument list, then it stops on the first anonymous argument that is a source file (.ml extension), and considers that the remaining arguments are to be processed by the script itself upon execution, so it rewrites Sys.argv in place to have just the remaining argument as the control is passed to the script.

As it rewrites the arguments, it checks that the new argument-list is smaller than the previous one (as it is supposed to be a suffix of it). This could fail in general if one of the Expand node would generate a source file argument. (Note that if the Expand nodes don't generate source files argument, this would succeed, as the suffix would remain the same).

I'm confident there is a reasonable solution (for example we could just disable the size check that is certainly there for debugging), but it also seems to require a bit more work than I hoped. Would you agree with merging the current PR, and eventually support the toplevel in a following PR?

(I'm not implying that you personally have to do the toplevel-related work. If I have time I could try to implement it myself. Let us know what you want to do.)

Member

gasche commented Oct 12, 2016

So if I understand correctly, the toplevel parses the provided argument list, then it stops on the first anonymous argument that is a source file (.ml extension), and considers that the remaining arguments are to be processed by the script itself upon execution, so it rewrites Sys.argv in place to have just the remaining argument as the control is passed to the script.

As it rewrites the arguments, it checks that the new argument-list is smaller than the previous one (as it is supposed to be a suffix of it). This could fail in general if one of the Expand node would generate a source file argument. (Note that if the Expand nodes don't generate source files argument, this would succeed, as the suffix would remain the same).

I'm confident there is a reasonable solution (for example we could just disable the size check that is certainly there for debugging), but it also seems to require a bit more work than I hoped. Would you agree with merging the current PR, and eventually support the toplevel in a following PR?

(I'm not implying that you personally have to do the toplevel-related work. If I have time I could try to implement it myself. Let us know what you want to do.)

@bschommer

This comment has been minimized.

Show comment
Hide comment
@bschommer

bschommer Oct 12, 2016

Contributor

That is fine with me.

Contributor

bschommer commented Oct 12, 2016

That is fine with me.

@gasche

This comment has been minimized.

Show comment
Hide comment
@gasche

gasche Oct 12, 2016

Member

Could you add a Changes entry? Suggestion:

### Compiler user-interface and warnings:

[...]

- MPR#7050, GPR#748 GPR#843: new `-arg/-arg0 <file>` parameters to provide
  extra command-line arguments in a file -- see documentation.
  User programs may implement similar options
  using the new `Arg.Expand` constructor.
  (Bernhard Schommer, review by Jérémie Dimino, Gabriel Scherer
   and Damien Doligez, discussion with Alain Frisch and Xavier Leroy,
   feature request from the Coq team)
Member

gasche commented Oct 12, 2016

Could you add a Changes entry? Suggestion:

### Compiler user-interface and warnings:

[...]

- MPR#7050, GPR#748 GPR#843: new `-arg/-arg0 <file>` parameters to provide
  extra command-line arguments in a file -- see documentation.
  User programs may implement similar options
  using the new `Arg.Expand` constructor.
  (Bernhard Schommer, review by Jérémie Dimino, Gabriel Scherer
   and Damien Doligez, discussion with Alain Frisch and Xavier Leroy,
   feature request from the Coq team)

@gasche gasche merged commit 9b08f08 into ocaml:trunk Oct 12, 2016

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@gasche

This comment has been minimized.

Show comment
Hide comment
@gasche

gasche Oct 12, 2016

Member

Thanks a lot for your hard work! Do you plan to look at the toplevel?

Member

gasche commented Oct 12, 2016

Thanks a lot for your hard work! Do you plan to look at the toplevel?

@bschommer

This comment has been minimized.

Show comment
Hide comment
@bschommer

bschommer Oct 12, 2016

Contributor

I will have a look at it. Basically it seems that instead of truncate one needs some kind of adjust primitive. When I find a solution I will open a new PR.

Contributor

bschommer commented Oct 12, 2016

I will have a look at it. Basically it seems that instead of truncate one needs some kind of adjust primitive. When I find a solution I will open a new PR.

@bschommer bschommer deleted the bschommer:responsefile branch Oct 12, 2016

@gasche

This comment has been minimized.

Show comment
Hide comment
@gasche

gasche Oct 12, 2016

Member

(I now understand that relaxing the check is not enough as truncate actually requires a smaller value. Maybe the toplevel could just forbid source script names from within the return of Expand?)

Member

gasche commented Oct 12, 2016

(I now understand that relaxing the check is not enough as truncate actually requires a smaller value. Maybe the toplevel could just forbid source script names from within the return of Expand?)

@gasche

This comment has been minimized.

Show comment
Hide comment
@gasche

gasche Oct 26, 2016

Member

@alainfrisch rightly pointed out that this feature was not documented (it's not in the manual or in the compiler manpages). I am planning on opening a Mantis ticket to track that need (tomorrow). @bschommer, would you be willing to send a patch for that?

Member

gasche commented Oct 26, 2016

@alainfrisch rightly pointed out that this feature was not documented (it's not in the manual or in the compiler manpages). I am planning on opening a Mantis ticket to track that need (tomorrow). @bschommer, would you be willing to send a patch for that?

end else begin
Buffer.add_char buf c; read ()
end
with End_of_file ->

This comment has been minimized.

@alainfrisch

alainfrisch Oct 4, 2017

Contributor

FWIW, this try...with breaks tail-recursion and would lead to stack overflow if the file is large enough. Will submit a PR to address that shortly.

@alainfrisch

alainfrisch Oct 4, 2017

Contributor

FWIW, this try...with breaks tail-recursion and would lead to stack overflow if the file is large enough. Will submit a PR to address that shortly.

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

Merge pull request #843 from bschommer/responsefile
[WIP] Implement responsefiles support PR7050
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment