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

freebsd: move net/if_mib.h contents to submodule #3367

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

ydirson
Copy link

@ydirson ydirson commented Sep 27, 2023

There is a conflict of NETLINK_GENERIC definitions between net/if_mib.h and netlink/netlink.h. netlink.h is already exported in the crate root for Linux (and those definitions are already used by at least crates neli and netlink-packet-route), and if_mib is not much used yet, so this moves if_mib contents into its own namespace to leave place for netlink support on FreeBSD (#3201).

Looks like it is the first time namespacing is used in the libc crate, so I had to guess a few things, being quite new to this project.

The moved symbols did not appear in semver, so I just added the new ones. sysinfo from @GuillaumeGomez seems to be the only impacted public crate.

@rustbot
Copy link
Collaborator

rustbot commented Sep 27, 2023

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @JohnTitor (or someone else) soon.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

  • @rustbot author: the review is finished, PR author should check the comments and take action accordingly
  • @rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

@GuillaumeGomez
Copy link
Member

Even if only sysinfo is impacted, it remains a breaking change (but thanks for the heads-up!).

@ydirson
Copy link
Author

ydirson commented Sep 27, 2023

New revision completely drops the semver part -- will gladly use any hint to get it working and add it back.

@GuillaumeGomez
Copy link
Member

To avoid the semver issue, the only way I see is to reexport items from the submodule by default.

@ydirson
Copy link
Author

ydirson commented Sep 27, 2023

To avoid the semver issue, the only way I see is to reexport items from the submodule by default.

That would avoid the breaking change, but that would not work for NETLINK_GENERIC, which this PR is all about.

@GuillaumeGomez
Copy link
Member

Indeed. Let's see if libc's maintainers have another way to do it. But otherwise, I'm very fine with the breaking change. I'll just update libc in sysinfo and everything should be fine.

@ydirson
Copy link
Author

ydirson commented Sep 27, 2023

I'm baffled by the tests still trying to look at ifmib::* after I removed them from semver...

@ydirson
Copy link
Author

ydirson commented Sep 28, 2023

I'm baffled by the tests still trying to look at ifmib::* after I removed them from semver...

OK I realize they come from the generated tests. This part is essentially dedicated to ctest, and I don't see a way there to tell a header's contents live in a submodule.
Looks like for now we'll have to flag those definitions not to be checked.

@ydirson ydirson force-pushed the freebsd-ifmib branch 3 times, most recently from e999399 to fe5a9ff Compare September 28, 2023 10:37
@JohnTitor
Copy link
Member

Note also that private code might use these items. The libs-maintainers team has been considering making a new major release (0.3? 1.0? I'm not sure though) and I think it'd be better to include this change there. Let's wait for that release.

@ydirson
Copy link
Author

ydirson commented Sep 28, 2023

Note also that private code might use these items. The libs-maintainers team has been considering making a new major release (0.3? 1.0? I'm not sure though) and I think it'd be better to include this change there. Let's wait for that release.

How can we track that here, when the symbols moved were not included in semver to start with? Is there a "list of breaking changes" somewhere to add this PR to for consideration?

@bors
Copy link
Contributor

bors commented Sep 29, 2023

☔ The latest upstream changes (presumably #3341) made this pull request unmergeable. Please resolve the merge conflicts.

@JohnTitor
Copy link
Member

See #3248, I've added this to the task list.

@ydirson
Copy link
Author

ydirson commented Dec 18, 2023

Rebased onto 0.2.151

@asomers
Copy link
Contributor

asomers commented Dec 18, 2023

What about adding a feature flag to libc? An off-by-default "ifmib" feature would let crates like @ydirson's use the constants in their correct namespace, not conflicting with the new constant introduced by #3201 , while other crates won't see a change. And libc could remove the feature flag when it does next make a major release.

@ydirson
Copy link
Author

ydirson commented Dec 18, 2023

As I understand it, such a flag would enable either ifmib of netlink, so for 0.2 it could make sense to let ifmib be the default, and rather add a netlink feature flag to disable it and enable netlink support?

@asomers
Copy link
Contributor

asomers commented Dec 18, 2023

Whatever it's named, the default option should be the status quo, and the alternative would be to put ifmib into a submodule. BTW, I've seen other crates that call this "unstable".

@ydirson
Copy link
Author

ydirson commented Dec 22, 2023

I've seen other crates that call this "unstable".

That would seem a bit too generic here, we could just name it netlink.
However I see some moves on the 0.3 front, so should I proceed with this suggestion for 0.2?

@ydirson
Copy link
Author

ydirson commented Jan 23, 2024

rebased

@bors
Copy link
Contributor

bors commented Feb 17, 2024

☔ The latest upstream changes (presumably #3587) made this pull request unmergeable. Please resolve the merge conflicts.

There is a conflict of NETLINK_GENERIC definitions between
net/if_mib.h and netlink/netlink.h.  netlink.h is already exported in
the crate root for Linux (and those definitions are already used by at
least crates neli and netlink-packet-route), and if_mib is not much
used yet, so this moves if_mib contents into its own namespace to
leave place for netlink support on FreeBSD (rust-lang#3194).

Module definition moved to the end of file to avoid cryptic style.rs
error "constant found after module when it belongs before".

ctest as of 0.22 cannot be told a given header's symbols live in a
submodule, so let the tests ignore all of them.

Signed-off-by: Yann Dirson <yann.dirson@vates.fr>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants