Skip to content

Conversation

@ViktorHofer
Copy link
Owner

Related dotnet#69980 (comment).

To avoid hardcoding these platforms, we would need to add an msbuild task into the TargetFramework.Sdk package that accepts the OSGroups.json file, the TargetFramework, TargetFrameworks and TargetPlatformIdentifier as an input.

@ViktorHofer
Copy link
Owner Author

@ericstj @buyaa-n @jeffhandley please take a look

@ericstj
Copy link

ericstj commented Jun 23, 2022

Is the problem that TargetPlatformIdentifier isn't set at this point of the evaluation? If so - could we move these to a target?

'$(TargetPlatformIdentifier)' != 'windows'" Include="Unix" />
<SupportedPlatform Remove="@(SupportedPlatform)" />
<SupportedPlatform Include="Windows"
Condition="'$(TargetPlatformIdentifier)' == 'windows' or
Copy link

@buyaa-n buyaa-n Jun 24, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NIT: adding Windows into SupportedPlatforms has no effect when targeting windows, though I see we did the same logic for Browser

<!-- Enables warnings for Android, iOS, tvOS and macCatalyst for all builds -->

<!-- Keep in sync with Microsoft.NET.SupportedPlatforms.props and OSGroups.json. -->
<ItemGroup>
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably should add condition Condition="'$(IsTestProject)' != 'true'" to avoid noise in test projects

!$(TargetFrameworks.Contains('$(TargetFramework)-Unix'))
)" />
<SupportedPlatform Include="Linux"
Condition="'$(TargetPlatformIdentifier)' == 'Linux' or
Copy link

@buyaa-n buyaa-n Jun 24, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here and all similar cases below: is redundant for Linux target (by assuming that [SupportedOSPlatform("Linux")] attribute will be added to the assembly by MSbuild)

Condition="'$(TargetPlatformIdentifier)' == 'illumos' or
'$(TargetPlatformIdentifier)' == 'Unix' or
(
'$(TargetPlatformIdentifier)' == '' and
Copy link

@buyaa-n buyaa-n Jun 24, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I remember correctly we did not wanted to include illumos and Solaris for all platform agnostic targets, though I don't remember what was the exact scenario

@buyaa-n
Copy link

buyaa-n commented Jun 24, 2022

Overall the changes makes sense to me, overwriting the MSBuild default list (Microsoft.NET.SupportedPlatforms.prop) and conditionally populating the <SupportedPlatforms> list.

Note that it is only needed for light up warning of unsupported APIs in cross platform build, for targeted builds as the assembly would have an Supported attribute for the targeted platform it has no effect, but I see we added it anyway, do not remember why we did that 😄.

ViktorHofer pushed a commit that referenced this pull request Jun 15, 2023
ViktorHofer pushed a commit that referenced this pull request Jun 15, 2023
…tnet#87189)

This fixes a startup crash on Big Sur:

> error: * Assertion at /Users/runner/work/1/s/src/mono/mono/utils/mono-hwcap-arm64.c:35, condition `res == 0' not met

Because sysctl can't find some of these options:

    $ sysctl hw.optional.armv8_crc32
    hw.optional.armv8_crc32: 1
    $ sysctl hw.optional.arm.FEAT_RDM
    sysctl: unknown oid 'hw.optional.arm.FEAT_RDM'
    $ sysctl hw.optional.arm.FEAT_DotProd
    sysctl: unknown oid 'hw.optional.arm.FEAT_DotProd'
    $ sysctl hw.optional.arm.FEAT_SHA1
    sysctl: unknown oid 'hw.optional.arm.FEAT_SHA1'
    $ sysctl hw.optional.arm.FEAT_SHA256
    sysctl: unknown oid 'hw.optional.arm.FEAT_SHA256'
    $ sysctl hw.optional.arm.FEAT_AES
    sysctl: unknown oid 'hw.optional.arm.FEAT_AES'

Full stack trace:

* thread #1, queue = 'com.apple.main-thread', stop reason = breakpoint 2.1
  * frame #0: 0x0000010ef37560 libmonosgen-2.0.dylib`monoeg_assertion_message
    frame #1: 0x0000010ef375cc libmonosgen-2.0.dylib`mono_assertion_message + 32
    frame #2: 0x0000010ef40d6c libmonosgen-2.0.dylib`mono_hwcap_arch_init + 544
    frame dotnet#3: 0x0000010ef54bd8 libmonosgen-2.0.dylib`mono_hwcap_init + 72
    frame dotnet#4: 0x0000010ee14dc0 libmonosgen-2.0.dylib`parse_optimizations + 52
    frame dotnet#5: 0x0000010edbed48 libmonosgen-2.0.dylib`mono_init
    frame dotnet#6: 0x0000010ee18968 libmonosgen-2.0.dylib`mono_jit_init_version
    frame dotnet#7: 0x0000010f48a300 libxamarin-dotnet-debug.dylib`xamarin_bridge_initialize + 216
    frame dotnet#8: 0x0000010f4900a4 libxamarin-dotnet-debug.dylib`xamarin_main + 376
@ViktorHofer ViktorHofer closed this Aug 1, 2023
akoeplinger pushed a commit that referenced this pull request Sep 11, 2024
* bug #1: don't allow for values out of the SerializationRecordType enum range

* bug #2: throw SerializationException rather than KeyNotFoundException when the referenced record is missing or it points to a record of different type

* bug dotnet#3: throw SerializationException rather than FormatException when it's being thrown by BinaryReader (or sth else that we use)

* bug dotnet#4: document the fact that IOException can be thrown

* bug dotnet#5: throw SerializationException rather than OverflowException when parsing the decimal fails

* bug dotnet#6: 0 and 17 are illegal values for PrimitiveType enum

* bug dotnet#7: throw SerializationException when a surrogate character is read (so far an ArgumentException was thrown)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants