search

dotnet / msbuild

The Microsoft Build Engine (MSBuild) is the build platform for .NET and Visual Studio.
https://docs.microsoft.com/visualstudio/msbuild/msbuild
MIT License
5.23k stars 1.35k forks source link

GenerateResource task does not support non-string resources on .NET core #2221

Closed nguerrera closed 5 years ago

nguerrera commented 7 years ago

From @tarekgh on June 14, 2017 15:54

@Spongman commented on Tue Jun 13 2017

a (.csproj) project a with an embedded text file, added using the VS2017.2 project properties UI.

ConsoleApp2.zip

on windows, it works fine.

on linux, it just shows:

$ dotnet --version
1.0.4
$ uname -a
Linux mybox 3.16.0-4-amd64 #1 SMP Debian 3.16.43-2 (2017-04-30) x86_64 GNU/Linux
$ dotnet run
..\Resources\New Text Document.txt;System.String, mscorlib, Version=4.0.0.0, Culture=neutral, PublicKeyToken=b77a5c561934e089;Windows-1252

@danmosemsft commented on Tue Jun 13 2017

@tarekgh are there any known issues here?

@tarekgh commented on Wed Jun 14 2017

Please take a look at the issue https://github.com/dotnet/cli/issues/3695 this was CLI issue and I believe it is already fixed but I am not sure which release include the fix. I'll move this issue to CLI repo.

Copied from original issue: dotnet/cli#6866

nguerrera commented 7 years ago

MSBuild does not support currently support non-string resources on .NET Core. However, it is not checking the resx data's type attribute, but instead blindly using the value as a string. This should either work or have a clear "not supported" error at build time.

Relevant snippet from repro project resx:

  <data name="New Text Document" type="System.Resources.ResXFileRef, System.Windows.Forms">
    <value>..\Resources\New Text Document.txt;System.String, mscorlib, Version=4.0.0.0, Culture=neutral, PublicKeyToken=b77a5c561934e089;Windows-1252</value>
  </data>

Code that is not checking the type attribute: https://github.com/Microsoft/msbuild/blob/cc727f715651f305639ad8635fefc40165299f86/src/Tasks/GenerateResource.cs#L2909-L2920

It might be easy enough to actually handle this ResxFileRef-to-string case correctly. If we do that, we still need a clear error in other non-string data cases.

eltiare commented 7 years ago

I am eagerly awaiting the embedding of all resources via the commandline as it will make my life easier with the watch command.

ThatRendle commented 7 years ago

As a stop-gap solution while we wait for resource embedding to become a properly-supported, cross-platform, I created a dotnet tool which does something vaguely similar: https://github.com/RendleLabs/Embedder/blob/master/README.md

(I forgot to git push, will do that soon)

jetersen commented 7 years ago

This also applies to images even on Windows using dotnet build, msbuild 馃槶 

.NET Command Line Tools (2.0.2)

Product Information:
 Version:            2.0.2
 Commit SHA-1 hash:  a04b4bf512

Runtime Environment:
 OS Name:     Windows
 OS Version:  10.0.16299
 OS Platform: Windows
 RID:         win10-x64
 Base Path:   C:\Program Files\dotnet\sdk\2.0.2\

Microsoft .NET Core Shared Framework Host

  Version  : 2.0.0
  Build    : e8b8861ac7faf042c87a5c2f9f2d04c98b69f28d
viktors-telle commented 6 years ago

Any ETA on fixing this?

auxym commented 6 years ago

I'm also getting with issue with dotnet build / dotnet msbuild, but not with the msbuild.exe packaged with VS2017, which seems to correctly embed string resources.

AlgorithmsAreCool commented 6 years ago

This explains a lot. I had a WinForms application that would fail to launch at runtime with dotnet build but worked fine in VS2017. I was getting an InvalidCastException when loading an image resource.

EDIT As auxym says, dotnet msbuild does not work as of cli version 2.1.2. You need to call the msbuild visual studio installs. For me this is located at:

"C:\Program Files (x86)\Microsoft Visual Studio\2017\Professional\MSBuild\15.0\Bin\MSBuild.exe"

For some reason this still does not work in release configuration. I have to use debug to get the resources embedded correctly. Hope this helps until a proper fix is available.

mikebridge commented 6 years ago

I have a ResourceManager.GetObject call on an embedded resource which succeeds on windows but fails with the following error on using dotnetcore 2.0.3 (from the dotnet-sdk-2.0.3 docker image) on Linux:

System.InvalidCastException : Unable to cast object of type 'System.String' to type 'System.Byte[]'.

Is this a related issue?

