Bazel improvements #9028

sschuberth · 2024-08-23T16:39:50Z

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

codecov · 2024-08-23T16:52:00Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 68.02%. Comparing base (7ad4bfe) to head (7b4a250).
Report is 9 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff            @@
##               main    #9028   +/-   ##
=========================================
  Coverage     68.02%   68.02%           
  Complexity     1158     1158           
=========================================
  Files           241      241           
  Lines          7690     7690           
  Branches        867      867           
=========================================
  Hits           5231     5231           
  Misses         2103     2103           
  Partials        356      356

Flag	Coverage Δ
funTest-docker	`67.39% <ø> (ø)`
funTest-non-docker	`33.92% <ø> (ø)`
test	`37.91% <ø> (ø)`

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.

nnobelis · 2024-08-26T05:01:10Z

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

-                    "The module '${it.name}' has multiple URLs ${archiveOverride.urls} defined in the archive " +
-                        "override, but ORT only supports one URL for source artefacts. Only the first URL will be used."
+                    "The module '${it.name}' has multiple archive override URLs defined. Only the first URL of the " +
+                        "following URLs will be used: ${archiveOverride.urls.joinToString()}"


While I agree with this change, I feel that the patches should be logged as it has an impact on compliance, even if ORT does not support them.
Could we create an issue with the patch urls here?

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

Line 224 in 1ec14b5

"The module $moduleName has patches defined in the archive override, but ORT does not support " +

Creating an issue would certainly be better / more visible than logging. Compliance people don't look at ORT logs in my experience, and even if they did, showing the patch's file names (or is it even the diff) IMO does not bring much value.

What's more important is that there are patches, i.e. the source code is modified compared to upstream, and that's exactly what 51c01dd is about. IMO that's the proper way to signal to compliance people that they have to "watch out".

Regarding ORT not supporting patches, that's only true in the scanner context: I.e. ORT does not apply any patches before scanning, and will always scan the upstream source code without the patches. But that's only relevant if something that a scanner would detect gets modified by the patch. But mostly, such patches are rather small, and apply bug fixes or similar.

What's more important is that there are patches, i.e. the source code is modified compared to upstream, and that's exactly what 51c01dd is about. IMO that's the proper way to signal to compliance people that they have to "watch out".

