Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix TargetOS value linux instead of Unix #80794

Merged

Conversation

am11
Copy link
Member

@am11 am11 commented Jan 18, 2023

Regression from 4ece8f0 (see src/coreclr/nativeaot/BuildIntegration/Microsoft.NETCore.Native.targets).

Daily build of SDK (8.0.100-alpha.1.23068.3) is failing with:

$  dotnet publish -o dist -p:PublishAot=true
...
  foo -> /foo/bin/Debug/net8.0/linux-arm64/foo.dll
  Generating native code
  Unhandled exception. System.CommandLine.CommandLineException: Target OS 'Unix' is not supported
     at System.CommandLine.Helpers.GetTargetOS(String) in /_/src/coreclr/tools/Common/CommandLineHelpers.cs:line 82
     at System.CommandLine.Argument`1.<>c__DisplayClass5_0.<.ctor>b__1(ArgumentResult, Object& )
     at System.CommandLine.Parsing.ArgumentResult.Convert(Argument)
     at System.CommandLine.Parsing.ArgumentResult.GetArgumentConversionResult()
     at System.CommandLine.Parsing.ParseResultVisitor.ValidateAndConvertArgumentResult(ArgumentResult)
     at System.CommandLine.Parsing.ParseResultVisitor.ValidateAndConvertOptionResult(OptionResult)
     at System.CommandLine.Parsing.ParseResultVisitor.Stop()
     at System.CommandLine.Parsing.Parser.Parse(IReadOnlyList`1, String )
     at ILCompiler.Program.Main(String[]) in /_/src/coreclr/tools/aot/ILCompiler/Program.cs:line 672
  Aborted
/root/.nuget/packages/microsoft.dotnet.ilcompiler/8.0.0-alpha.1.23067.2/build/Microsoft.NETCore.Native.targets(281,5): error MSB3073: The command ""/root/.nuget/packages/runtime.linux-arm64.microsoft.dotnet.ilcompiler/8.0.0-alpha.1.23067.2/tools/ilc" @"obj/Debug/net8.0/linux-arm64/native/foo.ilc.rsp"" exited with code 134. [/foo/foo.csproj]

copied this block: https://github.com/am11/runtime/blob/feature/nativeaot/build-integration2/src/coreclr/nativeaot/BuildIntegration/Microsoft.DotNet.ILCompiler.SingleEntry.targets#L5-L8 plus a line from @Thefrank's PR.

@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Jan 18, 2023
@ghost
Copy link

ghost commented Jan 18, 2023

Tagging subscribers to this area: @agocke, @MichalStrehovsky, @jkotas
See info in area-owners.md if you want to be subscribed.

Issue Details

Regression from 4ece8f0 (see src/coreclr/nativeaot/BuildIntegration/Microsoft.NETCore.Native.targets).

Daily build of SDK (8.0.100-alpha.1.23068.3) is failing with:

$  dotnet publish -o dist -p:PublishAot=true
...
  foo -> /foo/bin/Debug/net8.0/linux-arm64/foo.dll
  Generating native code
  Unhandled exception. System.CommandLine.CommandLineException: Target OS 'Unix' is not supported
     at System.CommandLine.Helpers.GetTargetOS(String) in /_/src/coreclr/tools/Common/CommandLineHelpers.cs:line 82
     at System.CommandLine.Argument`1.<>c__DisplayClass5_0.<.ctor>b__1(ArgumentResult, Object& )
     at System.CommandLine.Parsing.ArgumentResult.Convert(Argument)
     at System.CommandLine.Parsing.ArgumentResult.GetArgumentConversionResult()
     at System.CommandLine.Parsing.ParseResultVisitor.ValidateAndConvertArgumentResult(ArgumentResult)
     at System.CommandLine.Parsing.ParseResultVisitor.ValidateAndConvertOptionResult(OptionResult)
     at System.CommandLine.Parsing.ParseResultVisitor.Stop()
     at System.CommandLine.Parsing.Parser.Parse(IReadOnlyList`1, String )
     at ILCompiler.Program.Main(String[]) in /_/src/coreclr/tools/aot/ILCompiler/Program.cs:line 672
  Aborted
/root/.nuget/packages/microsoft.dotnet.ilcompiler/8.0.0-alpha.1.23067.2/build/Microsoft.NETCore.Native.targets(281,5): error MSB3073: The command ""/root/.nuget/packages/runtime.linux-arm64.microsoft.dotnet.ilcompiler/8.0.0-alpha.1.23067.2/tools/ilc" @"obj/Debug/net8.0/linux-arm64/native/foo.ilc.rsp"" exited with code 134. [/foo/foo.csproj]

copied this block: https://github.com/am11/runtime/blob/feature/nativeaot/build-integration2/src/coreclr/nativeaot/BuildIntegration/Microsoft.DotNet.ILCompiler.SingleEntry.targets#L5-L8 plus a line from @Thefrank's PR.

Author: am11
Assignees: -
Labels:

area-NativeAOT-coreclr

Milestone: -