I have an embedded text file "grades.json" which is accessed via the autogenerated Resources.Designer.cs file. That error message occurs here:

internal static byte[] grades {
    get {
        object obj = ResourceManager.GetObject("grades", resourceCulture);
        return ((byte[])(obj));
    }
}
danmoseley commented 6 years ago

ResourceManager will support binary blobs in resources with https://github.com/dotnet/corefx/issues/26745. I am guessing this is also "Unable to cast object of type 'System.String' to type 'System.Byte[]'" .. @viktorhofer can verify.

I had a WinForms application that would fail to launch at runtime with dotnet build

Bear in mind that Winforms is not supported on .NET Core so if it's actually a Winforms app you will hit more errors after this when you try to load a Winforms type.

One other Winforms caveat: the set of binary-serializable types is smaller in .NET Core than in .NET Framework (list) and it does not include for example Image, Icon and Font - types that the Winforms designer likes to binary serialize into resources.

mikebridge commented 6 years ago

@danmosemsft this particular instance is a c# library called from a webapp.

danmoseley commented 6 years ago

@mikebridge what are the type / mimetype you see in the resx? eg 

  <data name="pictureBox1.Image" type="System.Drawing.Bitmap, System.Drawing" mimetype="application/x-microsoft.net.object.bytearray.base64">
mikebridge commented 6 years ago

@danmosemsft The resx file shows this:

<data name="grades" type="System.Resources.ResXFileRef, System.Windows.Forms">
  <value>..\grades.json;System.Byte[], mscorlib, Version=4.0.0.0, Culture=neutral, PublicKeyToken=b77a5c561934e089</value>
</data>

I didn't realize it was specific to winforms---I can embed this file another way if necessary....

danmoseley commented 6 years ago