Is is_modified reflected in the WebApp report ?
Cf my question just underneath (I didn't know this property)

Is is_modified reflected in the WebApp report ?

I believe the Web App report does not show this (yet), but e.g. the CycloneDxReporter and CtrlXAutomationReporter do.

So, are you fine with only making use of isModified and not logging any patches?

I am fine with it if I can see the patch URLs in the ORT issues then. Deal ?

You don't have to implement it, a TODO is fine.

I'm still not convinced that using issues is the right "communication channel" here.

Instead, besides making isModified=true packages more visible in reports (for any package manager), we could think about further extending the data model to also track the actual patches (or better URLs to patches). That would be a more generic variant of #8452 and also allow the scanner to apply the patches before scanning.

...agers/bazel/src/funTest/assets/projects/synthetic/bazel-expected-output-archive-override.yml

nnobelis · 2024-08-26T05:09:58Z

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

I don't agree with this change.

Yes they could be alternative URLs but they could also point to a fork with some changes.

You have no way of knowing if the archive override point to the version of the package or not (as shown by the word "sometimes" in your commit message. You should never claim this is the same version as an official release, as it is lying in the sense of compliance.

In the test file, this is NOT rules_cude version 0.1.1 as there is some patch alongside.

To be frank, I find that Bazel did the right think by erasing the version, as you simply don't know what you are scanning.

You have no way of knowing if the archive override point to the version of the package or not

Right. But taking it to the extreme, you could not even tell whether it's the same package. It could be some unrelated URL that points to a completely different (and potentially malicious) package. In that sense we could not even rely on the name. But I'd prefer to not actually go that far.

In the test file, this is NOT rules_cude version 0.1.1 as there is some patch alongside.

So it is a patched variant of version 0.1.1, which is exactly what the combination of package properties version=0.1.1 and isModified=true expresses.

In the end, I believe there are two main uses cases of archive overrides:

Provides alternative URLs of unmodified packages, like internal mirrors.

Point to a patched version of the package hosted elsewhere.

In both cases the original version applies, IMO, and the only difference is the value of the isModified property.

I understand your reasoning and

So it is a patched variant of version 0.1.1, which is exactly what the combination of package properties version=0.1.1 and isModified=true expresses.

if a very valid point.
However I worry that if the modified packages are not visible clearly as such in the Webapp report, they will fly under the radar and we will have customer questions about it. Though, I know this is a bit stupid because I talk about rendering and you talk about the model.

Now a more provocative question: Curations, Package Configurations and Concluded licences: should they be applied on a package with modified=true?

Though, I know this is a bit stupid because I talk about rendering and you talk about the model.

It's not stupid, but as you say, it should be addressed elsewhere. We should not tamper with the data in the model just because (some) reports (currently) do not visualize the data properly.

Now a more provocative question:

Good question, but please let's not start discussing that as part of this PR 🙏🏻 Instead, it's probably a good topic for the upcoming community meeting.

fviernau · 2024-08-26T08:23:32Z

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

@@ -314,7 +314,7 @@ class Bazel(
            }

            Package(
-                id = it.copy(version = ""),
+                id = it,


commit-message: Why is it not reasonable to always maintain the version, but only in the mentioned cases?

With my change, the version now is always maintained, i.e. I've removed the only case where it was not maintained. I'll clarify that in the commit message. However, @nnobelis argues against maintaining the version. I'll reply to that separately.

With my change, the version now is always maintained,

Yes, I was only after reflecting this is the commit message, as you mentioned:

i.e. I've removed the only case where it was not maintained. I'll clarify that in the commit message.

I'm not sure whether I understand your reply correctly... is this review comment resolved now, as it was a misunderstanding originally?

The commit message says "In these cases it is beneficial to maintain the version instead
of removing it."... IMO this could be slightly misleading, e.g. sound like "only in these cases the version should be kept, but actually its kept in all cases." Maybe adding an "also" could help?

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

Only log the URL(s); other properties are not really worth logging and may literally be "(missing)". Regarding tracking of patched dependencies, an upcoming commit will leverage the dedicated `isModified` package property for that. Signed-off-by: Sebastian Schuberth <[email protected]>

Signed-off-by: Sebastian Schuberth <[email protected]>

…ified Signed-off-by: Sebastian Schuberth <[email protected]>

Sometimes archive overrides just provides alternative URLs for a given version. In these cases it is beneficial to maintain the version instead of removing it. Signed-off-by: Sebastian Schuberth <[email protected]>

This is to demonstrate that development dependencies are not correctly detected if they stem from an archive override. An upcoming commit will fix that. Signed-off-by: Sebastian Schuberth <[email protected]>

In order to use the correct keys for the scope lookup, name and version overrides need to be applied earlier. This fixes the recognition of development dependencies with archive overrides. Signed-off-by: Sebastian Schuberth <[email protected]>

nnobelis · 2024-08-26T12:12:19Z

For me it can be merged

fviernau · 2024-08-26T14:39:36Z

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

@@ -314,7 +314,7 @@ class Bazel(
            }

            Package(
-                id = it.copy(version = ""),
+                id = it,


The commit message says "In these cases it is beneficial to maintain the version instead
of removing it."... IMO this could be slightly misleading, e.g. sound like "only in these cases the version should be kept, but actually its kept in all cases." Maybe adding an "also" could help?

sschuberth · 2024-08-26T14:46:11Z

Maybe adding an "also" could help?

Sorry, too late due to auto-merge. But I had actually added "also" to the commit's title at least.

sschuberth requested a review from a team as a code owner August 23, 2024 16:39

sschuberth requested review from nnobelis and a team and removed request for a team August 23, 2024 16:39

sschuberth enabled auto-merge (rebase) August 23, 2024 16:40

nnobelis reviewed Aug 26, 2024

View reviewed changes

fviernau requested changes Aug 26, 2024

View reviewed changes

sschuberth force-pushed the bazel-imps branch from 67e9c01 to a59c5df Compare August 26, 2024 08:29

sschuberth added 6 commits August 26, 2024 10:30

chore(bazel): Do not quote URLs in logs for visual simplicity

dacb8e1

Signed-off-by: Sebastian Schuberth <[email protected]>

feat(bazel): Treat a package with archive override and patches as mod…

2ee3713

…ified Signed-off-by: Sebastian Schuberth <[email protected]>

fix(bazel): Maintain the version also in case of archive overrides

53e7619

Sometimes archive overrides just provides alternative URLs for a given version. In these cases it is beneficial to maintain the version instead of removing it. Signed-off-by: Sebastian Schuberth <[email protected]>

test(bazel): Add another archive override with dev_dependency=True

d0879c4

This is to demonstrate that development dependencies are not correctly detected if they stem from an archive override. An upcoming commit will fix that. Signed-off-by: Sebastian Schuberth <[email protected]>

sschuberth force-pushed the bazel-imps branch from a59c5df to 7b4a250 Compare August 26, 2024 09:35

sschuberth requested review from nnobelis and fviernau August 26, 2024 09:36

sschuberth requested a review from a team August 26, 2024 13:00

fviernau approved these changes Aug 26, 2024

View reviewed changes

sschuberth merged commit 34a222e into main Aug 26, 2024
23 checks passed

sschuberth deleted the bazel-imps branch August 26, 2024 14:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bazel improvements #9028

Bazel improvements #9028

sschuberth commented Aug 23, 2024

codecov bot commented Aug 23, 2024 •

edited

Loading

nnobelis Aug 26, 2024

sschuberth Aug 26, 2024

nnobelis Aug 26, 2024

sschuberth Aug 26, 2024

sschuberth Aug 26, 2024

nnobelis Aug 26, 2024

sschuberth Aug 26, 2024

nnobelis Aug 26, 2024

sschuberth Aug 26, 2024

nnobelis Aug 26, 2024 •

edited

Loading

sschuberth Aug 26, 2024 •

edited

Loading

fviernau Aug 26, 2024

sschuberth Aug 26, 2024

fviernau Aug 26, 2024

sschuberth Aug 26, 2024

fviernau Aug 26, 2024

nnobelis commented Aug 26, 2024

fviernau Aug 26, 2024

sschuberth commented Aug 26, 2024 •

edited

Loading

Bazel improvements #9028

Bazel improvements #9028

Conversation

sschuberth commented Aug 23, 2024

codecov bot commented Aug 23, 2024 • edited Loading

Codecov Report

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

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

Choose a reason for hiding this comment

nnobelis Aug 26, 2024 • edited Loading

Choose a reason for hiding this comment

sschuberth Aug 26, 2024 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nnobelis commented Aug 26, 2024

Choose a reason for hiding this comment

sschuberth commented Aug 26, 2024 • edited Loading

codecov bot commented Aug 23, 2024 •

edited

Loading

nnobelis Aug 26, 2024 •

edited

Loading

sschuberth Aug 26, 2024 •

edited

Loading

sschuberth commented Aug 26, 2024 •

edited

Loading