s2i build breaks due to LC_ALL warnings in any Exec tasks launched from msbuild

omajid commented 11 months ago

I have an application where I need to run a custom script after building it.

It's important to me that the custom script works successfully when I run it, so I set LogStandardErrorAsError="true".

Here's what it looks like:

Program.cs:

// See https://aka.ms/new-console-template for more information
Console.WriteLine("Hello, World!");

warnings.csproj:

<Project Sdk="Microsoft.NET.Sdk">

  <PropertyGroup>
    <OutputType>Exe</OutputType>
    <TargetFramework>net6.0</TargetFramework>
    <ImplicitUsings>enable</ImplicitUsings>
    <Nullable>enable</Nullable>
  </PropertyGroup>

  <Target Name="WhatAboutWarnings" AfterTargets="Build">
    <Exec Command="echo hi" LogStandardErrorAsError="true" />
  </Target>

</Project>

Building this works fine locally. Building it using s2i containers fails:

$ s2i -U unix:///run/user/1000/podman/podman.sock build .  registry.access.redhat.com/ubi8/dotnet-60
---> Installing application source...
Using SDK: 6.0.123
---> Restoring application dependencies...
Determining projects to restore...
Restored /opt/app-root/src/warnings.csproj (in 63 ms).
---> Publishing application...
Microsoft (R) Build Engine version 17.0.1+b177f8fa7 for .NET
Copyright (C) Microsoft Corporation. All rights reserved.

warnings -> /opt/app-root/src/bin/Release/net6.0/warnings.dll
/opt/app-root/src/warnings.csproj(11,5): error : /usr/bin/sh: warning: setlocale: LC_ALL: cannot change locale (en_US.UTF-8)
/opt/app-root/src/warnings.csproj(11,5): error : /usr/bin/sh: warning: setlocale: LC_ALL: cannot change locale (en_US.UTF-8)
hi
/opt/app-root/src/warnings.csproj(11,5): error MSB3073: The command "echo hi" exited with code -1.
Build failed
ERROR: An error occurred: non-zero (13) exit code from registry.access.redhat.com/ubi8/dotnet-60

Another version of this error can happen when I use third-party packages such as NSwag.MSBuild. These third-party packages do the Exec for me and I can not control them: https://github.com/RicoSuter/NSwag/blob/512eb654b1be5714c7923b74d22e2049d56b5622/src/NSwag.ApiDescription.Client/NSwag.ApiDescription.Client.targets#L32

omajid commented 11 months ago

The LC_ALL setting is done by msbuild: https://github.com/dotnet/msbuild/issues/4194

Maybe we should install glibc-langpack-en to stop this warning?

tmds commented 11 months ago

The LC_ALL setting is done by msbuild: https://github.com/dotnet/msbuild/issues/4194

This referenced issue is reporting the same problem. And the issue affects Microsoft images as much as ours.

It would be preferable to get an upstream fix rather than work around it.

omajid commented 11 months ago

@tmds Thanks for working on a long-term upstream fix for this!

Any thoughts on what we should do for 6.0 and/or 7.0 (and maybe even 8.0)? Our customers are actively hitting this. Do we want to backport the msbuild fix (seems too risky for everything before 8.0), add the glibc-langpack-en packages to containers or leave this as an issue that won't be fixed?

tmds commented 11 months ago

I expect tooling invoked from msbuild through Exec is going to work as well when ran under the C locale as when ran under the en_US.UTF-8 locale. So that patch to msbuild is a good option for me.

Otherwise installing glibc-langpack-en will also fix the issue at a small size cost. That does make the en_US locale available in our images, which we may then remove again in a future version, which then will be a breaking change. The package could be installed in the container images, or it could be added as a dependency of the sdk package (since that is what it is, when msbuild depends on it).

omajid commented 11 months ago

And the issue affects Microsoft images as much as ours.

My test case above passes on all of:

mcr.microsoft.com/dotnet/sdk:6.0
mcr.microsoft.com/dotnet/sdk:6.0-alpine
mcr.microsoft.com/dotnet/sdk:6.0-jammy

The failure seems specific to Red Hat's container images.

aslicerh commented 11 months ago

The package could be installed in the container images, or it could be added as a dependency of the sdk package (since that is what it is, when msbuild depends on it).

@omajid and I were talking about this. Do you have a preference about this?

My first thoughts on it were that, in most circumstances, not having the package doesn't cause an issue. And when it does the solution to the issue is pretty straight forward (install the package, either on the host system or when you're putting your app on top of our images). The only time it really becomes an issue is using our s2i support to build images.

As such, adding the package as a dependency to the SDK seems like turning an optional dependency into a required one. Similarly, lacking the package, generally speaking isn't causing .NET to break as much as it is breaking our s2i support.

tmds commented 11 months ago

