Fixing break on OOB System.IO.Ports package where native dependency was no longer produced

[joperezr]

When we merged the PR that added a lib prefix to our native libraries we didn't take into account that System.IO.Ports today is OOB and has a native shim that also ships oob. That change broke the package since now the package isn't being emitted with a package dependency to package runtime.native.SYstem.IO.Ports. This change fixes that.

[ghost]

Tagging subscribers to this area: @ViktorHofer
Notify danmosemsft if you want to be subscribed.

[joperezr]

cc: @ericstj

FYI: @tarekgh @krwq

[krwq]

@Anipik can we still take this for preview4? Without this the package is unusable (note this is an OOB package)

[danmoseley]

Broke by #33236
This break was in Preview 3, but not Preview 2.

[danmoseley]

Is there not even a loop back style test that we could imagine that would catch that the binary isn't getting loaded?

[tarekgh]

Is there not even a loop back style test that we could imagine that would catch that the binary isn't getting loaded?

I was going to ask this question. I am also wondering do we have the list of the native binaries we ship somewhere? this will be a good reference in the future.

[joperezr]

I was talking with @krwq about this. We do have package tests today that do a verification of runtime closure, but it looks like we only do that for managed dependencies which is why we didn’t catch this missing native dependency.

We could probably extend that to also check for native dependencies cc @ericstj

[joperezr]

To amswer Tarek’s question, this is only a problem for System.IO.Ports today as it is the only OOB package with a dependency on a native shim. All of the rest native libraries we built are part of the shared framework so they would never hit this problem

[krwq]

@danmosemsft unfortunatelly all regular tests which would hit native asset code paths require opening the file related to serial port first. We do have loopback tests (not sure if you mean the same loopback) which require TX/RX pins to be connected to each other but that requires physical setup which we currently don't have for runtime.

[krwq]

I'm wondering perhaps if I opened some random non-serial port file I would get different exception - that could be enough test case so this doesn't happen again

[danmoseley]

By loopback I didn't mean literal loopback, just something that works without a serial port.

Even if we have a test that attempts to dlopen/LoadLibrary the file it would be something.

[joperezr]

If you just want to test that this particular problem with this package won’t happen again it would be very easy to add a packaging test specifically to make sure this dependency was emitted

[danmoseley]

@joperezr that is probably a better idea. Our unit tests don't test package authoring, right?

[joperezr]

Edit: assuming that by unit tests you mean all tests we run in CI that include package tests we run in allconfigurations

They do. We make sure that the package has a full closure for all tfms and rids it supports, so that will catch most authoring issues. The problem here is that the missing lib in the closure is a native lib and I believe VerifyClosure task doesn’t verify missing native libs, which is why we didn’t catch this. We could look into adding this validation for native dependencies as well

[joperezr]

@krwq any type of loopback test or trying to load a library wouldn’t catch this, because we run our unit tests against the testhost we build which contains libraries outside the shared framework, including both System.IO.Ports.dll and libSystem.IO.Ports.Native.so. The right place to test this would be in our package Tests

[maloo]

@danmosemsft unfortunatelly all regular tests which would hit native asset code paths require opening the file related to serial port first. We do have loopback tests (not sure if you mean the same loopback) which require TX/RX pins to be connected to each other but that requires physical setup which we currently don't have for runtime.

There are virtual loopback drivers, that loop from one virtual serial port to another. But maybe installing a driver is just as hard as accessing external HW from CI.

[krwq]

@joperezr can you create an issue on the native assets tests?

[krwq]

Seems like this will fix the 5.0 package but scenario where 5.0 OOB package is added to 3.1 project might still be broken: currently 3.1 shipped with libraries without "lib" prefixes i.e. System.Native.so and in 5.0 it would be libSystem.Native.so. The interop in OOBs shipping with 5.0 have "lib" prefix hardcoded (for DllImports) for perf reasons which makes them not being able to find any native assets when using 3.1 sdk... I think in order to preserve perf optimization but also get this to work correctly we should #if library names so that OOBs don't hardcode "lib" prefixes and go slighthly slower path and rest of the code doesn't have penalty. Prefixes I'm talking about are mostly here (perhaps also in other files): https://github.com/dotnet/runtime/blob/master/src/libraries/Common/src/Interop/Unix/Interop.Libraries.cs#L10

[joperezr]

The scenario you mention should be fixed as well because 5.0 oob package will carry a pavkage dependency to native 5.0 package which does have the lib prefix and this will be restored for 3.1 as well so you shouldn’t require ifdefs here.

[joperezr]

Oh but I see your point, in the case of System.Native calls yes, you are correct and this would break. I thought S.IO.Ports only depended on libSystem.Io.ports.native.so

[ericstj]

It's broken that this library PInvokes to System.Native. We don't consider the shims to have any public surface area. See #33130

[joperezr]

Pending builds seem to be cancelled due to some infrastructure issue. Merging..

[Anipik]

Pending builds seem to be cancelled due to some infrastructure issue. Merging..

did we decide to port this to preview4 ?

[krwq]

@Anipik AFAIK this hasn't been approved for preview4 port. Also we need to fix #33130 to claim we fixed all the blocking issues for System.IO.Ports so we'll target preview5 anyway

[Anipik]

We are taking up a new change in runtime for preview4, so we have time if want to get something in preview4

[krwq]

if that's fine then IMO we should take this change. This change fixes this package when targeting 5.0, the other issue will fix it when targeting 3.1

[Anipik]

Okay, you can create a pr against preview4 and mark it as "servicing-consider".

[krwq]

I thought you're taking stuff from master, this already got rejected for preview 4