Skip to content

Revert Changes to compile against libbpf-sys 1.1.0, update to 1.1.1 #306

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

Merged
merged 2 commits into from
Jan 11, 2023

Conversation

mimullin-bbry
Copy link
Contributor

Changes which fixed libbpf-sys compile breakage for 1.1.0 now cause breaks against 1.1.1.

These two commits fix those issues.

This reverts commit 271895a.

libbpf-sys has updated to 1.1.1 and the changes to compile
against 1.1.0 now cause breaks.

Signed-off-by: Michael Mullin <[email protected]>
@danielocfb
Copy link
Collaborator

Thanks (again) for the pull request. The revert should definitely go in, but I am not convinced that we need the bump. Version 1.1.1 of libbpf-sys can be selected irrespective of your change. So it seems as if all we are doing is unnecessarily forcing this version. We should bump only once we need functionality available only in a newer version, I would say. What do you think @mimullin-bbry ?

@mimullin-bbry
Copy link
Contributor Author

mimullin-bbry commented Jan 11, 2023

@danielocfb I personally encourage updating as soon as possible, as often as possible. Even if you don't need the changes. Small incremental and often applied updates are usually easier than large-only-occasionally applied updates. My personal view is that not being "up-to-date" is a tech-debt. That said, I understand other's disagree with this.

Up until yesterday, our team specifically did not check Cargo.lock into versioning control, and we use the "*" versioning in Cargo.toml in order to force us to keep up-to-date as soon as changes occur. Because of the issues with libbpf-sys and with num_enum_derive this week, we've checked in a Cargo.lock, and will be adding a nightly CI check for cargo update. Just so that you don't think I'm insane, we will be locking our dependencies in Cargo.toml when we get close to release date. :)

A lot of the above is coloured by my observation of the difficulties the chrono crate had by not keeping up to date with their dependency of the time crate. This difficulty in updating caused them to be listed as a security risk by cargo audit for an extended period of time, and a significant percentage of the chrono userbase moved away from that crate because of this.

However, for specifically this dependencies semver, "customers" of libbpf-rs will normally be picking up the latest version of libbpf-sys upon a new project, or upon a cargo update. It might be wise to keep up-to-date specifically with libbpf-sys regardless of how you feel about my above described philosophy.

@danielocfb
Copy link
Collaborator

Thanks for sharing your thoughts, @mimullin-bbry. The main point I am making is that it is very much how semver operates to have a version range available (it does not have to, but that's the power of it). Needlessly restricting this range is counter productive.

What we are saying when we specify

libbpf-sys = { version = "1.0.3" }

in Cargo.toml is that 1.0.3 is the minimum version supported. That does not prevent users from using a higher version. That's what semver is about: it establishes a contract that, if version 1.0.4 (or 1.1.1) is available, it can be used because it is compatible with 1.0.3 (a super set, if you will). If it wasn't, it should have been called 2.0.0 (or whatever). Please refer to the respective Cargo documentation on the matter.

By saying that 1.1.1 is the new minimum version, we artificially (i.e., without good reason) limit what version of libbpf-sys clients can use. That can cause version resolution issues if at the same time a downstream project is asking for a specific version (bad practice, but possible nevertheless).

It is fine (and necessary) to depend on 1.1.1 once we use a feature that is only available there (i.e., we use a part of the (true) super set not available on 1.0.3), but that's not the case here.

Note that all of this is a bit different in bin vs. lib settings. A library has downstream consumers that should be given the most flexibility possible. For a binary it really does not matter what version is specified, because nobody is consuming it.

I personally encourage updating as soon as possible, as often as possible. Even if you don't need the changes. Small incremental and often applied updates are usually easier than large-only-occasionally applied updates. My personal view is that not being "up-to-date" is a tech-debt. That said, I understand other's disagree with this.

Yes, no disagreement on the updating part, but I think we are conflating mechanisms and their intended purposes here, no? I am happy to bump the version we use in CI in Cargo.lock. What I don't think is correct, is to force this version to downstream users by enshrining it in Cargo.toml. Because again, it's about choice in the presence of multiple version that all can reasonably work.

However, for specifically this dependencies semver, "customers" of libbpf-rs will normally be picking up the latest version of libbpf-sys upon a new project, or upon a cargo update. It might be wise to keep up-to-date specifically with libbpf-sys regardless of how you feel about my above described philosophy.

Yeah, they can pick any allowed version, but I don't think we should be artificially restricting for them what is allowed.

So here is my suggestion that I think addresses your up-to-date-ness concerns: let's drop the version bump in Cargo.toml. As I said it's fine to keep the changes to Cargo.lock. We can then think about adding a CI job that specifically tests with the most recent version of libbpf-sys. and/or consider enabling Dependabot to bump versions (in Cargo.lock) as they are made available (not familiar with it, but I believe that's what it's for).

Update the libbpf-sys library to version 1.1.1 in Cargo.lock

Signed-off-by: Michael Mullin <[email protected]>
@mimullin-bbry
Copy link
Contributor Author

Ah, I understand. When I library sets a dependency in Cargo.toml it's explicitly saying "this library requires this minimum version", which puts a restriction upon users of the library if they need to use an older version. Thanks for broadening my understanding of the ecosystem :)

Change updated.

Tangential question (related to the num_enum issues). Is there a way for a new project using libbpf-rs to explicitly state they want libbpf-sys 1.0.3? I was pulling out my hair yesterday trying to figure out how I could get my project, which uses libbpf-rs, which uses num_enum which uses num_enum_derive to pin to an exact version of num_enum and num_enum_derive.

@danielocfb
Copy link
Collaborator

danielocfb commented Jan 11, 2023

Change updated.

Awesome, thank you!

Tangential question (related to the num_enum issues). Is there a way for a new project using libbpf-rs to explicitly state they want libbpf-sys 1.0.3? I was pulling out my hair yesterday trying to figure out how I could get my project, which uses libbpf-rs, which uses num_enum which uses num_enum_derive to pin to an exact version of num_enum and num_enum_derive.

I believe there are two ways to go about that, with different trade-offs. First, they can just pin that version in Cargo.lock. So in your case I'd think that cargo update --package libbpf-sys --precise 1.0.3 will do the trick. This will pin the version for the time being (basically, until you explicitly update the package again or remove Cargo.lock). But if you don't check in Cargo.lock (which can be a reasonable thing to do for other reasons), that won't be forcing other users to the same constraint. (whether that's right or wrong depends on why you wanted to pin the version to begin with)

The other option is to pin a specific version (or range) in Cargo.toml. cargo add 'libbpf-sys@=1.0.3' should do that for the example at hand. Note that this will make libbpf-sys a direct dependency now, even if it was only a transitive one (e.g., through libbpf-rs) beforehand. That's expected, though, because you impose additional constraints on top of what consumption through libbpf-rs would dictate. Also note that pinning a version like this is commonly considered bad practice, because it will exclude you from receiving bug fixes, for example.

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.

2 participants