Resources should not be emitted into ref assemblies #31244

jcouv · 2018-11-18T22:38:43Z

Currently, resources are emitted into ref assemblies produced with /refout. The impact is that modifying a resource file causes dependent projects to re-compile un-necessarily.

Fixes #31197

Update (12/5/2018):
From discussion with Jared and Neal, we will not include resources in ref assemblies produced by /refonly either. We would like to hear from customer that need that. If there are some and we decide we need to include resources in that scenario after all, we'll have to design some solution (/refout:<option> or something).

Update (12/6/2018):
There is still some discussion about the /refonly case (#2184 (comment))

Update (1/10/2019):
From discussion with Eric StJohn and other BCL folks, we think it is safe to remove the resources. I'll still document as a breaking change though.

jaredpar · 2018-11-19T19:22:44Z

src/Compilers/Core/Portable/Compilation/Compilation.cs

@@ -2862,7 +2862,7 @@ private static Stream ConditionalGetOrCreateStream(EmitStreamProvider metadataPE
                Debug.Assert(!includePrivateMembers);

                if (!Cci.PeWriter.WritePeToStream(
-                    new EmitContext(moduleBeingBuilt, null, metadataDiagnostics, metadataOnly: true, includePrivateMembers: false),
+                    new EmitContext(moduleBeingBuilt, null, metadataDiagnostics, metadataOnly: true, includePrivateMembers: false, includeManifestResources: false),


Why do we have this new parameter vs. keying off of metadataOnly? #Resolved

In the refonly scenario, the primary assembly will be emitted with metadataOnly=true, but it should contain the metadata resource.
This is the spec'ed behavior (#2184) which Neal confirmed. It is a bit different from what you and I discussed last week.
Neal's reasoning is that if you specified resources on the command-line, you mean them to go somewhere. #Resolved

jaredpar · 2018-11-19T19:23:52Z

src/Compilers/Core/Portable/Emit/CommonPEModuleBuilder.cs

+            {
+                // Manifest resources are not included in ref assemblies
+                // Ref assemblies don't support added modules
+                return ImmutableArray<Cci.ManagedResource>.Empty;


This comment does not line up with the behavior. The behavior is that resources are included when the ref assembly is a secondary compilation. When it's the primary compilation then they are included.

I don't agree with this behavior but the change appears to be currently implemneted this way. #Resolved

jaredpar · 2018-11-19T19:24:57Z

src/Compilers/Core/Portable/Compilation/Compilation.cs

@@ -2840,7 +2840,7 @@ private static Stream ConditionalGetOrCreateStream(EmitStreamProvider metadataPE
            bool includePrivateMembersOnPrimaryOutput = metadataOnly ? includePrivateMembers : true;
            bool deterministicPrimaryOutput = (metadataOnly && !includePrivateMembers) || isDeterministic;
            if (!Cci.PeWriter.WritePeToStream(
-                new EmitContext(moduleBeingBuilt, null, metadataDiagnostics, metadataOnly, includePrivateMembersOnPrimaryOutput),
+                new EmitContext(moduleBeingBuilt, null, metadataDiagnostics, metadataOnly, includePrivateMembersOnPrimaryOutput, includeManifestResources: true),


Feel like this is the wrong approach. It means reference assemblies will have different content depending on how they are compiled. It also means that /refout is not equivalent to /out + /refonly. That feels wrong. #Resolved

Tagging @gafter for discussion of desired behavior. #Resolved

jaredpar · 2018-11-19T19:26:48Z

src/Compilers/CSharp/Portable/Emitter/Model/PEAssemblyBuilder.cs

                        {
-                            builder.Add(resource);
+                            // resources are not emitted into ref assemblies when a full assembly is also produced


The comment and if check seem out of sync (private members vs. resources) #Resolved

jaredpar · 2018-11-19T19:27:42Z

Done with review pass (iteration 1) #Resolved

jcouv · 2018-11-27T15:53:29Z

FYI, I'm holding off this PR to resolve the desired behavior with Jared and Neal. #Resolved

jcouv · 2019-01-11T01:08:04Z

@jaredpar This is ready for review again. Removed resources from all ref assemblies. #Resolved

jasonmalinowski · 2019-01-11T02:43:13Z

@jcouv Does this include any resources that specify version metadata if I pull up File Properties in Windows? I admit I don't know the exact file formats involved there; I could have sworn one of those counts as a resource of some kind. #Resolved

jcouv · 2019-01-11T18:01:57Z

@jasonmalinowski No, this only affect resources passed from -resource: command-line option.

I've double-checked that CommonCommandLineArguments.ManifestResources flows straight into CommonPEModuleBuilder.ManifestResources (which this PR interacts with). #Resolved

jaredpar · 2019-01-11T21:22:38Z

src/Compilers/CSharp/Portable/Emitter/Model/PEAssemblyBuilder.cs

@@ -17,6 +17,7 @@ internal abstract class PEAssemblyBuilderBase : PEModuleBuilder, Cci.IAssemblyRe
        private readonly SourceAssemblySymbol _sourceAssembly;
        private readonly ImmutableArray<NamedTypeSymbol> _additionalTypes;
        private ImmutableArray<Cci.IFileReference> _lazyFiles;
+        private ImmutableArray<Cci.IFileReference> _lazyFilesWithoutManifestResources; // we don't include manifest resources in ref assemblies


Consider putting the comment above the field. That will cause it to show up in Intellisense IIRC.

The only comments we show are going to be doc comments with a <summary> tag.

jaredpar

jcouv · 2019-01-14T21:47:16Z

@gafter @dotnet/roslyn-compiler for a second review. Thanks

gafter

jcouv added the Area-Compilers label Nov 18, 2018

jcouv added this to the 16.0.P2 milestone Nov 18, 2018

jcouv self-assigned this Nov 18, 2018

jcouv requested a review from a team as a code owner November 18, 2018 22:38

jcouv added the PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. label Nov 19, 2018

jcouv force-pushed the refout-resource branch from 48ab876 to fd19dfe Compare November 19, 2018 15:53

jaredpar reviewed Nov 19, 2018

View reviewed changes

jcouv force-pushed the refout-resource branch from 99e86ec to 460e565 Compare November 20, 2018 03:28

jcouv mentioned this pull request Dec 6, 2018

Make the command-line compilers able to generate reference assemblies #2184

Closed

jcouv mentioned this pull request Dec 17, 2018

Verify ref assembly behavior regarding embedded resources #31197

Closed

jcouv force-pushed the refout-resource branch from 460e565 to 6c6d158 Compare January 10, 2019 19:30

jcouv requested a review from a team as a code owner January 10, 2019 19:30

jcouv force-pushed the refout-resource branch 3 times, most recently from 7fdf2e0 to 8c47772 Compare January 10, 2019 23:00

jcouv changed the title ~~Resources should not be emitted into secondary output assemblies~~ Resources should not be emitted into ref assemblies Jan 10, 2019

jcouv changed the base branch from master to dev16.0-preview2 January 10, 2019 23:03

jcouv force-pushed the refout-resource branch from 8c47772 to 9d607df Compare January 10, 2019 23:03

jcouv modified the milestones: 16.0.P2, 16.0.P3 Jan 11, 2019

jcouv changed the base branch from dev16.0-preview2 to master January 11, 2019 00:04

jcouv added 3 commits January 10, 2019 16:05

Secondary assembly output should not include resources

122dafc

Add missing arguments

0cf0e3d

Don't emit resources in refonly ref assembly either

56c9840

jcouv force-pushed the refout-resource branch from 9d607df to 56c9840 Compare January 11, 2019 00:05

jcouv removed the PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. label Jan 11, 2019

Update EmbeddedText comment

69fd874

jaredpar reviewed Jan 11, 2019

View reviewed changes

jaredpar approved these changes Jan 11, 2019

View reviewed changes

Add xml doc comment

9c1513a

gafter approved these changes Jan 15, 2019

View reviewed changes

jcouv merged commit 50bb3fa into dotnet:master Jan 15, 2019

jcouv deleted the refout-resource branch January 15, 2019 05:59

xoofx pushed a commit to stark-lang/stark-roslyn that referenced this pull request Apr 16, 2019

Resources should not be emitted into ref assemblies (dotnet#31244)

216c002

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Resources should not be emitted into ref assemblies #31244

Resources should not be emitted into ref assemblies #31244

jcouv commented Nov 18, 2018 •

edited

Loading

jaredpar Nov 19, 2018 •

edited by jcouv

Loading

jcouv Nov 19, 2018 •

edited

Loading

jaredpar Nov 19, 2018 •

edited by jcouv

Loading

jaredpar Nov 19, 2018 •

edited by jcouv

Loading

jcouv Nov 19, 2018 •

edited

Loading

jaredpar Nov 19, 2018 •

edited by jcouv

Loading

jaredpar commented Nov 19, 2018 •

edited by jcouv

Loading

jcouv commented Nov 27, 2018 •

edited

Loading

jcouv commented Jan 11, 2019 •

edited

Loading

jasonmalinowski commented Jan 11, 2019 •

edited by jcouv

Loading

jcouv commented Jan 11, 2019 •

edited

Loading

jaredpar Jan 11, 2019

jasonmalinowski Jan 11, 2019

jaredpar left a comment

jcouv commented Jan 14, 2019

gafter left a comment

Resources should not be emitted into ref assemblies #31244

Resources should not be emitted into ref assemblies #31244

Conversation

jcouv commented Nov 18, 2018 • edited Loading

jaredpar Nov 19, 2018 • edited by jcouv Loading

Choose a reason for hiding this comment

jcouv Nov 19, 2018 • edited Loading

Choose a reason for hiding this comment

jaredpar Nov 19, 2018 • edited by jcouv Loading

Choose a reason for hiding this comment

jaredpar Nov 19, 2018 • edited by jcouv Loading

Choose a reason for hiding this comment

jcouv Nov 19, 2018 • edited Loading

Choose a reason for hiding this comment

jaredpar Nov 19, 2018 • edited by jcouv Loading

Choose a reason for hiding this comment

jaredpar commented Nov 19, 2018 • edited by jcouv Loading

jcouv commented Nov 27, 2018 • edited Loading

jcouv commented Jan 11, 2019 • edited Loading

jasonmalinowski commented Jan 11, 2019 • edited by jcouv Loading

jcouv commented Jan 11, 2019 • edited Loading

jaredpar Jan 11, 2019

Choose a reason for hiding this comment

jasonmalinowski Jan 11, 2019

Choose a reason for hiding this comment

jaredpar left a comment

Choose a reason for hiding this comment

jcouv commented Jan 14, 2019

gafter left a comment

Choose a reason for hiding this comment

jcouv commented Nov 18, 2018 •

edited

Loading

jaredpar Nov 19, 2018 •

edited by jcouv

Loading

jcouv Nov 19, 2018 •

edited

Loading

jaredpar Nov 19, 2018 •

edited by jcouv

Loading

jaredpar Nov 19, 2018 •

edited by jcouv

Loading

jcouv Nov 19, 2018 •

edited

Loading

jaredpar Nov 19, 2018 •

edited by jcouv

Loading

jaredpar commented Nov 19, 2018 •

edited by jcouv

Loading

jcouv commented Nov 27, 2018 •

edited

Loading

jcouv commented Jan 11, 2019 •

edited

Loading

jasonmalinowski commented Jan 11, 2019 •

edited by jcouv

Loading

jcouv commented Jan 11, 2019 •

edited

Loading