get_test_tmp_dir() should return system tmp, not current dir#6964
get_test_tmp_dir() should return system tmp, not current dir#6964steven-johnson wants to merge 1 commit intomainfrom
Conversation
If TEST_TMPDIR isn't defined, we return the current directory, which is pretty much *never* the right thing. We should use /tmp or the equivalent. (Note that on Windows there is the potential degenerate case of GetTempPath() failing, usually under either weird test setups or weird enterprise-configuration setups in which various temp dirs of users are artificially locked down; these are both corner cases that we should deal with as they become problems.)
|
What are the contexts in which TEST_TMPDIR is not set? Is it just running a test manually in the shell? When I'm doing that, I like output files to go in the current working directory by default. system-wide tmp dirs are a pretty useless place to put anything that a human might want to inspect, because it's in a distant relative path, often with a long auto-generated name that's hard to find. |
|
Actually I guess I'm not sure what TEST_TMPDIR is used for. Is it used for intermediate diagnostic outputs for things like simd op check that a human may read? Or is it just for things like checking that a generated object file has the right header? |
|
Looks like it's used for both :( |
The problem is that the "current working directory" isn't necessarily what you think it is. In some contexts on Windows (eg running in a debugger), it can be an unwritable system directory (!), so the test will just fail immediately. When combined with #6959, this can make for a very hard to debug frustration. I get that it's useful to see outputs in a handful of situations, but I argue that "the current directory" is still not the right place -- e.g., every time I ran simd_op_check via make and ended up with hundreds of .h and .s files littering the root Halide source directory. For the cases where these are often useful, what if we logged the path being used to stdout at start of the test (as we already do for HL_TARGET, etc)? |
It also occurs to me that tests like this are run as part of a test suite probably 1000x the rate that they are run by humans interactively; being able to direct the output to "current dir" or whatever sounds like a fine thing to have as opt-in, but definitely the wrong optimization for opt-out. |
|
But surely in all contexts when run as a test suite the environment variable will be set. The only difference in behavior here is for ad-hoc runs. Logging the path being used solves the findability problem I think. I'll just set TEST_TMPDIR=. in my bash profile to get the behavior I prefer. Should it really be HALIDE_TEST_TMPDIR? |
2 participants
If TEST_TMPDIR isn't defined, we return the current directory, which is pretty much never the right thing. We should use /tmp or the equivalent.
(Note that on Windows there is the potential degenerate case of GetTempPath() failing, usually under either weird test setups or weird enterprise-configuration setups in which various temp dirs of users are artificially locked down; these are both corner cases that we should deal with as they become problems.)