local actions, tmpdir: implement --local_tmp_root flag

[laszlocsomor]

Description of the problem / feature request:

Add a --local_tmp_root flag to Bazel that lets the user specify a temp directory for locally executed actions. This compliments the feature implemented for #3215 where Bazel picks up the client environment's TMPDIR envvar.

Feature requests: what underlying problem are you trying to solve with this feature?

Allow users to put this flag in project-specific bazelrc files, so users won't have to update their bashrc/bash_profile.

The former approach's benefits are that the information is local to a code base, and it has local effect (as opposed to the bashrc approach).

See also #3215 (comment)

What operating system are you running Bazel on?

All supported ones.

[ittaiz]

Thanks!

…

On Mon, 12 Feb 2018 at 13:56 László Csomor ***@***.***> wrote: Description of the problem / feature request: Add a --local_tmp_root flag to Bazel that lets the user specify a temp directory for locally executed actions. This compliments the feature implemented for #3215 <#3215> where Bazel picks up the client environment's TMPDIR envvar. Feature requests: what underlying problem are you trying to solve with this feature? Allow users to put this flag in project-specific bazelrc files, so users won't have to update their bashrc/bash_profile. The former approach's benefits are that the information is local to a code base, and it has local effect (as opposed to the bashrc approach). See also #3215 (comment) <#3215 (comment)> What operating system are you running Bazel on? All supported ones. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#4621>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABUIF-Y9Typk-6l02OTjgeDScTwzThaPks5tUCcCgaJpZM4SCEA5> .

[ittaiz]

@laszlocsomor I just tried running a test using bazel built from HEAD (I verified the change is in) when I ran it with --local_tmp_root=/tmp --strategy=TestRunner=standalone and I still get:

java.lang.RuntimeException: java.io.IOException: java.lang.RuntimeException: mysql start failed with error: [ERROR] The socket file path is too long (> 103): /private/var/tmp/_bazel_ittaiz/1c8ed8d84f6cb79483aa3cc4da758c86/execroot/foo/_tmp/d26550dc5bfd5572403cbf71f40d4422/2819823247725996448.sock

Am I doing something wrong?
If I add in the relevant BUILD file:

    jvm_flags = [
        "-Djava.io.tmpdir=/tmp",
    ],

The test passes

[laszlocsomor]

@ittaiz : You're right. I'm looking into this issue. Sorry this this feature is still broken.

[laszlocsomor]

Found the culprit:

bazel/src/main/java/com/google/devtools/build/lib/exec/TestPolicy.java

Line 81 in b653624

val = val.replace(TEST_TMP_DIR, tmpDirPath);

This sets $TEST_TMPDIR's value to be under a subdirectory of the test temp root, not under the local temp root. See below for more info.

Observations:

• I just found out that there's already a --test_tmpdir flag. However while --local_tmp_root affects all local actions, --test_tmpdir only affects local tests.
• Sadly, both flags are named misleadingly. (We can still easily change that for --local_tmp_root though.)
  • The new --local_tmp_root defines a directory (not a root), that all actions should use as $TMPDIR.
  • The existing --test_tmpdir defines a root (not a directory), that Bazel creates a unique subdirectory in for each test, and sets that as $TEST_TMPDIR. The java_binary/java_test runner stub then picks this up and sets -Djava.io.tmpdir to its value:

    bazel/src/main/java/com/google/devtools/build/lib/bazel/rules/java/java_stub_template.txt

    Line 276 in b653624

    JVM_FLAGS+=" -Djava.io.tmpdir=$TEST_TMPDIR"

TODO for me:

• update the TestPolicy to also consider --local_tmp_root (though IMO --test_tmpdir should have priority over it for tests)
• rename --local_tmp_root to --local_tmpdir
• opt: add a --test_tmproot flag, make it an alias of --test_tmpdir
• opt: make --test_tmpdir deprecated (may require internal depot cleanup)

[laszlocsomor]

@ittaiz : For now, maybe --test_tmpdir=/tmp would suffice? The effective temp directory will be something like /tmp/_tmp/3e07cf3aab431e611679f7f0415dffba, which is 42 characters long.

[laszlocsomor]

Also, let me roll back 04757db for now so I'm (a) not under time pressure to fix it, and (b) can discuss with the team whether adding another flag for local temp directory control, when we already have --test_tmpdir, is actually a good idea. (I wasn't aware of this flag until now.)

[ittaiz]

@laszlocsomor sorry for the late reply but wanted to test test_tmpdir on multiple scenarios and it's indeed very helpful. Thanks!

[ittaiz]

btw, my 2c is that if we have test_tmpdir it's indeed really unclear if we need another flag. I'd wait for more use-cases.
I would however consider renaming it since it's misleading as you pointed out...

[laszlocsomor]

@ittaiz No worries, and cool, that's good to hear! And yes, agreed on the naming and unclear necessity too.

[laszlocsomor]

I'm closing this because we already have --test_tmpdir and that seems to work. @ittaiz please let me know if that's not the case!