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.

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

ocamlc -config: new -config-var option to print specific configuration variables #1430

Merged
merged 1 commit into from Apr 30, 2018

Conversation

Projects
None yet
6 participants
@gasche
Copy link
Member

commented Oct 14, 2017

The proposed behavior of -config-var s is as follows:

  • if s is an existing configuration variable, print its value as
    a string, a newline, and exit with a success return value (0)
  • if s is not an existing configuration variable, print nothing
    and exit with a failure return value (non-0)

This gives two ways to distinguish success from failure to script
authors: the return code, and counting the lines of output.

The following alternative behaviors were considered:

  • We could not print a newline after the configuration value, which
    might arguably be useful in some configuration scenarios. The
    utility is doubtful as Unix shell expansions
    $(ocamlc -config-var ...) remove trailing newlines, and it prevents
    the line-count-based failure detection (an empty configuration variable
    would also print nothing).

  • We could print a message on the error output if the configuration
    variable does not exist. This is clearer to a human user, but it is
    annoying for scripts if they forget to silence the error output and
    get their output mixed with our error messages. The main use of this
    new feature is for scripting purposes.

@gasche

This comment has been minimized.

Copy link
Member Author

commented Oct 14, 2017

@gasche gasche added this to the 4.07-or-later milestone Oct 14, 2017

@gasche gasche force-pushed the gasche:config-var branch from 616de32 to c093a95 Oct 14, 2017

@shindere

This comment has been minimized.

Copy link
Contributor

commented Oct 16, 2017

@dra27
Copy link
Contributor

left a comment

Thanks for doing this! I agree with @shindere that it would be nice to work in a mechanism for deprecation, which would allow bytecomp_c_compiler and native_c_compiler to display a deprecation warning on stderr.

If that is done too, this is a pretty low-risk change - might inclusion in 4.06.0 be safe to do?

