Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(bazel): Add Conan dependencies to Bazel's dependency tree #10126

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

nnobelis
Copy link
Member

@nnobelis nnobelis commented Apr 3, 2025

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

@nnobelis nnobelis force-pushed the nnobelis/bazel_conan_dependencies branch 2 times, most recently from 4d1e0a4 to f3c982a Compare April 3, 2025 14:42
Copy link

codecov bot commented Apr 3, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 69.55%. Comparing base (de3a0c6) to head (f0c5148).
Report is 43 commits behind head on main.

Additional details and impacted files
@@             Coverage Diff              @@
##               main   #10126      +/-   ##
============================================
- Coverage     69.59%   69.55%   -0.04%     
  Complexity     1454     1454              
============================================
  Files           270      270              
  Lines          9670     9678       +8     
  Branches       1029     1033       +4     
============================================
+ Hits           6730     6732       +2     
- Misses         2488     2494       +6     
  Partials        452      452              
Flag Coverage Δ
funTest-docker 68.80% <ø> (ø)
funTest-non-docker 33.32% <ø> (ø)
test-ubuntu-24.04 39.13% <ø> (ø)
test-windows-2022 39.11% <ø> (ø)

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.

@nnobelis nnobelis force-pushed the nnobelis/bazel_conan_dependencies branch 2 times, most recently from 8d18240 to 1f5e04c Compare April 4, 2025 14:54
@nnobelis nnobelis marked this pull request as ready for review April 4, 2025 15:43
@nnobelis nnobelis requested a review from a team as a code owner April 4, 2025 15:43
@nnobelis nnobelis force-pushed the nnobelis/bazel_conan_dependencies branch from 1f5e04c to adec183 Compare April 7, 2025 07:54
@nnobelis nnobelis requested a review from sschuberth April 7, 2025 07:55
@sschuberth

This comment was marked as resolved.

@nnobelis nnobelis force-pushed the nnobelis/bazel_conan_dependencies branch from adec183 to 8719ccf Compare April 7, 2025 09:02
Copy link
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.

@mnonnenmacher I'd like you to also have a look.

val conanPackages = mutableSetOf<Package>()

conanExtension?.let { extension ->
val conanProjectReferences = mutableSetOf<PackageReference>()
Copy link
Member

Choose a reason for hiding this comment

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

How about moving this whole long block to a function for readability?

Copy link
Member Author

Choose a reason for hiding this comment

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

The problem is that resolveConanDependencies has a lot of parameters to carry on. Do you still think this would be worthwhile ?

Copy link
Member

Choose a reason for hiding this comment

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

I'd say it depends on the concrete number... if it's more than 7 parameters, then not, probably.

* Only scan Bazel dependencies and skip reporting Conan packages in the dependency tree.
*/
@OrtPluginOption(defaultValue = "false")
val bazelDependenciesOnly: Boolean
Copy link
Member

Choose a reason for hiding this comment

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

Related to my comment on the previous commit, can't we archive the same by simply disabling Conan as a package manager? Or do you think there are use-cases where there are both Bazel and Conan projects in one repository, but they should be recognized independently?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is a case with one of our customer: they have a repository with Bazel and Conan definition files + Conan packages in the Bazel project.

Additionally I created the bazelDependenciesOnly parameter because it also exist in Pub, so you can see it as some kind of alignment.

@@ -56,6 +56,7 @@ import org.ossreviewtoolkit.plugins.api.OrtPlugin
import org.ossreviewtoolkit.plugins.api.OrtPluginOption
import org.ossreviewtoolkit.plugins.api.PluginDescriptor
import org.ossreviewtoolkit.utils.common.CommandLineTool
import org.ossreviewtoolkit.utils.common.alsoIfNull
Copy link
Member

Choose a reason for hiding this comment

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

Esp. with this commit, I don't really see why one would need to disable the Bazel <-> Conan interaction.

@nnobelis nnobelis force-pushed the nnobelis/bazel_conan_dependencies branch 2 times, most recently from a93baa6 to b55755a Compare April 11, 2025 09:32
@nnobelis nnobelis requested a review from mnonnenmacher April 11, 2025 09:32
@nnobelis nnobelis force-pushed the nnobelis/bazel_conan_dependencies branch from b55755a to 8f426e7 Compare April 11, 2025 11:10
@nnobelis nnobelis requested a review from sschuberth April 11, 2025 11:10
@sschuberth sschuberth dismissed their stale review April 11, 2025 11:55

Comments addressed.

mnonnenmacher
mnonnenmacher previously approved these changes Apr 11, 2025
Copy link
Member

@mnonnenmacher mnonnenmacher left a comment

Choose a reason for hiding this comment

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

Looks good to me, but I don't know much about Bazel. @sschuberth I assume you would also be fine with merging this as you dismissed your review?

val dependencies: List<BazelModule> = emptyList(),
val extensionUsages: List<BazelExtension> = emptyList()
)
@Serializable
Copy link
Member

Choose a reason for hiding this comment

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

Nit: Missing blank line before this one.

@sschuberth
Copy link
Member

@sschuberth I assume you would also be fine with merging this as you dismissed your review?

I've dismissed my review as I current cannot spend the time to do yet another thorough review. I'm ok if you want to merge.

It is possible to use Conan dependencies with Bazel. This commit changes
the Bazel package manager to list the Conan dependencies with the Conan
package manager and complete the Bazel dependency tree with them. The
`PackageManagerDependency` approach in Pub/Gradle package manager cannot be
used here as the Conan packages have to be filtered in correlation with the
'bazel mod graph' output.
The test project has been generated thanks to the documentation at [1]. In
addition to the files present in the source repository, the other files
were generated by `conan install . --build=missing`. This command requires
Conan version 2 and is a prerequisite for having the Conan dependencies
available to Bazel. Please note that this command generates the needed
Bazel files and builds the Conan dependencies. Consequently, it cannot be
run by ORT: it is the responsibility of the repository owner to add the
generated *.bazel and *.bzl files to the repository.

There is a documentation at [2] about making Conan 1 work with Bazel,
however the resulting project does not use `bzlmod` on which ORT relies.

[1]: https://docs.conan.io/2/examples/tools/google/bazeltoolchain/build_simple_bazel_7x_project.html#examples-tools-bazel-7x-toolchain-build-simple-bazel-project
[2]: https://docs.conan.io/1/integrations/build_system/bazel.html

Signed-off-by: Nicolas Nobelis <[email protected]>
If a Bazel project uses some Conan packages, the corresponding Conan files
should not be picked up by the Conan package manager. Therefore, the Conan
file is checked to NOT contain the BazelDeps and BazelToolchain generators.

Signed-off-by: Nicolas Nobelis <[email protected]>
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.

3 participants