ocamltest: do not overwrite user-defined variables by nojb · Pull Request #9633 · ocaml/ocaml

nojb · 2020-06-04T05:04:13Z

Fixes a bug discovered when reviewing #9469 : certain user-defined ocamltest variables (such as ocamlrunparam) will be overwritten by what is defined in the system environment (or set to empty if not set). This problem is present in the testsuite where ocamlrunparam = ... is systematically ignored and was being "fixed" by also calling Printexc.record_backtrace true.

The reason for the bug is that the ocamltest environment is computed in two steps:

User-defined variables (as written in the script) are processed:

ocaml/ocamltest/main.ml

Lines 190 to 192 in abb8db4

    
           let root_environment = 
        
             interprete_environment_statements 
        
               initial_environment rootenv_statements in

Certain variables that come from the system environment are also added: see

ocaml/ocamltest/main.ml

Line 193 in abb8db4

let rootenv = Environments.initialize log root_environment in

Environment.initialize ends up calling

ocaml/ocamltest/ocaml_actions.ml

Lines 1095 to 1122 in abb8db4

    
           let config_variables _log env = 
        
             Environments.add_bindings 
        
             [ 
        
               Ocaml_variables.arch, Ocamltest_config.arch; 
        
               Ocaml_variables.ocamlrun, Ocaml_files.ocamlrun; 
        
               Ocaml_variables.ocamlc_byte, Ocaml_files.ocamlc; 
        
               Ocaml_variables.ocamlopt_byte, Ocaml_files.ocamlopt; 
        
               Ocaml_variables.bytecc_libs, Ocamltest_config.bytecc_libs; 
        
               Ocaml_variables.nativecc_libs, Ocamltest_config.nativecc_libs; 
        
               Ocaml_variables.mkdll, 
        
                 Sys.getenv_with_default_value "MKDLL" Ocamltest_config.mkdll; 
        
               Ocaml_variables.mkexe, Ocamltest_config.mkexe; 
        
               Ocaml_variables.c_preprocessor, Ocamltest_config.c_preprocessor; 
        
               Ocaml_variables.csc, Ocamltest_config.csc; 
        
               Ocaml_variables.csc_flags, Ocamltest_config.csc_flags; 
        
               Ocaml_variables.shared_library_cflags, 
        
                 Ocamltest_config.shared_library_cflags; 
        
               Ocaml_variables.objext, Ocamltest_config.objext; 
        
               Ocaml_variables.asmext, Ocamltest_config.asmext; 
        
               Ocaml_variables.sharedobjext, Ocamltest_config.sharedobjext; 
        
               Ocaml_variables.ocamlc_default_flags, 
        
                 Ocamltest_config.ocamlc_default_flags; 
        
               Ocaml_variables.ocamlopt_default_flags, 
        
                 Ocamltest_config.ocamlopt_default_flags; 
        
               Ocaml_variables.ocamlrunparam, Sys.safe_getenv "OCAMLRUNPARAM"; 
        
               Ocaml_variables.ocamlsrcdir, Ocaml_directories.srcdir; 
        
               Ocaml_variables.os_type, Sys.os_type; 
        
             ] env

which overwrites a number of variables, including ocamlrunparam.

~~This PR inverts the order of 1. and 2., hopefully fixing the issue.~~ UPDATE: it turns out this doesn't work because inverting the two calls breaks the logic that deals with the modules variable (at least). As a minimal fix, I changed it instead so that variables coming from the system environment are added only when the same variable is not already defined by the user.

As a test, the explicit calls to Printexc.record_backtrace are removed from the relevant tests.

nojb · 2020-06-04T05:20:11Z

@shindere

shindere · 2020-06-04T09:37:55Z

Thanks a lot, @nojb. In principle this looks all very neat and sensible to me. Something puzzles me, though. As you might have noticed when adapting the tests, they contained definitions like ``` ocamlrunparam += ",b=1" ``` Mark the "+=" and the comma at the beginning of the string. What was supposed to happen for a variable like ocamlrunparam is that it should be initialized by ocamltest with the content of the environment variable, to which the string should be concatenated. That may turn useful if you e.g. want to make the GC verbose in addition, or things like that, which is something we do or at least did at some point. What do you think, @nojb ?

