Bazel: various fixes #9243

nnobelis · 2024-10-04T13:32:06Z

Please have a look at the individual commit messages for the details.

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

sschuberth · 2024-10-04T13:41:04Z

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

@@ -50,6 +52,11 @@ internal class MultiBazelModuleRegistryService(
            // Add the default Bazel registry as a fallback.
            val registryUrls = (urls + DEFAULT_URL).distinctBy { it.removeSuffix("/") }

+            logger.info { "Creating a multi Bazel module registry:" }


IMO this should be debug instead of info.

Since the logging occurs only one, I think there is no harm in letting it to info, especially considering how noisy debug is.

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

clients/bazel-module-registry/src/main/kotlin/Model.kt

sschuberth · 2024-10-04T13:47:44Z

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

@@ -110,9 +110,9 @@ class Bazel(
        super.run(
            args = args,
            workingDir = workingDir,
-            // Disable the optional wrapper script under `tools/bazel`, to ensure the --version option works.
+            // Disable the optional wrapper script under `tools/bazel` only for the "--version" call.


can be relevant by defining

I don't understand this. Do you mean "can be relevant e.g. in case a specific '.bazelrc'
depending on the value of an environment variable is defined"?

I tried to explain the setup of our customer project and the phrasing is quite difficult...

They have in their project a wrapper script in tools/bazel. This script reads an environment variable and, depending on its value, change the .bazelrc of the project.
Therefore when you call bazel mod graph, this script "setups" your build environment depending on this environment variable.
However, bazel uses this script for all commands even if you call bazel version. In that case, the script fails because it does not support the --version parameter. While the correct fix would be to change the wrapper script to support this parameter, this is not possible to force all the projects to do so.
And, in all cases, ORT must make sure that the wrapper script is enabled when mod graph is called, as it may contain some build changing logic as described above.

codecov · 2024-10-04T13:55:16Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 67.65%. Comparing base (46afc3a) to head (e4046fe).
Report is 10 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff            @@
##               main    #9243   +/-   ##
=========================================
  Coverage     67.65%   67.65%           
  Complexity     1185     1185           
=========================================
  Files           239      239           
  Lines          7795     7795           
  Branches        899      899           
=========================================
  Hits           5274     5274           
  Misses         2153     2153           
  Partials        368      368

Flag	Coverage Δ
funTest-docker	`60.23% <ø> (ø)`
funTest-non-docker	`34.71% <ø> (ø)`
test	`37.07% <ø> (ø)`

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.

clients/bazel-module-registry/src/main/kotlin/BazelModuleRegistryService.kt

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

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

plugins/package-managers/bazel/src/funTest/kotlin/BazelFunTest.kt

clients/bazel-module-registry/src/main/kotlin/RemoteBazelModuleRegistryService.kt

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

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

clients/bazel-module-registry/src/main/kotlin/RemoteBazelModuleRegistryService.kt

sschuberth

Sorry for some follow-up requests for changes.

sschuberth · 2024-10-07T11:33:44Z

clients/bazel-module-registry/src/main/kotlin/LocalBazelModuleRegistryService.kt

@@ -35,7 +35,7 @@ const val SOURCE_JSON = "source.json"
 /**
 * A Bazel registry which is located on the local file system.
 */
-class LocalBazelModuleRegistryService(directory: File) : BazelModuleRegistryService {
+class LocalBazelModuleRegistryService(val url: String, directory: File) : BazelModuleRegistryService {


Sorry, same thing here: Drop val. The inspection did not trigger here as the properly is public currently.

Also, please document the url and directory constructor parameters to explain their relation (e.g., I was wondering whether the directory can't be deduced from the file://-type url).

sschuberth · 2024-10-07T11:42:32Z

clients/bazel-module-registry/src/main/kotlin/RemoteBazelModuleRegistryService.kt

+ * A remote Bazel Module Registry, based on the directory structure of https://bcr.bazel.build/ and the Git repository
+ * it is based on (https://github.com/bazelbuild/bazel-central-registry/).


Also here, while rewording the docs anyway, please incorporate the client and baseUrl parameters to explain their meaning.

clients/bazel-module-registry/src/main/kotlin/LocalBazelModuleRegistryService.kt

clients/bazel-module-registry/src/main/kotlin/RemoteBazelModuleRegistryService.kt

sschuberth · 2024-10-07T13:00:08Z

Please have a look at the MockKException in MultiBazelModuleRegistryServiceTest.

The `MultiBazelModuleRegistryService` tries several Bazel registries one after each other in order to query the `metadata.json` and the `source.json` of a module. If this information cannot be resolved, the server collates all the error messages and outputs them. To facilitate the analysis of the logs, each error message is now prefixed with the URL of the registry that created it. The URL property is now provided by each Bazel registry service. This required making `RemoteBazelModuleRegistryService.kt` a proper class that delegates the module lookups to a Retrofit client. Signed-off-by: Nicolas Nobelis <[email protected]>

sschuberth · 2024-10-07T13:35:01Z

Sorry again for finding more (optional) stuff:

chore(bazel): Support github in the module metadata JSON file

Shouldn't this be "feat" instead of "chore?

chore(bazel): Disable the wrapper script only for the --version call

Shouldn't this say "fix" instead of "chore"? Otherwise there wouldn't be a single "fix" commit in this "Bazel: various fixes" PR 😉

This is currently unused (as the whole `Maintainer` class) but it contains information that could be relevant in the future. See [1]. [1]: https://github.com/bazelbuild/bazel-central-registry/blob/main/metadata.schema.json Signed-off-by: Nicolas Nobelis <[email protected]>

The wrapper script can be relevant by defining e.g. a specific '.bazelrc' depending on the value of an environment variable. This is a fixup for 310470d. Signed-off-by: Nicolas Nobelis <[email protected]>

This is a test for 1045f89. Signed-off-by: Nicolas Nobelis <[email protected]>

nnobelis requested a review from a team as a code owner October 4, 2024 13:32

sschuberth requested changes Oct 4, 2024

View reviewed changes

nnobelis force-pushed the nnobelis/bazel_metadata branch from 3631770 to 1c2628d Compare October 7, 2024 05:44

nnobelis requested a review from sschuberth October 7, 2024 05:44

nnobelis force-pushed the nnobelis/bazel_metadata branch 2 times, most recently from 278302b to dffeaae Compare October 7, 2024 09:18

sschuberth requested changes Oct 7, 2024

View reviewed changes

nnobelis force-pushed the nnobelis/bazel_metadata branch from dffeaae to 12f2b37 Compare October 7, 2024 10:51

nnobelis requested a review from sschuberth October 7, 2024 10:53

github-advanced-security bot found potential problems Oct 7, 2024

View reviewed changes

clients/bazel-module-registry/src/main/kotlin/RemoteBazelModuleRegistryService.kt Fixed Show resolved Hide resolved

sschuberth requested changes Oct 7, 2024

View reviewed changes

nnobelis force-pushed the nnobelis/bazel_metadata branch from 12f2b37 to dfbd4a7 Compare October 7, 2024 11:27

nnobelis requested a review from sschuberth October 7, 2024 11:28

sschuberth requested changes Oct 7, 2024

View reviewed changes

nnobelis force-pushed the nnobelis/bazel_metadata branch from dfbd4a7 to 0fe3e93 Compare October 7, 2024 12:00

nnobelis requested a review from sschuberth October 7, 2024 12:00

github-advanced-security bot found potential problems Oct 7, 2024

View reviewed changes

nnobelis force-pushed the nnobelis/bazel_metadata branch from 0fe3e93 to 6b39da3 Compare October 7, 2024 13:23

sschuberth approved these changes Oct 7, 2024

View reviewed changes

sschuberth enabled auto-merge (rebase) October 7, 2024 13:31

sschuberth disabled auto-merge October 7, 2024 13:32

nnobelis added 3 commits October 7, 2024 15:36

fix(bazel): Disable the wrapper script only for the --version call

3ad055a

The wrapper script can be relevant by defining e.g. a specific '.bazelrc' depending on the value of an environment variable. This is a fixup for 310470d. Signed-off-by: Nicolas Nobelis <[email protected]>

test(bazel): Add a missing test for when there is no lockfile

e4046fe

This is a test for 1045f89. Signed-off-by: Nicolas Nobelis <[email protected]>

nnobelis force-pushed the nnobelis/bazel_metadata branch from 6b39da3 to e4046fe Compare October 7, 2024 13:37

nnobelis enabled auto-merge (rebase) October 7, 2024 13:38

nnobelis merged commit 1d71126 into oss-review-toolkit:main Oct 7, 2024
23 checks passed

nnobelis deleted the nnobelis/bazel_metadata branch October 7, 2024 14:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bazel: various fixes #9243

Bazel: various fixes #9243

nnobelis commented Oct 4, 2024

sschuberth Oct 4, 2024

nnobelis Oct 7, 2024

sschuberth Oct 4, 2024

nnobelis Oct 7, 2024

codecov bot commented Oct 4, 2024 •

edited

Loading

sschuberth left a comment

sschuberth Oct 7, 2024

sschuberth Oct 7, 2024

sschuberth Oct 7, 2024

sschuberth commented Oct 7, 2024

sschuberth commented Oct 7, 2024

		* A remote Bazel Module Registry, based on the directory structure of https://bcr.bazel.build/ and the Git repository
		* it is based on (https://github.com/bazelbuild/bazel-central-registry/).

Bazel: various fixes #9243

Bazel: various fixes #9243

Conversation

nnobelis commented Oct 4, 2024

sschuberth Oct 4, 2024

Choose a reason for hiding this comment

nnobelis Oct 7, 2024

Choose a reason for hiding this comment

sschuberth Oct 4, 2024

Choose a reason for hiding this comment

nnobelis Oct 7, 2024

Choose a reason for hiding this comment

codecov bot commented Oct 4, 2024 • edited Loading

Codecov Report

sschuberth left a comment

Choose a reason for hiding this comment

sschuberth Oct 7, 2024

Choose a reason for hiding this comment

sschuberth Oct 7, 2024

Choose a reason for hiding this comment

sschuberth Oct 7, 2024

Choose a reason for hiding this comment

sschuberth commented Oct 7, 2024

sschuberth commented Oct 7, 2024

codecov bot commented Oct 4, 2024 •

edited

Loading