Add support for illumos and the Sidecar platform#3
Conversation
zeeshanlakhani
left a comment
There was a problem hiding this comment.
Mostly, a few super minor things here, as things look to be building appropriately.
| @@ -0,0 +1,72 @@ | |||
| void *stdout; | |||
There was a problem hiding this comment.
Should we have a file-header comment about this shim file and what they are shimming/attached-to?
I know you mentioned this in the PR.
One item in particular that may need further explanation is oxide/wrap_libport_mgr_hw. The open source SDE comes with a pre-built library that was linked against a specific version of the linux glibc. This wrapper splits that library back into its original .o files, provides a shim that implements the needed glibc functions, and relinks the library for helios.
There was a problem hiding this comment.
Yeah, that's a good point.
I also changed how stdout is being defined. As far as I can tell from looking at the closed source code, the wrapped library would never try to use stdout from tofino2 code, so the fact that this was an uninitialized symbol shouldn't matter. Still, it seems like good hygiene to actually initialize it, even if it's a bit awkward.
There was a problem hiding this comment.
Do we need this file and/or do we want changelogs for helios?
There was a problem hiding this comment.
It's only needed to keep the debian package builder happy. The builder doesn't care what, if anything, is in it - it just needs to see the file.
For the purposes of review, it would be best to focus on the files under the
oxide/directory. Most of the changes to the rest of the files are ported directly (well, as directly as possible) from changes we made to the closed source SDE, and thus have already been reviewed. I was port each prior changeset individually, but that quickly proved to be untenable.One item in particular that may need further explanation is
oxide/wrap_libport_mgr_hw. The open source SDE comes with a pre-built library that was linked against a specific version of the linux glibc. This wrapper splits that library back into its original.ofiles, provides a shim that implements the neededglibcfunctions, and relinks the library for helios.The source for that library is available in the closed-source repo, so we are technically able to build our own helios-compatible binary block directly, but I'm not sure our license would allow us to put the resulting blob into this repo. That's worth looking into in the future and is tracked as issue #1.