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

Facades and NotSupported assemblies are bloated with resources they don't use #28888

Open
ericstj opened this issue Mar 6, 2019 · 9 comments
Labels
area-Infrastructure-libraries enhancement Product code improvement that does NOT require public API changes/additions help wanted [up-for-grabs] Good issue for external contributors
Milestone

Comments

@ericstj
Copy link
Member

ericstj commented Mar 6, 2019

Often times we have builds that cross-compile and have very different implementations. Our resource conventions don't force folks to think about this, so the resources are often a superset of all configurations.

We should change the defaults so that this isn't the case.

I propose the following:

  1. By default PartialFacades should not include any resources, unless a project opts-in. Folks should discover this relatively easily as SR.Foo will fail to work in the partial source. They can fix it by setting a flag and potentially providing specific resources for that build of the library.
  2. By default NotSupported assemblies require a unique resx. We can change the path convention to something like Resources\NotSupported.resx instead of strings.resx

Across all our builds this will save over a MB of unused resources.

@joperezr
Copy link
Member

Is this issue only to track those two cases? We also have the case where some resources are only used in a specific configuration but not in the rest. Of course fixing that issue would be harder than simply addressing the two cases you discussed above, but just wanted to know the scope of this issue.

@ericstj
Copy link
Member Author

ericstj commented May 31, 2019

ATM this doesn't account for the per-configuration case since I imagine those have more nuanced requirements.

@msftgits msftgits transferred this issue from dotnet/corefx Feb 1, 2020
@msftgits msftgits added this to the Future milestone Feb 1, 2020
@wtgodbe wtgodbe removed their assignment Feb 3, 2020
@maryamariyan maryamariyan added the untriaged New issue has not been triaged by the area owner label Feb 23, 2020
@ericstj ericstj removed the untriaged New issue has not been triaged by the area owner label Jun 25, 2020
@jkotas jkotas removed the area-Meta label Dec 7, 2020
@danmoseley
Copy link
Member

By default PartialFacades should not include any resources, unless a project opts-in. Folks should discover this relatively easily as SR.Foo will fail to work in the partial source. They can fix it by setting a flag and potentially providing specific resources for that build of the library.

Would this be done by setting to true (if not already explicitly set to false) if is true, then explicitly setting to false as needed to fix the build? Where would such a change go -- I could maybe do this if someone can give pointers.

@joperezr
Copy link
Member

joperezr commented Dec 7, 2020

Fix for that is trivial, here is where we add the embedded resources to all projects when strings.resx exists:

<ItemGroup Condition="'$(StringResourcesPath)' != '' and '$(OmitResources)' != 'true'">
<EmbeddedResource Include="$(StringResourcesPath)">
<Visible>true</Visible>
<ManifestResourceName>$(StringResourcesName)</ManifestResourceName>
<GenerateSource>true</GenerateSource>
<ClassName>$(StringResourcesNamespace).$(StringResourcesClassName)</ClassName>
</EmbeddedResource>
</ItemGroup>

The way to fix what Eric suggests as point #1 above is to add a line in the property group before this line to say something like:

<OmitResources Condition=“'$(OmitResources)' == '' and '$(IsPartialFacadeAssembly)' == 'true'”>true</OmitResources>

@joperezr
Copy link
Member

joperezr commented Dec 7, 2020

And of course after doing that for any partial facade that breaks the build would need to set OmitResources to false on the csproj for that tfm since it will mean that partial facade still requires the resources which is fine.

@danmoseley danmoseley self-assigned this Dec 16, 2020
@danmoseley
Copy link
Member

With the edit above, after building build clr+libs -rc release -allconfigurations these are the projects with errors in one TFM or another

\System.Collections.NonGeneric\src\System.Collections.NonGeneric.csproj
\System.ComponentModel.TypeConverter\src\System.ComponentModel.TypeConverter.csproj
\System.Collections\src\System.Collections.csproj
\System.IO.FileSystem\src\System.IO.FileSystem.csproj
\System.IO.FileSystem.AccessControl\src\System.IO.FileSystem.AccessControl.csproj
\System.Threading\src\System.Threading.csproj
\System.Memory\src\System.Memory.csproj
\System.Security.Permissions\src\System.Security.Permissions.csproj
\System.Security.Cryptography.Primitives\src\System.Security.Cryptography.Primitives.csproj
\System.Transactions.Local\src\System.Transactions.Local.csproj
\System.Runtime.InteropServices\src\System.Runtime.InteropServices.csproj
\System.Reflection.TypeExtensions\src\System.Reflection.TypeExtensions.csproj

@ViktorHofer
Copy link
Member

System.IO.FileSystem.AccessControl.csproj and System.Security.Permissions.csproj are the interesting ones as they multi-target for different TFM families. If you want to go the extra mile (point 2 from the top post), you could save size there as well, by using different resx per TFM group.

@danmoseley
Copy link
Member

The above leaves 87 projects that have IsPartialFacadeAssembly for at least one TFM but apparently don't need resources. However, many of those are in the shared framework, so it has no real impact as they're linked away

If you want to go the extra mile (point 2 from the top post), you could save size there as well, by using different resx per TFM group.

Yup

@danmoseley
Copy link
Member

The PF in S.IO.FS.AC only uses 1 string, but there's only 26, not work special resx perhaps.
S.S.P only has 8 strings total.

@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Dec 17, 2020
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Feb 14, 2021
@ViktorHofer ViktorHofer added the help wanted [up-for-grabs] Good issue for external contributors label Nov 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-Infrastructure-libraries enhancement Product code improvement that does NOT require public API changes/additions help wanted [up-for-grabs] Good issue for external contributors
Projects
None yet
8 participants