Skip to content

Conversation

@penguinland
Copy link
Contributor

I was looking into RDSK-10413 for the tflite_cpu repo (see viam-modules/mlmodel-tflite#28), and noticed this is basically the same ticket for a different repo.

This file was generated by running conan lock create .. I don't like that the file doesn't end with a newline, but I also don't like manually editing files created by machines.

I think pinning all versions of libraries we depend on is a useful feature, though I'm not good at modern C++ (let alone Conan itself), and might have missed something. I'm tagging @lia-viam, as our resident Conan expert, and @acmorrow as the ticket creator.

@penguinland penguinland requested a review from a team as a code owner April 22, 2025 18:41
@penguinland penguinland requested review from njooma and removed request for a team April 22, 2025 18:41
Copy link
Member

@acmorrow acmorrow left a comment

Choose a reason for hiding this comment

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

Having thought about this some more, I'm now doubting my self about how I wrote that ticket. For rust-y things at least, you want lockfiles on programs not libraries. Maybe what I was really thinking was that tflite_cpu needed a lockfile so builds of that binary were reproducible?

Comment on lines +59 to +60
"abseil/20250127.0#7f3643d6f847196fe0bdc2b83c109831%1742212255.256",
"abseil/20240116.2#dfc978c35194fee939d236253044b967%1740151184.642"
Copy link
Member

Choose a reason for hiding this comment

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

This is very interesting, and is maybe concerning, though I can't quite tell what the implications for having two versions of abseil in build_requires are w.r.t. to the scary ODR consequences of having two abseils in the graph. Hopefully @lia-viam can provide further insights.

I note though that there are other duplications in this list, particularly meson and pkgconf, which do make more sense as things that you could have two of without issue since they are presumably only used at build time.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Stuff in the build_requires section is only used in tests and compile time tools (e.g., the cmake and pkgconf packages).

I've tried running conan lock create . -vtrace, but haven't yet been able to track down where these dependencies are coming from. but my guess is that we have two tools used at build time that depend on the two different versions.

The ones I'd be concerned about are up in the requires section, and they're all unique.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, conan graph info is more useful! protobuf depends on any version of abseil between 20230802.1 and 20250127.0, whereas literally everything else (including grpc, oddly) depends on the much narrower range 20240116.1 to 20240117.0. The issue is just that protobuf is being very permissive with which versions it accepts.

My hope is that conan is smart enough to realize that a single version can satisfy all these requirements, though I don't actually know.

@lia-viam
Copy link
Collaborator

@acmorrow

Maybe what I was really thinking was that tflite_cpu needed a lockfile so builds of that binary were reproducible?

This is what I vaguely remember of our discussion as well. I'm not super familiar with lockfiles but tentatively going to say I'm opposed to doing it here, and for tflite_cpu we could maybe do it, or provide guidance for how to do it if you want a local, reproducible build, but not necessarily check it in to source control.

Will try to research conan lockfiles more and report back

@penguinland
Copy link
Contributor Author

Lia and I talked: she has convinced me that this PR should be closed but viam-modules/mlmodel-tflite#28 should remain open.

The versioning specified in conanfile.py is good enough for most things, and the lock file is only for when you absolutely need the exact same bytes every time. When you're building a standalone executable, you want the build to succeed every time, so make a lockfile so it's always the same. but when you're building a library that will get linked into someone else's code, that someone else might want slightly different versions of the dependencies. As long as those different versions still match our conanfile.py, we should allow them.

So, get rid of this PR, but keep the one for the tflite module.

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