-
Notifications
You must be signed in to change notification settings - Fork 4.9k
initial support for serial port on FreeBSD #42519
Conversation
@@ -2,12 +2,12 @@ | |||
<PropertyGroup> | |||
<AllowUnsafeBlocks>true</AllowUnsafeBlocks> | |||
<IsPartialFacadeAssembly Condition="'$(TargetsNetFx)' == 'true'">true</IsPartialFacadeAssembly> | |||
<GeneratePlatformNotSupportedAssemblyMessage Condition="'$(TargetsNetStandard)' == 'true' and '$(TargetsWindows)' != 'true' and '$(TargetsLinux)' != 'true' and '$(TargetsOSX)' != 'true'">SR.PlatformNotSupported_IOPorts</GeneratePlatformNotSupportedAssemblyMessage> | |||
<GeneratePlatformNotSupportedAssemblyMessage Condition="'$(TargetsNetStandard)' == 'true' and '$(TargetsWindows)' != 'true' and '$(TargetsLinux)' != 'true' and '$(TargetsOSX)' != 'true' and '$(TargetsFreeBSD)' != 'true'">SR.PlatformNotSupported_IOPorts</GeneratePlatformNotSupportedAssemblyMessage> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What configuration does this still apply to?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the UnknownUnix was originally there as stub for new platforms. Web assembly may fall into this bucket now. cc @safern
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not necessary for this PR, but this condition is getting a bit unwieldy... send like it could be better to start listing what should have this done rather than what shouldn't, or find some other way to simplify it.
cc: @krwq
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently this project (with this change) has the following configurations:
corefx/src/System.IO.Ports/src/Configurations.props
Lines 3 to 14 in 6e190fe
<PackageConfigurations> | |
netstandard2.0-Windows_NT; | |
netstandard2.0-Linux; | |
netstandard2.0-OSX; | |
netstandard2.0-FreeBSD; | |
netstandard2.0; | |
net461-Windows_NT; | |
</PackageConfigurations> | |
<BuildConfigurations> | |
$(PackageConfigurations) | |
netfx-Windows_NT; | |
</BuildConfigurations> |
If we look at the os groups definitions:
corefx/eng/configurations/osgroups.props
Lines 13 to 43 in 6e190fe
<OSGroups Include="Linux"> | |
<Imports>Unix</Imports> | |
<TargetsUnix>true</TargetsUnix> | |
<TargetsLinux>true</TargetsLinux> | |
<PackageTargetRuntime>linux</PackageTargetRuntime> | |
</OSGroups> | |
<OSGroups Include="OSX"> | |
<Imports>Unix</Imports> | |
<TargetsUnix>true</TargetsUnix> | |
<TargetsOSX>true</TargetsOSX> | |
<PackageTargetRuntime>osx</PackageTargetRuntime> | |
</OSGroups> | |
<OSGroups Include="FreeBSD"> | |
<Imports>Unix</Imports> | |
<TargetsUnix>true</TargetsUnix> | |
<TargetsFreeBSD>true</TargetsFreeBSD> | |
<PackageTargetRuntime>freebsd</PackageTargetRuntime> | |
</OSGroups> | |
<OSGroups Include="NetBSD"> | |
<Imports>Unix</Imports> | |
<TargetsUnix>true</TargetsUnix> | |
<TargetsNetBSD>true</TargetsNetBSD> | |
<PackageTargetRuntime>netbsd</PackageTargetRuntime> | |
</OSGroups> | |
<OSGroups Include="WebAssembly"> | |
<!-- TODO: we need to change this to import Unix instead whenever | |
we want to start using managed implementation for WebAssembly --> | |
<Imports>Linux</Imports> | |
<TargetsLinux>true</TargetsLinux> | |
<TargetsWebAssembly>true</TargetsWebAssembly> | |
</OSGroups> |
So basically this condition applies for AnyOS
which is the netstandard2.0
OS agnostic build configuration. We could update this condition to be:
'$(TargetsNetStandard)' == 'true' and '$(OSGroup)' == 'AnyOS'
Web assembly may fall into this bucket now
Not really. The way WebAssembly
is hooked up it will use the netstandard2.0-Linux
configuration (for now) we plan on updating it to fall into the Unix
one.
<DefineConstants>$(DefineConstants);NOSPAN</DefineConstants> | ||
<IncludeDllSafeSearchPathAttribute>true</IncludeDllSafeSearchPathAttribute> | ||
<Configurations>net461-Windows_NT-Debug;net461-Windows_NT-Release;netfx-Windows_NT-Debug;netfx-Windows_NT-Release;netstandard2.0-Debug;netstandard2.0-Linux-Debug;netstandard2.0-Linux-Release;netstandard2.0-OSX-Debug;netstandard2.0-OSX-Release;netstandard2.0-Release;netstandard2.0-Windows_NT-Debug;netstandard2.0-Windows_NT-Release</Configurations> | ||
</PropertyGroup> | ||
<ItemGroup Condition="'$(TargetsNetFx)' != 'true' and ('$(TargetsWindows)' == 'true' or '$(TargetsLinux)' == 'true' or '$(TargetsOSX)' == 'true')"> | ||
<ItemGroup Condition="'$(TargetsNetFx)' != 'true' and ('$(TargetsWindows)' == 'true' or '$(TargetsLinux)' == 'true' or '$(TargetsOSX)' == 'true' or '$(TargetsFreeBSD)' == 'true')"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same question
@@ -22,7 +22,7 @@ public class PortHelper | |||
|
|||
public static string[] GetPorts() | |||
{ | |||
if (RuntimeInformation.IsOSPlatform(OSPlatform.Linux) || RuntimeInformation.IsOSPlatform(OSPlatform.OSX)) | |||
if (RuntimeInformation.IsOSPlatform(OSPlatform.Linux) || RuntimeInformation.IsOSPlatform(OSPlatform.OSX) || RuntimeInformation.IsOSPlatform(OSPlatform.FreeBSD)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
!IsWindows?
<DefineConstants>$(DefineConstants);NOSPAN</DefineConstants> | ||
<IncludeDllSafeSearchPathAttribute>true</IncludeDllSafeSearchPathAttribute> | ||
<Configurations>net461-Windows_NT-Debug;net461-Windows_NT-Release;netfx-Windows_NT-Debug;netfx-Windows_NT-Release;netstandard2.0-Debug;netstandard2.0-Linux-Debug;netstandard2.0-Linux-Release;netstandard2.0-OSX-Debug;netstandard2.0-OSX-Release;netstandard2.0-Release;netstandard2.0-Windows_NT-Debug;netstandard2.0-Windows_NT-Release</Configurations> | ||
</PropertyGroup> | ||
<ItemGroup Condition="'$(TargetsNetFx)' != 'true' and ('$(TargetsWindows)' == 'true' or '$(TargetsLinux)' == 'true' or '$(TargetsOSX)' == 'true')"> | ||
<ItemGroup Condition="'$(TargetsNetStandard)' == 'true'"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this condition is missing and '$(OSGroup)' != 'AnyOS'
, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was looking at it and It did not seems like that was needed.
I guess I missed something but it seems like we want this every-time except netfx.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
netstandard
will generate a platform not supported assembly when OSGroup == AnyOS
so that's the netstandard2.0
rid-less build configuration in configurations.props.
That's why the all configurations build is failing, because members will be defined twice, by the auto generated code to throw platform not supported and by these compile items.
{ | ||
List<string> ports = new List<string>(); | ||
|
||
foreach (string name in Directory.GetFiles("/dev", "ttyd*")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would it make sense to merge this with Unix logic?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM code wise (or nothing blocking considering repo merge)
Failure in System.Net.Security is unrelated (#37899) |
It seems this broke the build in coreclr. The System.IO.Ports package depends on the |
I'm not sure why that got missed. an you revert the build change @ViktorHofer? |
I'm not reverting it right now as we don't publish packages in the new repo for some time which means I can simply downgrade the used package version of corefx's transport package. |
If there's a straightforward fix, you can submit it on Monday when we open the repo internally. |
In order for the |
So far this was always independent and it would need FreeBSD packages only when built on FreeBSD. I'm not quite sure why this is different. I'm skeptical we will have FreeBSD leg any time soon. |
I did some testing while back when I was working on Serial port but that never made it to product. FreeBSD has standard Unix termios and everything should just work.
For port names, I used guidance from https://www.freebsd.org/doc/en_US.ISO8859-1/books/faq/serial.html
minimal set of tests will pass