let p name valu = (name, String valu) in
let p_int name valu = (name, Int valu) in
let p_bool name valu = (name, Bool valu) in
[

This comment has been minimized.

Copy link
@dra27

dra27 Oct 20, 2017

Contributor

As this being updated, could we do s/valu/value ?


let print_config oc =
let print (name, valu) =
Printf.fprintf oc "%s: %a\n" name print_config_value valu in

This comment has been minimized.

Copy link
@dra27

dra27 Oct 20, 2017

Contributor

Similarly here s/valu/value

match List.assoc_opt name configuration_variables with
| None -> None
| Some v ->
let s = match v with

This comment has been minimized.

Copy link
@dra27

dra27 Oct 20, 2017

Contributor

It's a smallmicroscopic nit, but valu/value is used elsewhere to refer to a value, but v here


let show_config_variable_and_exit var =
match Config.config_var var with
| Some valu ->

This comment has been minimized.

Copy link
@dra27

dra27 Oct 20, 2017

Contributor

Same here with s/valu/value!

let show_config_variable_and_exit var =
match Config.config_var var with
| Some valu ->
print_endline valu;

This comment has been minimized.

Copy link
@dra27

dra27 Oct 20, 2017

Contributor

Why do this - not doing this will be amazingly wonderful for compatibility between Windows/Cygwin and the rest of the world where shell scripting is concerned (no more tr -d '\r' all over the place)

This comment has been minimized.

Copy link
@gasche

gasche Oct 20, 2017

Author Member

You mean why include a line break at the end instead of using print_string?

This comment has been minimized.

Copy link
@dra27

dra27 Oct 20, 2017

Contributor

Yes, indeed - sorry for being somewhat less than clear!

@gasche

This comment has been minimized.

Copy link
Member Author

commented Oct 20, 2017

I don't like valu but I'm not fond of value either, so I may just use v if that tickles.

@shindere

This comment has been minimized.

Copy link
Contributor

commented Oct 20, 2017

@gasche

This comment has been minimized.

Copy link
Member Author

commented Oct 20, 2017

I've written enough code in revised syntax to see it as a keyword.

@shindere

This comment has been minimized.

Copy link
Contributor

commented Oct 20, 2017

@gasche

This comment has been minimized.

Copy link
Member Author

commented Oct 20, 2017

v will be just fine.

@shindere

This comment has been minimized.

Copy link
Contributor

commented Oct 20, 2017

@dbuenzli

This comment has been minimized.

Copy link
Contributor

commented Oct 20, 2017

In a context where there are both variables and values

Unless the terminology is already pervasive in the code you could use the configuration key terminology rather than configuration variable, and thus k and v.

@gasche gasche force-pushed the gasche:config-var branch from c093a95 to 8ca69d4 Oct 24, 2017

@gasche

This comment has been minimized.

Copy link
Member Author

commented Oct 24, 2017

I rebased the PR with the variable renaming (and against trunk). I went for x and v instead of k and v because then I would have had to rename -config-var into -config-key and I found I like "configuration variable and value" better than "configuration key and value" (but let your preference be known if it differs).

I am procrastinating against removing the newline for no very good reason. Deep down I like printing a newline (for testing the feature from the command-line) and I find it extremely frustrating that bash on Windows does not strip the trailing \r\n away and that I have to change my code for this. I also spent some time staring at David's tr -d calls all over the build system in pain and disbelief. I plan to wait until the burning fades away and do the reasonable thing of using print_string.

@xavierleroy

This comment has been minimized.

Copy link
Contributor

commented Oct 24, 2017

I am procrastinating against removing the newline for no very good reason

I think you should bite the bullet and do it. We'll see how users react to this.

I like printing a newline (for testing the feature from the command-line)

If we want to get fancy (but not in 4.06) we could print a newline if the output is a terminal... Now that we have a "isatty" function in the runtime, and that it is almost working, we could use it more widely.

@gasche gasche force-pushed the gasche:config-var branch from 8ca69d4 to 748a07a Oct 25, 2017

@gasche

This comment has been minimized.

Copy link
Member Author

commented Oct 25, 2017

I rebased the PR to remove the newline, but I remain unsatisfied. Could someone that knows more about shell scripting than I do tell me how they would typically (and correctly) use this new -config-var option in a shell script? I must admit that I don't understand how to test for the return code of the command in a nice/concise way.

(Edit: a previous version of this message had an incorrect shell script that didn't work below. Fixed.)

The best I found is

VALUE=$(ocamlc -config-var $VARIABLE)
if [ $? -ne "0" ]
then echo "variable does not exist"
else echo "variable does exist and its value is \"$VALUE\""
fi

I'm thinking that maybe I should also add a -config-has-var <variable> command that would output true or false, because shell scripting is just too hard.

@gasche

This comment has been minimized.

Copy link
Member Author

commented Oct 25, 2017

This appears to work and it does look reasonably nice (in particular, contrarily to the $? business, it is not too fragile if several calls to ocamlc -config-var are made in sequence).

if VALUE=$(ocamlc -config-var $VARIABLE)
then echo "variable does exist and its value is \"$VALUE\""
else echo "variable does not exist"
fi
@dra27

This comment has been minimized.

Copy link
Contributor

commented Oct 25, 2017

I think you may be virtuously applying good software engineering principles to the evil world of shell scripting 🙂. I would anticipate doing something like:

PREFIX=$(ocamlc -config-var prefix 2>&1 || ocamlc -config | tr -d '\015' | sed -ne "s/prefix: //p")

and expect things to fail noisily in the strange event that PREFIX is "". If I were writing something which requires an OCaml which has this feature, then I wouldn't expect to test the exit code at all, I'd just expect ocamlc -config-var prefix to have worked (having previously checked ocamlc -vnum, etc.)

@gasche gasche force-pushed the gasche:config-var branch from 748a07a to 48453ad Nov 11, 2017

@gasche

This comment has been minimized.

Copy link
Member Author

commented Nov 11, 2017

I rebased, I believe all comments have been taken into account, and I added manpage and manual documentation.

@damiendoligez
Copy link
Member

left a comment

Looks pretty good but I have two remarks.

@@ -38,7 +38,7 @@ endif
OBJS=dynlinkaux.cmo dynlink.cmo

COMPILEROBJS=\
../../utils/misc.cmo ../../utils/config.cmo \
../../utils/config.cmo ../../utils/misc.cmo \

This comment has been minimized.

Copy link
@damiendoligez

damiendoligez Nov 13, 2017

Member

This makes me slightly nervous. Maybe show_config_and_exit and show_config_var_and_exit should be in config.ml rather than misc.ml?

This comment has been minimized.

Copy link
@gasche

gasche Nov 18, 2017

Author Member

I think that config should remain a "pure data" module as much as possible.

I don't think we should add logic to it. Maybe I could create a separate config_misc module depending on config?

On the other hand, I think that config should really be at the bottom of the module dependency graph (especially if we commit to keeping it a pure data module), so I think the change is right in the long term -- although I agree it makes us a bit nervous.

This comment has been minimized.

Copy link
@damiendoligez

damiendoligez Jan 16, 2018

Member

I agree that Config should be a pure data module and that pure data modules should be at the bottom of the graph, so this is OK after all.

let mk_config_var f =
"-config-var", Arg.String f,
" Print the value of a configuration variable, a newline, and exit \
(print nothing and exit with error value if the variable does not exist)"

This comment has been minimized.

Copy link
@damiendoligez

damiendoligez Nov 13, 2017

Member

Could you please cut this message with \n\ and not make the compiler's help output look like shit (any more than it already does)?

This comment has been minimized.

Copy link
@gasche

gasche Nov 18, 2017

Author Member

@damiendoligez could I implement an Arg.wrap : ~line_length:int -> specs -> specs function instead of manually doing the job of choosing the right cut place depending on the option name and alignment choice?

This comment has been minimized.

Copy link
@damiendoligez

damiendoligez Jan 16, 2018

Member

Sure, go ahead.

This comment has been minimized.

Copy link
@gasche

gasche Apr 10, 2018

Author Member

In the end I did the simple thing of adding turning the end-of-line \ into \n \.

@gasche

This comment has been minimized.

Copy link
Member Author

commented Apr 10, 2018

I had forgotten that this hadn't been merged yet (I thought of it because of #1691). I will do the short fix of adding \ns in the help output, and propose this for merging again.

@gasche gasche force-pushed the gasche:config-var branch from 48453ad to aa736c6 Apr 10, 2018

@gasche

This comment has been minimized.

Copy link
Member Author

commented Apr 10, 2018

@damiendoligez, I believe that your changes have been addressed. Would you approve the PR? My plan would be to merge into trunk (not 4.07) once the CI passes, to be done with this change.

@gasche gasche force-pushed the gasche:config-var branch from aa736c6 to 92d6dbe Apr 20, 2018

\item["-config-var" \var{var}]
Print the value of a specific configuration variable from the
"-config" output, then exit. If the variable does not exist, the exit
code is non-zero. This option is only available since OCaml 4.07,

This comment has been minimized.

Copy link
@damiendoligez

damiendoligez Apr 24, 2018

Member

You'll have to change this to 4.08 :-(

This comment has been minimized.

Copy link
@gasche

gasche Apr 24, 2018

Author Member

Thanks for the catch, I changed it and rebased the PR.

@gasche gasche force-pushed the gasche:config-var branch 3 times, most recently from b38af78 to 6a47aa3 Apr 24, 2018

ocamlc -config: new -config-var option to print specific configuratio…
…n variables

The proposed behavior of `-config-var s` is as follows:
- if `s` is an existing configuration variable, print its value as
  a string and exit with a success return value (0)
- if `s` is not an existing configuration variable, print nothing
  and exit with a failure return value (non-0)

Note that we do not print a newline after the value of the
configuration variable. In particular, if the value is an empty
string, the output is undistinguishable from the output for
non-existing variables, the return value has to be considered instead.

The following alternative behaviors were considered:

- We could print a newline after the configuration value, which
  would let users distinguish empty values from non-existing variables
  by counting the lines of output, and would also be more pleasant for
  users invoking the option from the command-line. However, the way
  bash works on Windows means that $(ocamlc -config-var foo) would keep
  a trailing \r in its output, and portable scripts would have to use
  $(ocamlc -config-var foo | tr -d '\r') instead, which is a pain.
  (This issue was pointed out by David Allsopp)

- We could print a message on the error output if the configuration
  variable does not exist. This is clearer to a human user, but it is
  annoying for scripts if they forget to silence the error output and
  get their output mixed with our error messages. The main use of this
  new feature is for scripting purposes.

@gasche gasche force-pushed the gasche:config-var branch from 6a47aa3 to 559206b Apr 28, 2018

@gasche gasche merged commit 832a3ec into ocaml:trunk Apr 30, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.