@am11
Copy link
Member Author

am11 commented Jan 18, 2023

@jkotas this is an alt. fix for #80791.

@ViktorHofer
Copy link
Member

Why do we need to pass a target OS value to ILC at all? Before 4ece8f0, we didn't do that as ILC already has OS runtime detection code.

@MichalStrehovsky
Copy link
Member

Why do we need to pass a target OS value to ILC at all? Before 4ece8f0, we didn't do that as ILC already has OS runtime detection code.

FreeBSD is a first instance of where we need to do cross-OS build. In our infrastructure, the build machine is Linux. The target is FreeBSD. Before FreeBSD the most advanced thing we needed was a cross-architecture build (from x64 to arm64, but same OS).

@MichalStrehovsky
Copy link
Member

Since the revert merged, could you add a revert of the revert (or just the extra IlcArg to specify target os)?

@MichalStrehovsky
Copy link
Member

Let's also wait for dotnet/sdk#29800 to merge so that if there's something wrong, we don't break it again. I'll mark this no merge for now.

@MichalStrehovsky MichalStrehovsky added NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) blocked Issue/PR is blocked on something - see comments labels Jan 19, 2023
@am11
Copy link
Member Author

am11 commented Jan 19, 2023

could you add a revert of the revert

This PR is targeting the main branch. We do not support NativeAOT FreeBSD in current main branch. First PR doing the actual work is #80323.

FreeBSD is a first instance of where we need to do cross-OS build.

It's not supported even with that PR, cross compilation is disabled: https://github.com/dotnet/runtime/pull/80323/files#diff-115c40d5f6cdfb8ebea1490c13581e76b60c3430cbf13f45c270fc5ec383daffR22

@MichalStrehovsky
Copy link
Member

could you add a revert of the revert

This PR is targeting the main branch. We do not support NativeAOT FreeBSD in current main branch. First PR doing the actual work is #80323.

I meant that #80791 removed passing TargetOS to ILC. So with this PR we would fill out the property, but still not actually use it.

@am11
Copy link
Member Author

am11 commented Jan 19, 2023

I mean that revert is best suited for #80323, not this PR. (i.e. we shouldn't have taken that one-off NativeAOT related change in #71486 in first place)

@MichalStrehovsky
Copy link
Member

I mean that revert is best suited for #80323, not this PR. (i.e. we shouldn't have taken that one-off NativeAOT related change in #71486 in first place)

Agreed on both counts but I'm also a bit worried about PRs that only update dead code. If we make this actually use the value of target OS we get at least some coverage.

@am11
Copy link
Member Author

am11 commented Jan 19, 2023

Yup, TargetOS is also used in conditions to distinguish between Windows, OSX and everything_else. It is not completely unused. @ViktorHofer was suggesting to prefer lower-casing the OS name #80791 (comment), which this PR is doing (Unix -> linux as a default value; follow up of #80164).

@MichalStrehovsky
Copy link
Member

Makes sense. Let's wait for the SDK update to merge and we can merge this too.

@Thefrank
Copy link
Contributor

As both my pending PR and me look responsible for this one...how do I prevent BreakEverything() from happening in the future?

In all cases I came across the os logic looks like windows -> osx -> linux -> I_have_no_idea_what_this_is_so_nonportable_linux (or windows -> linux -> osx -> ?)
I tried to make sure to leave the defaults in place and only insert freebsd things where needed, but it appears I overlooked something, or worse, poked something that did not need poking.

@am11
Copy link
Member Author

am11 commented Jan 19, 2023

how do I prevent BreakEverything() from happening

@Thefrank, at least this issue was avoidable if #71486 hadn't touched anything related to "NativeAOT on FreeBSD" and left it for #80323.

Generally, yes it is sometimes a pain as some of these issues are revealed during the integration or code flow. The runtime CI does some testing, but since there are growing number of configurations in NativeAOT, it is not possible to test all combinations in the runtime repo.

@jkotas
Copy link
Member

jkotas commented Jan 19, 2023

how do I prevent BreakEverything() from happening

We have tiered validation system. Some issues are only going to get caught in secondary tiers.

@sec
Copy link
Contributor

sec commented Jan 19, 2023

@Thefrank, at least this issue was avoidable if #71486 hadn't touched anything related to "NativeAOT on FreeBSD" and left it for #80323.

Yeah that's on me - I was thinking those were needed for crossgen to work as it was failing/generating Linux binaries instead of FreeBSD ones, hence the TargetOS changes :( Sorry for that

@am11 am11 removed NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) blocked Issue/PR is blocked on something - see comments labels Jan 19, 2023
@am11
Copy link
Member Author

am11 commented Jan 19, 2023

Let's also wait for dotnet/sdk#29800 to merge

It's merged. Removed the labels.

@MichalStrehovsky MichalStrehovsky merged commit 93c2747 into dotnet:main Jan 20, 2023
mdh1418 pushed a commit to mdh1418/runtime that referenced this pull request Jan 24, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Feb 19, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-NativeAOT-coreclr community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants