Skip to content

Conversation

@rainersigwald
Copy link
Member

@rainersigwald rainersigwald commented Jun 4, 2019

Initial implementation of support for non-string resources from .resx in .NET Core.

Uses System.Resources.Extensions (dotnet/corefx#36906) to embed resources in whatever serialization format they were in in the .resx, deserializing from that format at runtime.

Remaining work:

cc @ericstj @nguerrera @merriemcgaw @dreddy-work

The previous test setup failed on Linux on runtimes less than 3.0,
because of https://github.com/dotnet/corefx/issues/32115.

We can't avoid the delay entirely by artificially setting the date into
the future, because we also need the output of the next execution of the
task to have a later date than the previous one, but we can ensure that
the timestamp on the input file is in the future regardless of
granularity.
Since the new resx reader works on core, use it to populate the list of
linked resources to use in GenerateResource incremental up-to-date
checks.
Fixes a possible test-time crash introduced in f8330fd: there's no
specified ordering for running the finalizer of _env, so sometimes it
might run after another Dispose cleaned up the test environment, and
then attempt to throw an assertion exception at a poorly-specified time,
leading to a crash in the test process instead of a test failure.
@rainersigwald rainersigwald force-pushed the core-resx-understanding branch from c96d1d4 to 8065d64 Compare June 21, 2019 16:28
case "resheader":
break;
case "data":
ParseData(filename, pathsRelativeToBasePath, resources, aliases, elem);
Copy link
Contributor

@cdmihai cdmihai Jun 21, 2019

Choose a reason for hiding this comment

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

nit: make ParseData return resources, to make it clear what its final goal is.


private static void ParseData(string resxFilename, bool pathsRelativeToBasePath, List<IResource> resources, Dictionary<string,string> aliases, XElement elem)
{
string name = elem.Attribute("name").Value;
Copy link
Contributor

Choose a reason for hiding this comment

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

Who checks that the xml is in the right resx format?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I struggled with going strict or permissive here. The current error handling approach is "assume good faith and throw". I'm optimistic that that's ok since most of these files are machine-generated and machine-edited. If it proves not to be in the previews we can make the errors much nicer.

Copy link
Member Author

Choose a reason for hiding this comment

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

@benvillalobos and I came up with a cheap-but-better-than-it-was improvement in 442c03b. I think that's good enough for 16.3p1.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not a fan of that catch (Exception). If I followed right, it will lead to confounding user-facing errors like "MSB3103: Invalid Resx file. Object reference not set to an instance of an object.`" when there is actually a bug in our code.

Copy link
Contributor

Choose a reason for hiding this comment

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

On this being good enough for 16.3p1, my experience is that we don't go back and make aggressive catches less aggressive. I'd rather leave proper error handling unfinished than hacked for a preview.

@rainersigwald rainersigwald force-pushed the core-resx-understanding branch from c5c2f5a to 255de24 Compare June 21, 2019 22:12
This improves the error experience for invalid resx xml quite a bit, but
there's lots of room for further improvement.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area: Task: GenerateResource Problems with the task itself, resgen.exe, and resx resources in general.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants