Conversation
There was a problem hiding this comment.
Pull request overview
This pull request modifies the native networking code to prefer AF_LINK over AF_PACKET on SunOS/illumos platforms. SunOS defines both AF_LINK and AF_PACKET constants, but using AF_PACKET requires access to system-private headers, which creates unnecessary build dependencies. The change ensures that the codebase uses AF_LINK (the preferred API) on SunOS by adding !defined(TARGET_SUNOS) conditions to the existing AF_PACKET preprocessor checks.
Changes:
- Modified conditional compilation in
pal_maphardwaretype.cto exclude SunOS from using AF_PACKET code paths - Modified conditional compilation in
pal_interfaceaddresses.cto exclude SunOS from AF_PACKET header includes - Added explanatory comments documenting why SunOS should use AF_LINK instead of AF_PACKET
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/native/libs/System.Native/pal_maphardwaretype.c | Updated compile-time conditionals at lines 13 and 35 to exclude TARGET_SUNOS from AF_PACKET code paths, forcing the use of AF_LINK for hardware type mapping on SunOS |
| src/native/libs/System.Native/pal_interfaceaddresses.c | Updated compile-time conditional at line 52 to exclude TARGET_SUNOS from AF_PACKET header includes, ensuring AF_LINK headers are used instead |
It was discussed with illumos channel when I set it up. It is build-only quirk and shouldn't block the real work which is completion of port. Let’s look for a proper replacement that doesn’t come at the cost of a reduced or compromised implementation. |
| { | ||
| #if defined(AF_PACKET) | ||
| // Want to use AF_LINK on SunOS | ||
| #if defined(AF_PACKET) && !defined(TARGET_SUNOS) |
There was a problem hiding this comment.
I'm not sure why I would do that when the #elif defined(AF_LINK) section that follows does what we want on SunOS. Did you see that?
I don't see any reduced functionality here. This is just making it use the AF_LINK code paths. |
Have you validated it in a fresh docker? If so, I don't see how it would be behaving properly given the order at call-site: runtime/src/native/libs/System.Native/pal_interfaceaddresses.c Lines 228 to 254 in a48fa8e |
Thanks. I've only gone as far as building this. |
illumos support in build-rootfs.sh was originally added for the dotnet prereq Docker image. Some time ago the illumos image build itself was removed due to staleness(although previously built images still exist). So changes in build-rootfs will need to be validated manually: # fresh docker
$ cd runtime
$ git branch --show-current # make sure it's this branch
# remove the extra headers download block from build-rootfs.sh
# run docker interactively
$ docker run --rm -v$(pwd):/runtime -w /runtime -e ROOTFS_DIR=/crossrootfs/x64 -it ubuntu
$ eng/common/native/install-dependencies.sh
$ apt install -y libmpc-dev
$ eng/common/cross/build-rootfs.sh noble illumos x64
$ ./build.sh clr+libs --cross --os illumos --arch x64 --gcc
I was actually referring to validation of build test and also fixing the logic, I think it needs |
|
OK. This seems to work as well as the previous did. System.Net.NetworkInformation.Functional.Tests Results |
|
Can anyone help me interpret the CI build analysis? |
You can skip those with the following attribute: [SkipOnPlatform(TestPlatforms.illumos | TestPlatforms.Solaris, "Not supported on SunOS-based platforms.")]on top of those methods. But double check if PlatformNotSupported is intended there or something is off (in which case add a tracking issue and put that issue URL in "Not supported on SunOS-based platforms." reason string to imply it needs to be fixed as opposed to it's a known limitation; won't-fix). |
Thanks. However, I don't yet see where I should be looking to add that. |
In the test logs you can find the names of the 29 failing tests, and then add the attribute to each of those methods. |
|
This test appears to be failing due to ENOSPACE I don't see any failures related to the changes in this PR. |
Yes, the support for getting interface information needs work. |
|
Sounds good. Did you get a fresh build succeeding without private headers download block in build-rootfs.sh: #124728 (comment)? |
Yes, but it will be easier to do the arcade update after this PR goes in, to avoid a "can't build latest in the repo" situation. |
There is no CI leg blocked on illumos. The |
|
There turned out to be a bunch more to do before |
|
I guess I'll reopen this since #125054 is blocked, and I need (at least) these PF_LINK changes to allow building with a less ancient crossrootfs. |
|
I've been working on updates to arcate/eng/common/cross/build-rootfs.sh
and discovered it doing some ill-advised header copying for illumos.
With this small configuration change, that's no longer needed.
As the comment in the change explains:
SunOS defines both AF_LINK and AF_PACKET but AF_LINK is preferred.
Using AF_PACKET on SunOS requires access to system-private headers.