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

configure: use variables rather than arguments for a few options #8616

Merged
merged 1 commit into from May 9, 2019

Conversation

Projects
None yet
6 participants
@shindere
Copy link
Contributor

commented Apr 15, 2019

This PR replaces a few configure command-line options by configuration
variables:

  • "--with-dllibs" is replaced by DLLIBS
  • "--with-reserved-header-bits" is replaced by RESERVED_HEADER_BITS
  • "--with-default-string" is replaced by DEFAULT_STRING
@dra27

This comment has been minimized.

Copy link
Contributor

commented Apr 15, 2019

Thoughts and cross-references to the original PR!

cf. #2139 (comment) which suggested using AC_ARG_ENABLE for the string mode. I agree(d) with the DLLIBS change (cf. #2139 (comment)). For --with-reserved-header-bits was noted in #2139 (comment), I suggested switching to AC_ARG_ENABLE - I don't have as strong an opinion, but it feels as though AC_ARG_VAR should be always responsible for changing something (like flags, etc.) and not for enabling/disabling something (i.e. there should not be a special value for an environment which is responsible for turning things on and off).

@gasche

This comment has been minimized.

Copy link
Member

commented Apr 15, 2019

Is the idea that environment variables are a better interface than command-line arguments? (I generally tend to assume the converse, that explicit arguments are a clearer interface than passing configuration options on-the-side through the environment; but then I don't have much experience with autoconf conventions etc.)

For DEFAULT_STRING I'm not convinced by the idea that force/don't-force is a command-line parameter but safe/unsafe default is in the environment; it makes sense to have all the options at the same place -- possibly as @dra27 suggests as a single setting, force / default-safe / default-unsafe, although personally I'm fine with having two different knobs (will comment in the relevant discussion).

@mshinwell

This comment has been minimized.

Copy link
Contributor

commented Apr 15, 2019

I'm not sure what the rationale for this patch is, but I'm against using environment variables for configuration in general. The problem with environment variables is that their state is easy to forget, for a human user; and for a program calling a script that depends on such variables, you need to take care that they are in the correct state. That basically means clearing the environment and starting from scratch, which isn't great.

@shindere

This comment has been minimized.

Copy link
Contributor Author

commented Apr 15, 2019

@shindere

This comment has been minimized.

Copy link
Contributor Author

commented Apr 15, 2019

@gasche

This comment has been minimized.

Copy link
Member

commented Apr 15, 2019

I had the same confusion as Mark (I wasn't actually aware of "configuration variables" as a distinct thing from "environment variables"). Your explanation justifies having --enable-force-safe-string but DEFAULT_STRING=....

Some further questions come from a closer look:

  • when is it appropriate to use the --enable-feature[=ARG] form that takes an argument?
  • what do the [=PKGS] parameters mean for --enable-{shared,static,fast-install} and --with-pic?
  • wouldn't --enable-reserved-header-bits=NUM make sense? (This is a feature that is enabled?)

I suppose that you are also planning to convert --with-pic and --with-sysroot to this style later?

@gasche

This comment has been minimized.

Copy link
Member

commented Apr 15, 2019

Re force-safe-string vs. default-safe-string, I wrote on the thread in the other PR:

There is a conceptual difference between:

  • --force-safe-string, which must be a configure-time option that changes the runtime definitions and has a global impact on all modules compiled and linked by the resulting compiler

  • --default-string={safe,unsafe} which is "merely" a convenience feature to set a default for compile-time options, rather than a configure-time option by nature (afl is the same, etc.). It does nothing more than change the default for a choice that the user makes on a per-invocation basis.

In my opinion this conceptual difference justifies not showing the three options as a single knob to fix at configure-time, but two separate kind of options. There is a dependency between them (--force-safe-string makes it incorrect to change the per-invocation value, so it makes --default-string useless and meaningless), but that is common across the configuration space, for example --disable-native-compiler will make many other settings meaningless.

@shindere

This comment has been minimized.

Copy link
Contributor Author

commented Apr 15, 2019

@shindere

This comment has been minimized.

Copy link
Contributor Author

commented Apr 16, 2019

@shindere shindere force-pushed the shindere:configvars branch from 0a4fcc2 to ee3e121 Apr 16, 2019

@shindere

This comment has been minimized.

Copy link
Contributor Author

commented Apr 16, 2019

Code updated to use AC_ARG_ENABLE to define the number of bits
reserved for profiling info in block headers.

Only numbers between 0 and 31 are accepted as arguemnts.

Giving no argument to this option is considered an error.

@gasche

This comment has been minimized.

Copy link
Member

commented Apr 16, 2019

Only numbers between 0 and 31 are accepted as arguemnts.

I'm not sure why you chose 31 as an upper bound? I haven't seen such a bound in the implementation and it seems to me that some choices between 32 and 54 would still make sense.

@shindere

This comment has been minimized.

Copy link
Contributor Author

commented Apr 16, 2019

@gasche

This comment has been minimized.

Copy link
Member

commented Apr 16, 2019

I guess 31 or 32 is fine then. (I would find it strange if there is nothing closer to the actual logic of the code that checks that this parameter is sensible, but you're not responsible for checking or changing this.)

@shindere

This comment has been minimized.

Copy link
Contributor Author

commented Apr 17, 2019

@damiendoligez damiendoligez added this to the 4.08 milestone May 6, 2019

@damiendoligez
Copy link
Member

left a comment

The code looks good and it seems everyone agrees on the interface.

@shindere

This comment has been minimized.

Copy link
Contributor Author

commented May 7, 2019

@shindere shindere force-pushed the shindere:configvars branch from ee3e121 to 3d4a61e May 7, 2019

@damiendoligez
Copy link
Member

left a comment

CI is unhappy because of something to do with Dynlink.

@@ -102,7 +102,6 @@ AC_SUBST([oc_cflags])
AC_SUBST([oc_cppflags])
AC_SUBST([oc_ldflags])
AC_SUBST([bytecclibs])
AC_SUBST([dllibs])

This comment has been minimized.

Copy link
@damiendoligez

damiendoligez May 7, 2019

Member

I would naively expect this to be replaced by AC_SUBST([DLLIBS]) rather than removed altogether.

@@ -1265,7 +1267,7 @@ AS_IF([$shared_libraries_supported],
[AC_CHECK_FUNC([dlopen],
[supports_shared_libraries=true dllibs=""],

This comment has been minimized.

Copy link
@damiendoligez

damiendoligez May 7, 2019

Member

Here dllibs should be replaced with DLLIBS, right?

configure: use variables rather than arguments for a few options
This commit replaces a few configure command-line options by configuration
variables:

- "--with-dllibs" is replaced by DLLIBS
- "--with-reserved-header-bits" is replaced by RESERVED_HEADER_BITS
- "--with-default-string" is replaced by DEFAULT_STRING

@shindere shindere force-pushed the shindere:configvars branch from 3d4a61e to 9d1940f May 7, 2019

@shindere

This comment has been minimized.

Copy link
Contributor Author

commented May 7, 2019

@shindere shindere merged commit 17feab2 into ocaml:trunk May 9, 2019

2 checks passed

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

This comment has been minimized.

Copy link
Contributor Author

commented May 9, 2019

Merged into trunk and cherry-picked to 4.08 as commit
6da973d

@shindere shindere deleted the shindere:configvars branch May 9, 2019

@trefis

This comment has been minimized.

Copy link
Contributor

commented May 9, 2019

It would be a shame if you forgot about 4.09.

@shindere

This comment has been minimized.

Copy link
Contributor Author

commented May 9, 2019

shindere added a commit that referenced this pull request May 9, 2019

configure: use variables rather than arguments for a few options (#8616)
This commit replaces a few configure command-line options by configuration
variables:

- "--with-dllibs" is replaced by DLLIBS
- "--with-reserved-header-bits" is replaced by RESERVED_HEADER_BITS
- "--with-default-string" is replaced by DEFAULT_STRING

shindere added a commit that referenced this pull request May 9, 2019

configure: use variables rather than arguments for a few options (#8616)
This commit replaces a few configure command-line options by configuration
variables:

- "--with-dllibs" is replaced by DLLIBS
- "--with-reserved-header-bits" is replaced by RESERVED_HEADER_BITS
- "--with-default-string" is replaced by DEFAULT_STRING

smuenzel added a commit to smuenzel/ocaml that referenced this pull request Jun 26, 2019

configure: use variables rather than arguments for a few options (oca…
…ml#8616)

This commit replaces a few configure command-line options by configuration
variables:

- "--with-dllibs" is replaced by DLLIBS
- "--with-reserved-header-bits" is replaced by RESERVED_HEADER_BITS
- "--with-default-string" is replaced by DEFAULT_STRING
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.