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.

Sign up for GitHub

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

Jump to bottom

NativeAOT publish fails on fi_FI.utf8 locale #98550

Closed
De-Crypted opened this issue Feb 15, 2024 · 12 comments · Fixed by #98552
Closed

NativeAOT publish fails on fi_FI.utf8 locale #98550

De-Crypted opened this issue Feb 15, 2024 · 12 comments · Fixed by #98552
Labels
area-NativeAOT-coreclr
Milestone
9.0.0

Comments

@De-Crypted
Copy link

De-Crypted commented Feb 15, 2024 •
edited
Loading

Describe the bug

Trying to publish freshly created console application with dotnet publish /p:PublishAot=true produces error:

MSBuild version 17.9.4+90725d08d for .NET
/home/user/.nuget/packages/microsoft.dotnet.ilcompiler/8.0.2/build/Microsoft.DotNet.ILCompiler.SingleEntry.targets(9,16): error MSB4086: A numeric comparison was attempted on "$(_indexOfPeriod)" that evaluates to "−1" instead of a number, in condition "'$(_indexOfPeriod)' > -1". [/home/user/sources/project/daemon/daemon.csproj]

Note that the first -1 actually has "U+2212 : MINUS SIGN" printed in the console.

As a workaround locale can be set to en_US.utf8 and the publish works as expected.

To Reproduce

On Linux machine set locale to fi_FI.utf8
Create console application with

dotnet new console -n app

Publish it with

dotnet publish /p:PublishAot=true

Further technical details

.NET SDK:
 Version:           8.0.200
 Commit:            438cab6a9d
 Workload version:  8.0.200-manifests.cdf2cc8e

Runtime Environment:
 OS Name:     manjaro
 OS Version:  
 OS Platform: Linux
 RID:         linux-x64
 Base Path:   /home/tk/.dotnet/sdk/8.0.200/

.NET workloads installed:
There are no installed workloads to display.

Host:
  Version:      8.0.2
  Architecture: x64
  Commit:       1381d5ebd2

.NET SDKs installed:
  8.0.200 [/home/tk/.dotnet/sdk]

.NET runtimes installed:
  Microsoft.AspNetCore.App 8.0.2 [/home/tk/.dotnet/shared/Microsoft.AspNetCore.App]
  Microsoft.NETCore.App 8.0.2 [/home/tk/.dotnet/shared/Microsoft.NETCore.App]

Other architectures found:
  None

Environment variables:
  DOTNET_ROOT       [/usr/share/dotnet]

global.json file:
  Not found

Learn more:
  https://aka.ms/dotnet/info

Download .NET:
  https://aka.ms/dotnet/download
@dotnet-issue-labeler dotnet-issue-labeler bot added the untriaged New issue has not been triaged by the area owner label Feb 15, 2024
@dotnet-issue-labeler dotnet-issue-labeler
Copy link

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

@akoeplinger akoeplinger transferred this issue from dotnet/sdk Feb 16, 2024
@dotnet-issue-labeler dotnet-issue-labeler bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Feb 16, 2024
@akoeplinger akoeplinger added area-NativeAOT-coreclr and removed needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners untriaged New issue has not been triaged by the area owner labels Feb 16, 2024
@ghost
Copy link

ghost commented Feb 16, 2024

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

Issue Details

Describe the bug

Trying to publish freshly created console application with dotnet publish /p:PublishAot=true produces error:

MSBuild version 17.9.4+90725d08d for .NET
/home/user/.nuget/packages/microsoft.dotnet.ilcompiler/8.0.2/build/Microsoft.DotNet.ILCompiler.SingleEntry.targets(9,16): error MSB4086: A numeric comparison was attempted on "$(_indexOfPeriod)" that evaluates to "−1" instead of a number, in condition "'$(_indexOfPeriod)' > -1". [/home/user/sources/project/daemon/daemon.csproj]

Note that the first -1 actually has "U+2212 : MINUS SIGN" printed in the console.

As a workaround locale can be set to en_US.utf8 and the publish works as expected.

To Reproduce

On Linux machine set locale to fi_FI.utf8
Create console application with

dotnet new console -n app

Publish it with

dotnet publish /p:PublishAot=true

Further technical details

.NET SDK:
 Version:           8.0.200
 Commit:            438cab6a9d
 Workload version:  8.0.200-manifests.cdf2cc8e

Runtime Environment:
 OS Name:     manjaro
 OS Version:  
 OS Platform: Linux
 RID:         linux-x64
 Base Path:   /home/tk/.dotnet/sdk/8.0.200/

.NET workloads installed:
There are no installed workloads to display.

Host:
  Version:      8.0.2
  Architecture: x64
  Commit:       1381d5ebd2

.NET SDKs installed:
  8.0.200 [/home/tk/.dotnet/sdk]

.NET runtimes installed:
  Microsoft.AspNetCore.App 8.0.2 [/home/tk/.dotnet/shared/Microsoft.AspNetCore.App]
  Microsoft.NETCore.App 8.0.2 [/home/tk/.dotnet/shared/Microsoft.NETCore.App]

Other architectures found:
  None

Environment variables:
  DOTNET_ROOT       [/usr/share/dotnet]

global.json file:
  Not found

Learn more:
  https://aka.ms/dotnet/info

Download .NET:
  https://aka.ms/dotnet/download
Author: De-Crypted
Assignees: -
Labels:

untriaged, area-NativeAOT-coreclr
Milestone: -

@akoeplinger akoeplinger added this to the 9.0.0 milestone Feb 16, 2024
@akoeplinger
Copy link
Member

akoeplinger commented Feb 16, 2024 •
edited
Loading