nojb · 2020-06-04T09:45:45Z

Thanks a lot, @nojb. In principle this looks all very neat and sensible to me. Something puzzles me, though. As you might have noticed when adapting the tests, they contained definitions like ocamlrunparam += ",b=1" Mark the "+=" and the comma at the beginning of the string. What was supposed to happen for a variable like ocamlrunparam is that it should be initialized by ocamltest with the content of the environment variable, to which the string should be concatenated. That may turn useful if you e.g. want to make the GC verbose in addition, or things like that, which is something we do or at least did at some point. What do you think, @nojb ?

Indeed! The problem is that the setting OCAMLRUNPARAM to ,b=1 (ie when not extending anything), does not currently work (but there is a fix in #9634).

If we merge that PR before I will amend this one accordingly.

In any case, there is a Travis failure related to the debug runtime that seems related to the proposed change. However, I haven't had time to investigate yet.

shindere · 2020-06-04T10:04:43Z

Nicolás Ojeda Bär (2020/06/04 02:46 -0700):

Indeed! The problem is that the setting `OCAMLRUNPARAM` to `,b=1` (ie when not extending anything), does not currently work (but there is a fix in #9634).

So nice! Thanks!

If we merge that PR before I will amend this one accordingly.

That looks like the right approach to me. thanks again.

nojb · 2020-06-04T21:24:51Z

I rebased the PR on top of #9634 (which is a dependency), and it is ready for a final review.

Explanation of the bug/resolution follows:

ocamltest has a notion of "initializers" which are sequences of variable bindings which are added to the environment, currently after the variables defined by the user in the test file.
The problem is that among those initializers there is one (called config_variables) which injects into the ocamltest environment some of the variables of the system environment. Among those, OCAMLRUNPARAM. Clearly, this will overwrite anything the user has written in their file.
Obvious solution is to invert the order in which the two sets of variables are added to the environment: first inject the variables coming from the system environment, and then add the ones coming from the test file.
The problem with this, is that there is another "initializer" that sets the value of the all_modules variable, and it requires knowledge of the modules variable to do so. This variable is often set in the test script, so this initializer cannot be "run" before the variables from the test script have been added to the environment.
The solution, then, is to add an extra property to the "initializers", we can have either a "Pre" initializer or a "Post" initializer. "Pre" initializers are "run" before the variables from the test script are processed, "Post" initializers, after.

And with that, the problem is fixed. (The explanation of the fix is longer than the fix itself.)

nojb · 2020-06-05T05:48:40Z

Rebased on trunk now that #9634 has been merged.

shindere · 2020-06-05T08:37:55Z

Nicolás Ojeda Bär (2020/06/04 22:48 -0700):

Rebased on trunk now that #9634 has been merged.

Thanks. Won't be able to review before later today but will do ASAP.

shindere · 2020-06-08T09:03:19Z

Gosh, so sorry I made things so complicated. The most frustrating is, I can't remember, several years later, why I did that. I guess (I hoope!) I had good reasons but forgot about them.

shindere · 2020-06-08T09:11:40Z

I went through the code which looks good to me, thanks.

I have one remaining question. Is the modofication of tests still needed?

nojb · 2020-06-08T09:15:54Z

I have one remaining question. Is the modofication of tests still needed?

I think so. By removing the call to Printexc.record_backtrace they serve as evidence that the bug fix works. And also, it seems weird to fix the handling of the ocamlrunparam variable and not use it in those tests that were written with the intent of using it.

nojb · 2020-06-08T10:13:38Z

Thanks for the review @shindere! Do you mind pressing the button to give a formal approval? Thanks!

shindere · 2020-06-08T10:59:23Z

Nicolás Ojeda Bär (2020/06/08 02:16 -0700):

> I have one remaining question. Is the modofication of tests still needed? I think so. By removing the call to `Printexc.record_backtrace` they serve as evidence that the bug fix works. And also, it seems weird to fix the handling of the `ocamlrunparam` variable and not use it in those tests that were written with the intent of using it.

Sure! I didn't check that commit, sorry.

shindere · 2020-06-08T12:07:01Z

Nicolás Ojeda Bär (2020/06/08 03:13 -0700):

Thanks for the review @shindere! Do you mind pressing the button to give a formal approval? Thanks!

I think I did it, no?

nojb · 2020-06-08T12:13:53Z

I think I did it, no?

Yes, thanks!

nojb mentioned this pull request Jun 4, 2020

Better backtraces for lazy values #9469

Merged

nojb force-pushed the ocamltest_env_fix branch from 2930f62 to 29d3dfa Compare June 4, 2020 05:38

nojb mentioned this pull request Jun 4, 2020

Allow ',' at the start of OCAMLRUNPARAM #9634

Merged

nojb force-pushed the ocamltest_env_fix branch 2 times, most recently from 033ee88 to b636f97 Compare June 4, 2020 21:01

nojb added 2 commits June 5, 2020 07:40

ocamltest: do not overwrite user-defined variables

6ff2c11

Adapt tests

d8d3875

nojb force-pushed the ocamltest_env_fix branch from b636f97 to d8d3875 Compare June 5, 2020 05:45

Changes

b765600

check-typo

a8cd077

shindere approved these changes Jun 8, 2020

View reviewed changes

nojb merged commit 990bc3c into ocaml:trunk Jun 8, 2020

	let root_environment =
	interprete_environment_statements
	initial_environment rootenv_statements in

	let config_variables _log env =
	Environments.add_bindings
	[
	Ocaml_variables.arch, Ocamltest_config.arch;
	Ocaml_variables.ocamlrun, Ocaml_files.ocamlrun;
	Ocaml_variables.ocamlc_byte, Ocaml_files.ocamlc;
	Ocaml_variables.ocamlopt_byte, Ocaml_files.ocamlopt;
	Ocaml_variables.bytecc_libs, Ocamltest_config.bytecc_libs;
	Ocaml_variables.nativecc_libs, Ocamltest_config.nativecc_libs;
	Ocaml_variables.mkdll,
	Sys.getenv_with_default_value "MKDLL" Ocamltest_config.mkdll;
	Ocaml_variables.mkexe, Ocamltest_config.mkexe;
	Ocaml_variables.c_preprocessor, Ocamltest_config.c_preprocessor;
	Ocaml_variables.csc, Ocamltest_config.csc;
	Ocaml_variables.csc_flags, Ocamltest_config.csc_flags;
	Ocaml_variables.shared_library_cflags,
	Ocamltest_config.shared_library_cflags;
	Ocaml_variables.objext, Ocamltest_config.objext;
	Ocaml_variables.asmext, Ocamltest_config.asmext;
	Ocaml_variables.sharedobjext, Ocamltest_config.sharedobjext;
	Ocaml_variables.ocamlc_default_flags,
	Ocamltest_config.ocamlc_default_flags;
	Ocaml_variables.ocamlopt_default_flags,
	Ocamltest_config.ocamlopt_default_flags;
	Ocaml_variables.ocamlrunparam, Sys.safe_getenv "OCAMLRUNPARAM";
	Ocaml_variables.ocamlsrcdir, Ocaml_directories.srcdir;
	Ocaml_variables.os_type, Sys.os_type;
	] env

Conversation

nojb commented Jun 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nojb commented Jun 4, 2020

Uh oh!

shindere commented Jun 4, 2020 via email

Uh oh!

nojb commented Jun 4, 2020

Uh oh!

shindere commented Jun 4, 2020 via email

Uh oh!

nojb commented Jun 4, 2020

Uh oh!

nojb commented Jun 5, 2020

Uh oh!

shindere commented Jun 5, 2020 via email

Uh oh!

shindere commented Jun 8, 2020 via email

Uh oh!

shindere commented Jun 8, 2020

Uh oh!

nojb commented Jun 8, 2020

Uh oh!

nojb commented Jun 8, 2020

Uh oh!

shindere commented Jun 8, 2020 via email

Uh oh!

shindere commented Jun 8, 2020 via email

Uh oh!

nojb commented Jun 8, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

nojb commented Jun 4, 2020 •

edited

Loading