fix(bazel): Always disable the disk cache #8811

fviernau · 2024-06-28T07:21:53Z

See individual commits.

plugins/package-managers/bazel/src/main/kotlin/Bazel.kt

fviernau · 2024-06-28T07:30:27Z

Reading up on [1], it may be better to always disable the disk cache. From the comments I guess that it
speeds up builds only if you make changes to the code / switch branches, which is not the case in ORT analysis.
Any thoughts @haikoschol @sschuberth ?

This can be done by:

        val modGraphProcess = run(
            "mod", "graph", "--output", "json", "--disk_cache", "\"\"", workingDir = projectDir
        )

[1] https://stackoverflow.com/questions/61282419/bazel-how-outputroot-and-disk-cache-relate-regarding-local-caching

nnobelis · 2024-06-28T07:34:36Z

@fviernau As I mentioned in the issue, you need to specify the parameter (temp disk cache or no disk cache) to EVERY Bazel command run by ORT, even the --version.

codecov · 2024-06-28T07:35:20Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 67.79%. Comparing base (767475e) to head (5a6dbac).

Additional details and impacted files

@@            Coverage Diff            @@
##               main    #8811   +/-   ##
=========================================
  Coverage     67.79%   67.79%           
  Complexity     1164     1164           
=========================================
  Files           243      243           
  Lines          7711     7711           
  Branches        861      861           
=========================================
  Hits           5228     5228           
  Misses         2127     2127           
  Partials        356      356

