Skip to content

Fix/bazel parser#11826

Open
losmateusz1 wants to merge 4 commits into
oss-review-toolkit:mainfrom
losmateusz1:fix/bazel_parser_verbosity
Open

Fix/bazel parser#11826
losmateusz1 wants to merge 4 commits into
oss-review-toolkit:mainfrom
losmateusz1:fix/bazel_parser_verbosity

Conversation

@losmateusz1
Copy link
Copy Markdown
Contributor

  • Add variable definition parsing inside Module.bazel
  • Add more verbose logs when unsupported configuration is found

@losmateusz1 losmateusz1 force-pushed the fix/bazel_parser_verbosity branch from 0e6793b to 3814da7 Compare May 12, 2026 10:22
Comment thread plugins/package-managers/bazel/src/main/kotlin/StarlarkParser.kt Fixed
Comment thread plugins/package-managers/bazel/src/main/kotlin/StarlarkParser.kt Fixed
Comment thread plugins/package-managers/bazel/src/main/kotlin/StarlarkParser.kt Fixed
Comment thread plugins/package-managers/bazel/src/main/kotlin/StarlarkParser.kt Fixed
@codecov
Copy link
Copy Markdown

codecov Bot commented May 12, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 58.43%. Comparing base (33f9795) to head (cd77d67).
⚠️ Report is 3 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff            @@
##               main   #11826   +/-   ##
=========================================
  Coverage     58.43%   58.43%           
  Complexity     1809     1809           
=========================================
  Files           361      361           
  Lines         13499    13499           
  Branches       1383     1383           
=========================================
  Hits           7888     7888           
  Misses         5115     5115           
  Partials        496      496           
Flag Coverage Δ
funTest-no-external-tools 30.37% <ø> (-0.03%) ⬇️
test-ubuntu-24.04 41.78% <ø> (ø)

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.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@losmateusz1 losmateusz1 force-pushed the fix/bazel_parser_verbosity branch from 3814da7 to 39ba368 Compare May 20, 2026 09:57
Comment thread plugins/package-managers/bazel/src/main/kotlin/StarlarkParser.kt Fixed
Comment thread plugins/package-managers/bazel/src/main/kotlin/StarlarkParser.kt Fixed
Comment thread plugins/package-managers/bazel/src/main/kotlin/StarlarkParser.kt Fixed
@losmateusz1 losmateusz1 force-pushed the fix/bazel_parser_verbosity branch 3 times, most recently from 19fbdee to bdc27c1 Compare May 20, 2026 10:28
@losmateusz1 losmateusz1 marked this pull request as ready for review May 20, 2026 11:25
@losmateusz1 losmateusz1 requested a review from a team as a code owner May 20, 2026 11:25
Copy link
Copy Markdown
Member

@sschuberth sschuberth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nnobelis, please also have a look.

Comment thread plugins/package-managers/bazel/src/main/kotlin/StarlarkParser.kt Outdated
Comment thread plugins/package-managers/bazel/src/main/kotlin/StarlarkParser.kt Outdated
@losmateusz1 losmateusz1 force-pushed the fix/bazel_parser_verbosity branch from bdc27c1 to 625ae23 Compare May 20, 2026 18:02
Comment thread plugins/package-managers/bazel/src/main/kotlin/StarlarkParser.kt Outdated
Comment thread plugins/package-managers/bazel/src/main/kotlin/StarlarkParser.kt Outdated
@sschuberth sschuberth requested a review from a team May 20, 2026 18:30
@losmateusz1 losmateusz1 force-pushed the fix/bazel_parser_verbosity branch 3 times, most recently from 81dafb8 to ed71738 Compare May 27, 2026 13:07
Comment thread plugins/package-managers/bazel/src/main/kotlin/StarlarkParser.kt

package org.ossreviewtoolkit.plugins.packagemanagers.bazel

import org.ossreviewtoolkit.utils.common.enumSetOf
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Commit message: Here, the first "Bazel" should be "bazel" (as it refers to the scope / directory name), and the second "bazel" should be "Bazel" (as it refers to the build system name) 😆

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, please explain in the commit message what's the effect of adding variable parsing. Does it avoid an exception? Or does it provide more data now? What else?

Comment thread plugins/package-managers/bazel/src/main/kotlin/StarlarkParser.kt Outdated
Comment thread plugins/package-managers/bazel/src/main/kotlin/StarlarkParser.kt
@losmateusz1 losmateusz1 force-pushed the fix/bazel_parser_verbosity branch 2 times, most recently from e75853e to 1ec446a Compare June 2, 2026 18:26
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The MODULE.bazel.lock files should not be deleted as part of this commit.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code stats still look suspicious:

image

Copy link
Copy Markdown
Contributor Author

@losmateusz1 losmateusz1 Jun 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in my opinion we shouldn't keep lock files in repository, as they are regenerated during tests anyway. After running all tests we are generating more lockfiles than those already in the repository, so I am not sure why we should keep those two specifically, they are not being used for anything in other tests

for now I just moved removals to separate commit, I thought that this is what you are requesting :)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure why we should keep those two specifically, they are not being used for anything in other tests

I believe they're need for getting reproducible results in tests even if new version of dependencies have been published, correct @nnobelis?

Anyway, even if removing them, that must be done in a separate commit with a dedicated rationale.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it is in separate commit now, would you like me to remove it from here and add separate PR for the cleanup?

Copy link
Copy Markdown
Member

@sschuberth sschuberth Jun 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would like to get @nnobelis's feedback first.

Signed-off-by: Mateusz Los <mateusz.los@extern.wenovate.de>
Signed-off-by: Mateusz Los <mateusz.los@extern.wenovate.de>
Signed-off-by: Mateusz Los <mateusz.los@extern.wenovate.de>
Signed-off-by: Mateusz Los <mateusz.los@extern.wenovate.de>
@losmateusz1 losmateusz1 force-pushed the fix/bazel_parser_verbosity branch from 1ec446a to cd77d67 Compare June 3, 2026 08:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants