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

GenerateResources task is not incremental on .NET Core #1197

Closed
rainersigwald opened this issue Oct 14, 2016 · 12 comments
Closed

GenerateResources task is not incremental on .NET Core #1197

rainersigwald opened this issue Oct 14, 2016 · 12 comments

Comments

@rainersigwald
Copy link
Member

The CoreResGen target is not incremental, because it assumes that the GenerateResources task handles incrementality internally. It does . . . on full framework, when FEATURE_BINARY_SERIALIZATION is enabled and NeedToRebuildSourceFile gets called.

On .NET Core, though, that codepath falls back to the conservative-but-slow "always just rebuild".

(noticed by @eerhardt in dotnet/sdk#284)

@rainersigwald
Copy link
Member Author

IIRC there's a plan to make binary serialization available everywhere (just not portable between .NET Core and Full Framework). If that is already done this might be easy. If not, we might have to devise a new serialization format 👎

@eerhardt
Copy link
Member

I believe binary serialization is coming to .NET Core with netstandard2.0. But that won't be available for a while yet. So we may have to devise a way around this for Dev15.

/cc @stephentoub @terrajobst @ericstj

@rainersigwald rainersigwald added this to the Visual Studio 15 RC milestone Oct 17, 2016
@terrajobst
Copy link
Member

That's correct, binary serialization will come back.

@danmosemsft, who is working on it and can provide an update where we are with it?

@stephentoub
Copy link
Member

BinaryFormatter is already implemented for 1.2. It's in master. It works. And most of the core types that should be serializable now are, though there are still a bunch of types across corefx that need some work done to restore their serialization implementations.

@jeffkl jeffkl added Priority 2 needs-design Requires discussion with the dev team before attempting a fix. labels Oct 17, 2016
@terrajobst
Copy link
Member

terrajobst commented Oct 17, 2016

@stephentoub: Sweet!

@rainersigwald, what gurantees do you need for incremental build? I suppose you don't care about binary serialization having different output between .NET Framework and .NET Core?

@danmoseley
Copy link
Member

@terrajobst it uses BinaryFormatter for 2 purposes

  1. To cache between builds the set of files if any that are linked to from within the .resx: http://index/#Microsoft.Build.Tasks.Core/ResGenDependencies.cs,183 ... cross FX serialization is not important.

  2. To deserialize types found within the resx in order to figure out whether they are in the GAC. If deserialization fails, they aren't, so it needs to process the resx in its own appdomain. It wants to avoid that as it's so slow. http://index/#Microsoft.Build.Tasks.Core/GenerateResource.cs,1821 That would need tobe cross FX, but it can remain ifdeffed out as we don't have resx's with such types. We do want to fix this later because creating an appdomain is slow and as we have it right now, it will always do it.

  3. ResourceReader/Writer itself will serialize/deserialize types as part of its work. I'm guessing that none of our resx's have such things (it's really a Winforms artefact) so we're getting away without that.

@danmoseley
Copy link
Member

Just realized we can't create AppDomains so that's not an issue. Another reason we shouldn't run on .resx's with serialized user types - we'll lock them.

@danmoseley
Copy link
Member

I think the ifdef can be narrowed to give us pretty good incremental build. Let me see.

@danmoseley
Copy link
Member

A sketch (or possibly the actual) fix for incrmentality without BinarySerialization is in the PR #1202

@rainersigwald
Copy link
Member Author

Ok, so my current understanding is:

.NET Core assemblies cannot use linked resources (that's what it sounds like from comments @DamianEdwards made on https://github.com/dotnet/corefx/issues/8200 which is now https://github.com/dotnet/cli/issues/3695). This implies that it's fine to drop all of the code in GenerateResources that parses the resx and check incrementality as is-resx-newer-than-resources.

That could create a behavior difference when using .NET Core MSBuild to build assemblies for Desktop. CLI currently does that, but the plan has been to always use Desktop MSBuild if it's available.

Given this, I think it's ok to do something like #1202.

@danmosemsft and others--is this a reasonable read on the situation?

@danmoseley
Copy link
Member

@rainersigwald yes I think so. BTW I don't have cycles right now to test/commit #1202 ... for some reason I can't get MSBuild to actually build for me right now.

rainersigwald added a commit to rainersigwald/msbuild that referenced this issue Oct 26, 2016
Closes dotnet#1197.

This implementation is sufficient for current .NET Core--see dotnet#1247 for
details. If we start supporting linked resources, this will produce false
up-to-date results and must be changed.
@rainersigwald rainersigwald added Needs Review and removed needs-design Requires discussion with the dev team before attempting a fix. Needs Review labels Oct 26, 2016
@rainersigwald
Copy link
Member Author

Fixed in xplat packages from 15.1.0-preview-000358-00

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Sprint 108.2
In Progress (Rainer)
Sprint 108.3
Completed
Development

No branches or pull requests

7 participants