Flag	Coverage Δ
funTest-non-docker	`33.96% <ø> (ø)`
test	`38.12% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

fviernau · 2024-06-28T07:36:23Z

@fviernau As I mentioned in the issue, you need to specify the parameter (temp disk cache or no disk cache) to EVERY Bazel command run by ORT, even the --version.

Did I miss any bazel command invocations?

nnobelis · 2024-06-28T07:36:09Z

plugins/package-managers/bazel/src/funTest/assets/projects/synthetic/bazel/.bazelrc

@@ -0,0 +1 @@
+common --disk_cache=/non-existing-dir


Missing line at the end

nnobelis · 2024-06-28T07:36:30Z

plugins/package-managers/bazel/src/main/kotlin/Bazel.kt

@@ -20,6 +20,7 @@
 package org.ossreviewtoolkit.plugins.packagemanagers.bazel


Commit message: fixes the related issue.

nnobelis · 2024-06-28T07:39:13Z

@fviernau As I mentioned in the issue, you need to specify the parameter (temp disk cache or no disk cache) to EVERY Bazel command run by ORT, even the --version.

Did I miss any bazel command invocations?

As I said version here https://github.com/oss-review-toolkit/ort/blob/main/plugins/package-managers/bazel/src/main/kotlin/Bazel.kt#L85

haikoschol · 2024-06-28T07:43:57Z

Reading up on [1], it may be better to always disable the disk cache. From the comments I guess that it speeds up builds only if you make changes to the code / switch branches, which is not the case in ORT analysis. Any thoughts @haikoschol @sschuberth ?

I agree that always disabling the disk cache seems like the best approach.

fviernau · 2024-06-28T07:44:35Z

As I said version here https://github.com/oss-review-toolkit/ort/blob/main/plugins/package-managers/bazel/src/main/kotlin/Bazel.kt#L85

I am able to run bazel --version successfully. Have you managed to reproduce the problem with bazel --version ?

nnobelis · 2024-06-28T07:50:39Z

This is really strange. bazel version works fine in the CLI but it fails when ORT runs it:

Exception in thread "main" java.io.IOException: Running 'bazel version' in '/path' failed with exit code 2:
ERROR: /home/dockeruser/.cache/bazel_disk_cache (Permission denied)
ERROR: Error initializing RemoteModule

nnobelis · 2024-06-28T07:53:17Z

@fviernau As a sidenote, please note that there is bazel version run by ORT which gives a verbose output and bazel --version which just give the version number.

Maybe we can use --version and remove transformBazelVersion alltogether.

@haikoschol Would it be ok?

sschuberth · 2024-06-28T07:54:47Z

bazel version works fine in the CLI but it fails when ORT runs it:

Maybe due to different implicit working directories?

fviernau · 2024-06-28T07:55:43Z

Maybe due to different implicit working directories?

I've run bazel in the directory in which the .bazelrc resides in.

nnobelis · 2024-06-28T07:57:01Z

plugins/package-managers/bazel/src/main/kotlin/Bazel.kt

@@ -171,7 +171,10 @@ class Bazel(
        }.getOrNull()

    private fun getDependencyGraph(projectDir: File, depDirectives: Map<String, BazelDepDirective>): Set<Scope> {
-        val modGraphProcess = run("mod", "graph", "--output", "json", workingDir = projectDir)
+        val modGraphProcess = run(
+            "mod", "graph", "--output", "json", "--disk_cache", "\"\"", workingDir = projectDir


I did my tests with run("mod", "graph", "--output", "json", "--disk_cache=", workingDir = projectDir) and it worked fine.

So you mean to remove the "dummy" "\"\"" argument in favor of adding a = after the option name?

I changed it as @nnobelis has put it.

haikoschol · 2024-06-28T07:58:08Z

@fviernau As a sidenote, please note that there is bazel version run by ORT which gives a verbose output and bazel --version which just give the version number.

Maybe we can use --version and remove transformBazelVersion alltogether.

@haikoschol Would it be ok?

I don't see why not. Weird that I didn't discover --version myself back then.

BTW, I run Bazel through Bazelisk on my laptop, which seems to intercept the version command:

$ bazel version
Bazelisk version: 1.20.0
Build label: 7.2.1
Build target: @@//src/main/java/com/google/devtools/build/lib/bazel:BazelServer
Build time: Tue Jun 25 15:53:57 2024 (1719330837)
Build timestamp: 1719330837
Build timestamp as int: 1719330837

And --version still has some more output than the version number, but again, might be a Bazelisk thing:

$ bazel --version
bazel 7.2.1

fviernau · 2024-06-28T08:06:07Z

Thanks guys, very helpful! I've added a commit to switch to bazel --version.

haikoschol · 2024-06-28T08:19:43Z

plugins/package-managers/bazel/src/test/kotlin/BazelTest.kt

-    "transformBazelVersion()" should {
-        "remove everything except for the version number" {
-            val bazelVersionOutput = """
-                Bazelisk version: development


I even put the Bazelisk output in the test, even though I didn't use it in the Docker image. Surprising that this ever worked. 😅

sschuberth · 2024-06-28T08:31:55Z

plugins/package-managers/bazel/src/main/kotlin/Bazel.kt

-    override fun getVersionArguments() = "version"
-
-    override fun transformVersion(output: String) = transformBazelVersion(output)
+    override fun transformVersion(output: String) = output.removePrefix("bazel ")


Nit: Maybe say "Using the default bazel --version..." to better explain why this works by basically just removing code.

sschuberth · 2024-06-28T08:33:57Z

plugins/package-managers/bazel/src/main/kotlin/Bazel.kt

@@ -171,7 +171,10 @@ class Bazel(
        }.getOrNull()

    private fun getDependencyGraph(projectDir: File, depDirectives: Map<String, BazelDepDirective>): Set<Scope> {
-        val modGraphProcess = run("mod", "graph", "--output", "json", workingDir = projectDir)
+        val modGraphProcess = run(
+            "mod", "graph", "--output", "json", "--disk_cache", "\"\"", workingDir = projectDir


So you mean to remove the "dummy" "\"\"" argument in favor of adding a = after the option name?

Using the default `bazel --version` is simpler and more resilient in case of mis-configuration, such as the disk cache pointing to a non-existing directory. Signed-off-by: Frank Viernau <[email protected]>

When the disk-cache is set via `.bazelrc` to a non-existing directory, `bazel mod graph` fails with a complaint about permissions, which in turn makes the analysis fail. The disk cache is a build cache which seems to target speeding up builds in case of code changes (such as switching branches) [1]. During ORT analysis code changes are not in play, so the disk cache seems to be of little use anyway. Simply disable it to avoid running into that permission issue. Fixes: #8802. [1] https://stackoverflow.com/questions/61282419/bazel-how-outputroot-and-disk-cache-relate-regarding-local-caching Signed-off-by: Frank Viernau <[email protected]>

fviernau requested a review from a team as a code owner June 28, 2024 07:21

github-advanced-security bot found potential problems Jun 28, 2024

View reviewed changes

plugins/package-managers/bazel/src/main/kotlin/Bazel.kt Fixed Show fixed Hide fixed

sschuberth requested a review from nnobelis June 28, 2024 07:23

fviernau mentioned this pull request Jun 28, 2024

Bazel: It should be possible to disable the disk cache #8802

Closed

fviernau force-pushed the bazel-set-disk-cache branch from 1aa30ed to 17f0735 Compare June 28, 2024 07:27

nnobelis reviewed Jun 28, 2024

View reviewed changes

fviernau force-pushed the bazel-set-disk-cache branch from 17f0735 to 60f1cd1 Compare June 28, 2024 07:54

fviernau changed the title ~~fix(bazel): Use a fixed disk cache directory~~ fix(bazel): Always disable the disk cache Jun 28, 2024

fviernau requested a review from nnobelis June 28, 2024 07:56

nnobelis reviewed Jun 28, 2024

View reviewed changes

fviernau force-pushed the bazel-set-disk-cache branch from 60f1cd1 to c727623 Compare June 28, 2024 08:04

fviernau requested a review from nnobelis June 28, 2024 08:06

fviernau force-pushed the bazel-set-disk-cache branch from c727623 to 63a30bd Compare June 28, 2024 08:08

haikoschol reviewed Jun 28, 2024

View reviewed changes

sschuberth reviewed Jun 28, 2024

View reviewed changes

fviernau added 2 commits June 28, 2024 10:39

chore(bazel): Simplify obtaining the Bazel version

e6283d0

Using the default `bazel --version` is simpler and more resilient in case of mis-configuration, such as the disk cache pointing to a non-existing directory. Signed-off-by: Frank Viernau <[email protected]>

fviernau force-pushed the bazel-set-disk-cache branch from 63a30bd to 5a6dbac Compare June 28, 2024 08:42

fviernau requested a review from sschuberth June 28, 2024 08:43

sschuberth approved these changes Jun 28, 2024

View reviewed changes

sschuberth enabled auto-merge (rebase) June 28, 2024 08:50

sschuberth merged commit a6894a2 into main Jun 28, 2024
20 checks passed

sschuberth deleted the bazel-set-disk-cache branch June 28, 2024 08:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(bazel): Always disable the disk cache #8811

fix(bazel): Always disable the disk cache #8811

fviernau commented Jun 28, 2024 •

edited

Loading

fviernau commented Jun 28, 2024 •

edited

Loading

nnobelis commented Jun 28, 2024

codecov bot commented Jun 28, 2024 •

edited

Loading

fviernau commented Jun 28, 2024

nnobelis Jun 28, 2024

nnobelis Jun 28, 2024

nnobelis commented Jun 28, 2024 •

edited

Loading

haikoschol commented Jun 28, 2024

fviernau commented Jun 28, 2024

nnobelis commented Jun 28, 2024

nnobelis commented Jun 28, 2024

sschuberth commented Jun 28, 2024

fviernau commented Jun 28, 2024

nnobelis Jun 28, 2024

sschuberth Jun 28, 2024

nnobelis Jun 28, 2024

fviernau Jun 28, 2024

haikoschol commented Jun 28, 2024

fviernau commented Jun 28, 2024 •

edited

Loading

haikoschol Jun 28, 2024

sschuberth Jun 28, 2024 •

edited

Loading

sschuberth Jun 28, 2024

		@@ -20,6 +20,7 @@
		package org.ossreviewtoolkit.plugins.packagemanagers.bazel

fix(bazel): Always disable the disk cache #8811

fix(bazel): Always disable the disk cache #8811

Conversation

fviernau commented Jun 28, 2024 • edited Loading

fviernau commented Jun 28, 2024 • edited Loading

nnobelis commented Jun 28, 2024

codecov bot commented Jun 28, 2024 • edited Loading

Codecov Report

fviernau commented Jun 28, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nnobelis commented Jun 28, 2024 • edited Loading

haikoschol commented Jun 28, 2024

fviernau commented Jun 28, 2024

nnobelis commented Jun 28, 2024

nnobelis commented Jun 28, 2024

sschuberth commented Jun 28, 2024

fviernau commented Jun 28, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

haikoschol commented Jun 28, 2024

fviernau commented Jun 28, 2024 • edited Loading

Choose a reason for hiding this comment

sschuberth Jun 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fviernau commented Jun 28, 2024 •

edited

Loading

fviernau commented Jun 28, 2024 •

edited

Loading

codecov bot commented Jun 28, 2024 •

edited

Loading

nnobelis commented Jun 28, 2024 •

edited

Loading

fviernau commented Jun 28, 2024 •

edited

Loading

sschuberth Jun 28, 2024 •

edited

Loading