freebsd: move net/if_mib.h contents to submodule

[ydirson]

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]

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]

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

[ydirson]

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

[GuillaumeGomez]

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

[ydirson]

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]

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]

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

[ydirson]

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.

[JohnTitor]

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]

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]

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

[JohnTitor]

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

[ydirson]

Rebased onto 0.2.151

[asomers]

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]

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]

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]

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]

rebased

[bors]

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

[tgross35]

Could we just add a single NETLINK_GENERIC_IF_MIB? Or are more conflicts expected?

[ydirson]

Could we just add a single NETLINK_GENERIC_IF_MIB? Or are more conflicts expected?

All software using the if_mib.h version will miscompile and need to make a change to get the NETLINK_GENERIC they need. My gut feeling is that anyone using that API is likely to use other consts (but I may be wrong), and the namespacing would prevent miscompilation if someone just casually tries to bump libc. The change would be more fullproof if we forced attention by moving the ifmibdata type to the namespace as well.

But I would not object to a simpler change without namespacing if other parties more in contact with that API feel it's OK. @GuillaumeGomez do you think about any FreeBSD devs who would have informed opinion?

Note I just stumbled by chance (digging into sysinfo to check how that API is used there) against recently-merged #4022, adding the same if_mib,h API for apple, whose code would now need to be changed at the same time.

[GuillaumeGomez]

I don't know of any FreeBSD devs so I have no idea.

[tgross35]

If the intent is that both netlink and if_mib are available and the user has to select one, then it makes sense to move either everything from if_mib to a separate module, possibly everything from netlink to its own module as well. If instead it is only a couple of variables that conflict then I don't think we need to move everything.

@asomers what is planned here? Based on your above comment

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.

this makes it sound a bit like if_mib is intended to replace netlink at some point.

[asomers]

If the intent is that both netlink and if_mib are available and the user has to select one, then it makes sense to move either everything from if_mib to a separate module, possibly everything from netlink to its own module as well. If instead it is only a couple of variables that conflict then I don't think we need to move everything.

@asomers what is planned here? Based on your above comment

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.

this makes it sound a bit like if_mib is intended to replace netlink at some point.

No. If anything, more stuff is moving toward netlink. But I definitely expect both to be used for a long time. I don't see any solution that doesn't involve moving one thing or the other into a submodule. Perhaps both.

[rustbot]

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

[tgross35]

Wrote down my thoughts for how I would like to proceed: #3201 (comment)

@rustbot author

[rustbot]

Reminder, once the PR becomes ready for a review, use @rustbot ready.