It looks like <_indexOfPeriod>$(_originalTargetOS.IndexOf('.'))</_indexOfPeriod> causes the int->string conversion to happen with current culture:

runtime/src/coreclr/nativeaot/BuildIntegration/Microsoft.DotNet.ILCompiler.SingleEntry.targets

Lines 8 to 9 in 7185df8

<_indexOfPeriod>$(_originalTargetOS.IndexOf('.'))</_indexOfPeriod>
<_originalTargetOS Condition="'$(_indexOfPeriod)' &gt; -1">$(_originalTargetOS.SubString(0, $(_indexOfPeriod)))</_originalTargetOS>

@akoeplinger
Copy link
Member

We can't use .ToString(CultureInfo.InvariantCulture) since CultureInfo is not allowed in msbuild.

A workaround I found is using .ToString('0;-0') to use an explicit minus sign for negative numbers in the format string.
Or we could just duplicate the .IndexOf() call instead of using the msbuild property as an intermediate.

Any preferences?

@tmds
Copy link
Member

tmds commented Feb 16, 2024

.NET SDKs installed:
8.0.200 [/home/tk/.dotnet/sdk]

I wonder if you're seeing this because of dotnet/msbuild#9449.

If you export MSBUILDDISABLEFEATURESFROMVERSION=17.10, does it work?

@akoeplinger
Copy link
Member

@tmds I don't think so, that PR only changes the culture of the Exec task which is not used here.

@MichalStrehovsky
Copy link
Member

We can't use .ToString(CultureInfo.InvariantCulture) since CultureInfo is not allowed in msbuild.

A workaround I found is using .ToString('0;-0') to use an explicit minus sign for negative numbers in the format string. Or we could just duplicate the .IndexOf() call instead of using the msbuild property as an intermediate.

Any preferences?

Maybe duplicating would be better. We can use Contains to make a it bit more readable, although it still looks like LISP.

<_originalTargetOS Condition="$(_originalTargetOS.Contains('.'))">$(_originalTargetOS.SubString(0, $(_originalTargetOS.IndexOf('.'))))</_originalTargetOS>

@akoeplinger
Copy link
Member

Yeah I like Contains better too.

@akoeplinger akoeplinger mentioned this issue Feb 16, 2024
Merged
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Feb 16, 2024
@akoeplinger
Copy link
Member

#98552

akoeplinger added a commit to akoeplinger/runtime that referenced this issue Feb 16, 2024
@akoeplinger
Fix NativeAOT publish failure on fi_FI culture
cc33bea 
This culture uses `U+2212 : MINUS SIGN` instead of `-` for negative numbers which trips up msbuild when comparing the property.
Instead of using an intermediate property just inline the usage and use `Contains()` for better readability.

Fixes dotnet#98550
@tmds
Copy link
Member

tmds commented Feb 16, 2024

I don't think so, that PR only changes the culture of the Exec task which is not used here.

I don't suspect that either, though it's a weird coincidence that this is reported with 8.0.200 which is the first SDK to include that change.

I'm surprised the result of the evaluation depends on the system culture. I had expected msbuild to be using the invariant culture when it evaluates them.

akoeplinger added a commit that referenced this issue Feb 17, 2024
@akoeplinger
Fix NativeAOT publish failure on fi_FI culture (#98552)
c768315 
This culture uses `U+2212 : MINUS SIGN` instead of `-` for negative numbers which trips up msbuild when comparing the property.
Instead of using an intermediate property just inline the usage and use `Contains()` for better readability.

Fixes #98550
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Feb 17, 2024
@akoeplinger
Copy link
Member

akoeplinger commented Feb 17, 2024 •
edited
Loading

If you export MSBUILDDISABLEFEATURESFROMVERSION=17.10, does it work?

I tried that and it didn't fix the issue.

I also tried with an 8.0.100 SDK and it already happens there.

I'm surprised the result of the evaluation depends on the system culture. I had expected msbuild to be using the invariant culture when it evaluates them.

Logged dotnet/msbuild#9757 to get some input from the msbuild team.

akoeplinger added a commit to akoeplinger/runtime that referenced this issue Feb 17, 2024
@akoeplinger
Fix NativeAOT publish failure on fi_FI culture (dotnet#98552)
eb6dab4 
This culture uses `U+2212 : MINUS SIGN` instead of `-` for negative numbers which trips up msbuild when comparing the property.
Instead of using an intermediate property just inline the usage and use `Contains()` for better readability.

Fixes dotnet#98550

(cherry picked from commit c768315)
This was referenced Feb 17, 2024
Merged
Closed
akoeplinger added a commit that referenced this issue Feb 20, 2024
@akoeplinger
Fix NativeAOT publish failure on fi_FI culture (#98552) (#98601)
fe2ea4e 
This culture uses `U+2212 : MINUS SIGN` instead of `-` for negative numbers which trips up msbuild when comparing the property.
Instead of using an intermediate property just inline the usage and use `Contains()` for better readability.

Fixes #98550

(cherry picked from commit c768315)
@danmoseley
Copy link
Member

@rainersigwald

It looks like <_indexOfPeriod>$(_originalTargetOS.IndexOf('.'))</_indexOfPeriod> causes the int->string conversion to happen with current culture:

this seems like something MSBuild should consider fixing -- by whatever trick inside the binder. Anything culture-specific by default in a build seems like a bug to me. wdyt?

@github-actions github-actions bot locked and limited conversation to collaborators Mar 28, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
area-NativeAOT-coreclr
Projects
AppModel
Archived in project
Milestone
9.0.0
Development

Successfully merging a pull request may close this issue.

Fix NativeAOT publish failure on fi_FI culture akoeplinger/runtime
5 participants
@tmds @akoeplinger @danmoseley @MichalStrehovsky @De-Crypted