The failure seems specific to Red Hat's container images.

Interesting.

The warning is printed by bash.

That's why the reproducer for the Microsoft image is with a script that uses bash: https://github.com/dotnet/msbuild/issues/4194.

It affects our images more because /bin/sh links to bash, whereas Microsoft images default to another shell.

As such, adding the package as a dependency to the SDK seems like turning an optional dependency into a required one. Similarly, lacking the package, generally speaking isn't causing .NET to break as much as it is breaking our s2i support.

ok.

Do you have a preference about this?

I prefer to fix it with a patch to msbuild because that removes the unneeded dependency in all cases, and it doesn't add additional files to our images.

If you prefer to add glibc-langpack-en to the images that is also fine by me. Maybe we can consider to patch 8.0 in that case.

omajid commented 11 months ago

I prefer to fix it with a patch to msbuild because that removes the unneeded dependency in all cases, and it doesn't add additional files to our images.

I am worried about the msbuild change being a breaking change in 6.0. If a user was relying on the locale being set by msbuild, the fix would change that behaviour. If the choice is leaving the bug in for 6.0 (67% through it's entire lifecycle) vs fixing in a a breaking way, I would rather leave the bug in, and ask folks to migrate to a recent version with the bug fixed.

If you prefer to add glibc-langpack-en to the images that is also fine by me.

I am leaning towards this (either directly, or via a SDK dependency), because this is the only non-breaking option I see.

Maybe we can consider to patch 8.0 in that case.

I would be strongly in favour of this, specially if we can get upstream msbuild to make this change in their 8.0 branch as well. If this is a fix that's specific to our SDK, then I am hesitant about that too - it's another way our SDK diverges from Microsoft's SDK. But it's still much, much less risky than making this change in 6.0.

tmds commented 11 months ago

this is the only non-breaking option I see

You want to add glibc-langpack-en to account for Exec calling tools that only work under the US English locale. I don't think this is a problem we need to solve. If there are such tools (which I consider unlikely), they won't work in Microsoft's images either. Such tools need to be fixed to work under 'any' locale.

tmds commented 11 months ago

We can patch msbuild to no longer set a locale.

diff --git a/src/Tasks/Exec.cs b/src/Tasks/Exec.cs
index df817422bfd..95f43a488bb 100644
--- a/src/Tasks/Exec.cs
+++ b/src/Tasks/Exec.cs
@@ -590,7 +590,7 @@ protected internal override void AddCommandLineCommands(CommandLineBuilderExtens
             {
                 commandLine.AppendSwitch("-c");
                 commandLine.AppendTextUnquoted(" \"");
-                commandLine.AppendTextUnquoted("export LANG=en_US.UTF-8; export LC_ALL=en_US.UTF-8; . ");
+                commandLine.AppendTextUnquoted(". ");
                 commandLine.AppendFileNameIfNotNull(batchFileForCommandLine);
                 commandLine.AppendTextUnquoted("\"");
             }

That will fix the issue reported here.

If a user has a tool that must have the US English locale (which I don't think is an issue we need to address), they can build a custom image that includes glibc-langpack-en and set the envvars.

tmds commented 11 months ago

they won't work in Microsoft's images either.

Actually, they would. I'm conflating native system locales and .NET locale support through libicu.

There is a way to set the locale to en_US.UTF-8 without bash printing warnings.

From setlocale:

> For glibc, first
       (regardless of category), the environment variable LC_ALL is
       inspected, next the environment variable with the same name as
       the category (see the table above), and finally the environment
       variable LANG.

bash only prints warnings for the LCs but not LANG.

So we can set the locale to en_US.UTF-8 by using:

commandLine.AppendTextUnquoted("unset LC_{ALL,ADDRESS,COLLATE,CTYPE,IDENTIFICATION,MEASUREMENT,MESSAGES,MONETARY,NAME,NUMERIC,PAPER,TELEPHONE,TIME}; export LANG=en_US.UTF-8; . ");

omajid commented 11 months ago

After some discussion, this seems like the outline of the plan we are considering:

.NET 6 and .NET 7:
- Option 1: add glibc-langpack-en to container images
- Option 1.1: Add dependency to dotnet-sdk-x.y package
- Option 1.2: Add glibc-langpack-en to container images manually
- Option 2: No change, .NET 6 and .NET 7 are too close to EOL
.NET 8:
- Option 1: Backport the upstream fix from https://github.com/dotnet/msbuild/issues/4194 (same as https://github.com/redhat-developer/s2i-dotnetcore/issues/478#issuecomment-1801650150) and guard it under a changewave
.NET 9: Wait for upstream fix for https://github.com/dotnet/msbuild/issues/4194

redhat-developer / s2i-dotnetcore

s2i build breaks due to LC_ALL warnings in any Exec tasks launched from msbuild #478