Yeah, ResxFileRef is a Winforms type for whatever reason (although presumably it doesn't need Winforms proper so it could be ported.) You'll want to embed as a string or a binary formatted string directly into the resx.

tarekgh commented 6 years ago

you can just embed the file directly to the resources and read it with Assembly.GetManifestResourceStream. no need to miss with the resx at all.

mikebridge commented 6 years ago

@danmosemfst @tarekgh, Thanks, I managed to work around it like this:


var assembly = Assembly.GetExecutingAssembly();
String resourceName = @"GradeTranslator.grades.json";
using (var stream = assembly.GetManifestResourceStream(resourceName))
{
    if (stream == null)
    {
        throw new Exception($"Resource {resourceName} not found in {assembly.FullName}.  Valid resources are: {String.Join(", ", assembly.GetManifestResourceNames())}.");
    }
    using (var reader = new StreamReader(stream))
    {
        return reader.ReadToEnd();
    }
}

This appears to work under both dotnet cli and msbuild

amrmahdi commented 6 years ago

When is this planned to be fixed ?

nguerrera commented 6 years ago

I'm pretty sure this is now a must-fix for the announced .NET Core 3.0 support for Windows Forms: https://blogs.msdn.microsoft.com/dotnet/2018/05/07/net-core-3-and-support-for-windows-desktop-applications/ :)

And we can't just error out and say only strings are supported. We have to actually make non-strings work.

danmoseley commented 6 years ago

fyi @ericstj

nguerrera commented 6 years ago

@tarekgh wrote:

you can just embed the file directly to the resources and read it with Assembly.GetManifestResourceStream. no need to miss with the resx at all.

Note that this doesn't work very well if your resources are localized. You also need the logic to get the right resource for the current UI culture. ResourceManager is supposed to be the thing that handles that.

tarekgh commented 6 years ago

Note that this doesn't work very well if your resources are localized. You also need the logic to get the right resource for the current UI culture. ResourceManager is supposed to be the thing that handles that.

That is right. who want language-specific resources will need this functionality fixed otherwise it will not be easy to workaround this except if you do a lot of manual workaround work.

@nguerrera I was looking at some other offline internal reported issue, and it end-up it is the same issue here but using binary files and calling ResourceManager.GetStream. 

  <data name="LanguageDetection" type="System.Resources.ResXFileRef, System.Windows.Forms">
    <value>AudioFiles\pt-br\LanguageDetection.wav;System.IO.MemoryStream, mscorlib, Version=4.0.0.0, Culture=neutral, PublicKeyToken=b77a5c561934e089</value>
  </data>

of course, using mscorlib need to change anyway by our tools.

I have a couple of questions here:

rainersigwald commented 6 years ago

does currently msbuild differ if building for Windows than building on Linux?

The distinction is whether the MSBuild that is building the project is the full-framework MSBuild.exe that is distributed with VS and runs only on Windows, or the .NET Core version that comes with the dotnet SDK and runs cross-platform. The latter does not support some paths through resource generation.

jaredpar commented 6 years ago

For a real world example of where this can come up: this bug is currently blocking the VB compiler tests from running on CoreClr. The dotnet build command embeds our resources used in tests incorrectly and as a result the tests end up failing.

https://github.com/dotnet/roslyn/pull/28063

jaredpar commented 6 years ago

@nguerrera gave me a work around here. Going to move to this declaration to work around it.

<EmbeddedResource Include="results\uncheckedctypeandimplicitconversionseven.txt" />
maxrandolph commented 6 years ago

Commenting for visibility. This is not purely an issue on Linux.

jaredpar commented 6 years ago

@maxrandolph definitely. If you look at all the commits I've been linking in here they are fixing problems on Windows. It's a dotnet build issue.

@nguerrera okay to change the title?

nguerrera commented 6 years ago

I changed the title. It was just carried over from the original user report on dotnet/sdk that got moved here.

nguerrera commented 6 years ago

It's a dotnet build issue.

Correct. (More pedantically, it's an issue with MSBuild running on .NET Core.)

nguerrera commented 6 years ago

Just bumping this comment: https://github.com/Microsoft/msbuild/issues/2221#issuecomment-389000840

This is a must-fix for .NET Core 3. @AndyGerlicher, do we have a milestone or tag we can use for that?

@dsplaisted Can you put this on the agenda for next sync up with msbuild?

cc @dasMulli

rainersigwald commented 6 years ago

We're not currently distinguishing between 16.0 and .NET Core 3 even though the timelines may not match in practice, so pulled this in for 16.0.

danmoseley commented 6 years ago

cc @dreddy-work who is on the winforms side.

rainersigwald commented 6 years ago

I spent some time looking into what MSBuild's dependencies are here.

GenerateResource has a few categories of thing gated behind feature flags:

dsplaisted commented 6 years ago

System.Resources.ResXResourceReader looks like it's available in the Microsoft.DesktopUI shared framework. Probably we don't want to add a dependency in MSBuild to that, so we'll have to discuss further.

nguerrera commented 6 years ago

no resgen equivalent on core

I would imagine it would be possible to build resgen on core now if we need it.

References to System.Resources.ResXResourceReader (from System.Windows.Forms.dll).

I see this type in System.Windows.Forms.dll of 3.0 builds now. I think we still have a layering issue, though. I'm not sure if we can make msbuild depend on the winforms shared runtime component.

In Core there are no appdomains nor assembly unloading.

AFAIK, collectible assemblies are planned for 3.0. https://github.com/dotnet/coreclr/issues/552#issuecomment-410238253

ericstj commented 6 years ago

System.Windows.Forms

We produce a NuGet package for that assembly as well. Again, not sure if that is the final form of this (perhaps we want to push the type(s) down) but it could work for now.

filipnavara commented 6 years ago

I would imagine it would be possible to build resgen on core now if we need it.

I don't think it's necessary. It is not normally used for builds anymore unless someone explicitly forces it. The only reason to bring it back in is perhaps to replace the AppDomain isolation.

sdmaclea commented 6 years ago

@vitek-karas @jeffschwMSFT Note @rainersigwald's comment https://github.com/Microsoft/msbuild/issues/2221#issuecomment-426028467 above regarding AppDomain dependency. Also .Net Core resgen dependency.

ericstj commented 6 years ago

AppDomain dependency

Another angle to take on that is: why does ResGen need to load user code for execution/relfection? ResGen should be able to get everything it needs by just reading assembly metadata and never loading types. That likely means larger changes in either this task or in the WinForms implementation of ResXResourceReader but that's the direction we've been heading with other build tools.

nguerrera commented 5 years ago

I've asked this before, but I'll ask it again here. Why do we need reflection at all to go from one serialized format (resx) to another (resources)?

Ignoring format details, it seems theoretically possible to defer all instantiation until runtime and just encode the resx without loss of information into the resource.

Is there a limitation of the .resources format itself at play, or is it just the API we've been using to write it?

dasMulli commented 5 years ago

The design of .resx for non-string resources is questionable already. Currently it states "use this class for serialisation with this input parameter", the parameter being file references and the classes being inside the windows forms namespaces or system.drawing.. A "new" first-class format would be great (with xliff support), but for portability we'd need some sort of compatibility..

jnm2 commented 5 years ago

Bootstrapping a net472 deployment tool via dotnet run causes this subtle issue. You don't find out until you've already executed the SQL command path\to\file.sql;System.String, mscorlib, Version=4.0.0.0, Culture=neutral, PublicKeyToken=b77a5c561934e089;Windows-1252 against your SQL Server. 馃槀

We're working around it by assuming there will be a Visual Studio installation on the machine. dotnet run deploy\Foo.Deploy becomes:

$visualStudioInstallation = & "${env:ProgramFiles(x86)}\Microsoft Visual Studio\Installer\vswhere.exe" -latest -products * -requires Microsoft.Component.MSBuild -property installationPath
$configuration = 'Release'
$targetFramework = 'net472'

$msbuild = Join-Path $visualStudioInstallation 'MSBuild\15.0\Bin\MSBuild.exe'
& $msbuild deploy\Foo.Deploy /restore /p:Configuration=$configuration /v:quiet /nologo
$fooDeploy= "deploy\Foo.Deploy\bin\$configuration\$targetFramework\Foo.Deploy.exe"

& $fooDeploy $args

Looking forward to the .NET Core 3.0 SDK! 馃帀

wheerd commented 5 years ago

We are currently working around this issue in our projects with the following helper function:

public static class ResourceHelper
{
    public static byte[] GetBytes<T>(string name)
    {
        var assembly = typeof(T).Assembly;
        using (var stream = assembly.GetManifestResourceStream(name))
        {
            if (stream == null)
            {
                throw new Exception(
                    $"Resource {name} not found in {assembly.FullName}.  Valid resources are: {string.Join(", ", assembly.GetManifestResourceNames())}.");
            }
            using (var ms = new MemoryStream())
            {
                stream.CopyTo(ms);
                return ms.ToArray();
            }
        }
    }
}

Then we add the files as <EmbeddedResource Include="Resources\some_file" /> and replace the old Resourceswith this:

namespace Some.Namespace.Resources
{
    public class Resources
    {
        public static byte[] SomeFile => ResourceHelper.GetBytes<Resources>("Some.Namespace.Resources.some_file");
    }
}
rainersigwald commented 5 years ago

Extremely sloppy notes to myself from things I've figured out recently:

jnm2 commented 5 years ago

does type-name resolution but never loads a type (somehow?)

Will https://github.com/dotnet/corefx/pull/33201 possibly help?

rainersigwald commented 5 years ago

Will dotnet/corefx#33201 possibly help?

I don't think so, because the problem is that in the normal current flow, the type is deserialized into a live object in memory, so the assembly that contains the type definition must be really loaded, not reflection-only loaded or metadata loaded.

rainersigwald commented 5 years ago

I did a sloppy prototype to learn about the feasibility of using AddResourceData: https://github.com/rainersigwald/msbuild/commit/9ee5773739eb743dba4bea22cb4ad348dd5ab0da.

It's super close, but doesn't quite work.

Unhandled Exception: System.Runtime.Serialization.SerializationException: The input stream is not a valid binary format. The starting contents (in bytes) are: FF-D8-FF-E0-00-10-4A-46-49-46-00-01-01-01-00-60-00 ...
   at System.Runtime.Serialization.Formatters.Binary.SerializationHeaderRecord.Read(BinaryParser input)
   at System.Runtime.Serialization.Formatters.Binary.BinaryParser.ReadSerializationHeaderRecord()
   at System.Runtime.Serialization.Formatters.Binary.BinaryParser.Run()   at System.Runtime.Serialization.Formatters.Binary.ObjectReader.Deserialize(BinaryParser serParser, Boolean fCheck)
   at System.Runtime.Serialization.Formatters.Binary.BinaryFormatter.Deserialize(Stream serializationStream, Boolean check)
   at System.Runtime.Serialization.Formatters.Binary.BinaryFormatter.Deserialize(Stream serializationStream)   at System.Resources.ResourceReader.DeserializeObject(Int32 typeIndex)
   at System.Resources.ResourceReader.LoadObjectV2(Int32 pos, ResourceTypeCode& typeCode)   at System.Resources.RuntimeResourceSet.GetObject(String key, Boolean ignoreCase, Boolean isString)
   at System.Resources.ResourceManager.GetObject(String name, CultureInfo culture, Boolean wrapUnmanagedMemStream)   at netcore.Form1.InitializeComponent() in S:\work\netcore_resources\netcore\Form1.Designer.cs:line 38
   at netcore.Form1..ctor() in S:\work\netcore_resources\netcore\Form1.cs:line 17
   at netcore.Program.Main() in S:\work\netcore_resources\netcore\Program.cs:line 19

Diffing the .resources file, left=full-framework MSBuild, right=hardcoded AddResourceData

Mode:  Differences
Left file: S:\work\netcore_resources\examples\full\netcore.Form1.resources
Right file: S:\work\netcore_resources\netcore\obj\Debug\netcoreapp3.0\netcore.Form1.resources
0000013C 00 49 00 6D 00 61 00 67  00 65 00 00 00 00 00 40  00 01 00 00 00 FF FF FF  .I.m.a.g.e.....@.....每每每 <> 0000013C 00 49 00 6D 00 61 00 67  00 65 00 00 00 00 00 40                           .I.m.a.g.e.....@
00000154 FF 01 00 00 00 00 00 00  00 0C 02 00 00 00 51 53  79 73 74 65 6D 2E 44 72  每.............QSystem.Dr
0000016C 61 77 69 6E 67 2C 20 56  65 72 73 69 6F 6E 3D 34  2E 30 2E 30 2E 30 2C 20  awing, Version=4.0.0.0,
00000184 43 75 6C 74 75 72 65 3D  6E 65 75 74 72 61 6C 2C  20 50 75 62 6C 69 63 4B  Culture=neutral, PublicK
0000019C 65 79 54 6F 6B 65 6E 3D  62 30 33 66 35 66 37 66  31 31 64 35 30 61 33 61  eyToken=b03f5f7f11d50a3a
000001B4 05 01 00 00 00 15 53 79  73 74 65 6D 2E 44 72 61  77 69 6E 67 2E 42 69 74  ......System.Drawing.Bit
000001CC 6D 61 70 01 00 00 00 04  44 61 74 61 07 02 02 00  00 00 09 03 00 00 00 0F  map.....Data............
000001E4 03 00 00 00 05 4B 00 00  02 FF D8 FF E0 00 10 4A  46 49 46 00 01 01 01 00  .....K...每脴每脿..JFIF.....    0000014C                             FF D8 FF E0 00 10 4A  46 49 46 00 01 01 01 00           每脴每脿..JFIF.....
----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
000001FC 60                                                                         `                        =  0000015B 60                                                                         `
----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
00004CE2 40 05 14 51 40 05 14 51  40 05 14 51 40 1F FF D9  0B                       @..Q@..Q@..Q@.每脵.        <> 00004C41 40 05 14 51 40 05 14 51  40 05 14 51 40 1F FF D9                           @..Q@..Q@..Q@.每脵
----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------

Looks like a missing type header, which I bet is a result of binary serializing the object. I think the resx contains only the payload data, since it also has type information elsewhere in the XML.

It's probably possible to reconstruct the payload header from the resx information directly (looks like a few length-delimited fields), but that might be more coupling with implementation details of BinaryFormatter than we care for.

We could of course do what we do on full framework: use ResXResourceReader to deserialize the type and ResourceWriter to serialize it again. Edit: But note that right now ResourceWriter on core doesn't support BinaryFormatter--that was lit up for reading in https://github.com/dotnet/coreclr/pull/20907 but not writing (https://github.com/dotnet/corefx/issues/26745#issuecomment-438342828).

nguerrera commented 5 years ago

I think we should avoid the deserialize, reseralize route because it's basically incompatible with cross-compilation.

The lengths involved in handling different classic runtime versions for generate resource today demonstrate this.

If we're really just a few header bytes away, let's ask corefx for an api that adds this envelope. I think that would address the coupling concern.

danmoseley commented 5 years ago

@nguerrera I haven't (re)read the entire thread - but is the only reason we're not following .NET Framework verbatim, that dotnet/corefx#26745 is not done?

@ericstj

nguerrera commented 5 years ago

@danmosemsft I don't think so. Using the exact same code as full framework msbuild (is that what you mean by following .NET Framework verbatim?), actively deserializes objects in the build only to reserialize them. I really think we should not do that. At some point, I suspect that even the full framework msbuild should avoid this. With the current approach, we cannot ever have data in a .resx that could only be used at runtime on .NET Core.

Another issue with using the exact same code is that it would introduce a dependency from MSBuild -> Windows Forms, which we shouldn't have anywhere IMHO, and can't have x-plat. We would have to move the RexXResourceReader down and make it portable.

danmoseley commented 5 years ago

@nguerrera that makes sense. I am on the lookout for BCL work necessary for this. It sounds like we haven't identified any remaining BCL requirements here, right now.

nguerrera commented 5 years ago

@rainersigwald was going to set something up to discuss the next steps.

I don't think we can say that we don't need bcl changes for this. We just don't know exactly what we'll need yet. Even if avoiding deserialization works out, we may need API to write the blobs with appropriate header, etc.

And if we don't avoid it, then we do need at least the above work item for ResourceWriter and maybe moving ResxResourceReader out of winforms.