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

Docker image sets NUGET_XMLDOC_MODE=skip, disrupting nuget restore #2790

Closed
ghost opened this issue May 7, 2021 · 13 comments
Closed

Docker image sets NUGET_XMLDOC_MODE=skip, disrupting nuget restore #2790

ghost opened this issue May 7, 2021 · 13 comments

Comments

@ghost
Copy link

ghost commented May 7, 2021

Describe the Bug

The docker image https://hub.docker.com/_/microsoft-dotnet-sdk sets environment variable NUGET_XMLDOC_MODE=skip. This negatively impacts nuget restore, causing certain package files to fail to restore properly.

While this setting may be useful in your build process during creation of the nuget package, it negatively impacts users of the docker image that may not be aware of this non-default setting. Our build infrastructure used this nuget image as an image for our build agents, and it's causing our builds to fail due to missing files in the nuget restore.

While I do know how to override this setting, I consider this a temporary work around. Nuget images shouldn't leak their build settings into the image produced, and should instead clean up the environment for the user's uses when finalizing the docker image. The only environment variables set in the final image should be (a) those built into the OS it's using (e.g. having a username set, program files, etc.) and (b) those necessary for the container's intended function (e.g. WindowsSdkDir, and other things a Developer Command Prompt for VS 2019 might set).

Steps to Reproduce

in the linked docker container, run echo NUGET_XMLDOC_MODE='%NUGET_XMLDOC_MODE%'

Other Information

Output of docker version

Output of docker info

@MichaelSimons
Copy link
Member

This negatively impacts nuget restore, causing certain package files to fail to restore properly.

@aaronla-ms, can you please include examples (e.g. a concrete example that you encountered)?

This setting is intended for customer scenarios, not the image build process. Restoring xml doc can cause a significant amount of data to be written to the disk during restore operations which has performance impact on Docker builds. Xml doc is most often not useful for non-interactive scenarios which is why the setting exists.

@ghost
Copy link
Author

ghost commented May 11, 2021

This setting doesn't disable just xml docs. My team owns a nuget which (for compatibility reasons) includes xml files that are provided side by side in the lib/net462 folder. Our build team claims to be using these docker images for their builds, and the net results is that nuget restore fails to restore our entire package.

The net result is the build fails to restore packages as it would on a typical developer machine (out of box configuration of visual studio doesn't set NUGET_XMLDOC_MODE=skip, for instance).

@ghost
Copy link
Author

ghost commented May 12, 2021

Per offline discussion, it looks like the NUGET_XMLDOC_MODE is a bit of an "expediency" -- it tries to solve a problem of not unpacking documentation, but it can't do it accurately, since there's no spec for what constitutes "documentation". As a result, it sometimes prunes too much, and breaks our build.

I get the performance win here, but a general purpose image like dotnetsdk should only use reliable, accurate features that are likely to be maximally compatible. This setting amounts to a recursive del /s lib/*.xml, which is Not A Good Idea. Instead, recommend it to users as a way to improve perf IF they have perf issues AND they confirm it's compatible with all their nuget packages.

@MichaelSimons
Copy link
Member

MichaelSimons commented May 12, 2021

@AArnott@aaronla-ms, Can you please open a NuGet issue to track the issue with the NUGET_XMLDOC_MODE setting - e.g. it applies to more than just xmldoc?

@ghost
Copy link
Author

ghost commented May 12, 2021

@MichaelSimons I'm following up with my dev on the nuget package where this was used. On closer inspection, it looks like this xml file is actually a doc file and it was shipped with our service only accidentally. That said, I still can't find any metadata to suggest this -- it just seems to be a file naming convention in nuget. Would appreciate getting to the bottom of this setting.

To summarize,
(a) we probably shouldn't be relying on this xml file in our builds, but
(b) it is a regression that our builds stopped working when we adopted container images with this non-default setting configured.

@AArnott
Copy link

AArnott commented May 12, 2021

@MichaelSimons I wonder why you're asking me to do this. I'm not working with docker and I'm not on the nuget team either. I could do this, but if you do it, you'll get the notifications and I won't, which sounds appropriate since this has nothing to do with me.

FWIW though, have you folks thought about the "dev container" scenario? VS Code promotes using a .devcontainer/Dockerfile in repos so that anyone who opens the repo can be immediately productive in development, with all dependencies satisfied from the container. These containers are often based on a dotnet sdk docker image (for example this one). Since these are used for development, dropping .xml doc files will do substantial harm to the Intellisense experience for these developers.

@MichaelSimons
Copy link
Member

I apologize @AArnott, I mistakenly referenced your username versus the original author aaronla. You raise a good point however, we will take a look at this scenario.

@MichaelSimons
Copy link
Member

[Triage] We should validate the e2e UX the xml doc setting has on the dev container scenario. We should also measure the gains the xml doc setting has on a range of restore scenarios. This setting was originally added early on in .NET to provide a smaller sdk image as a result of the SDK carrying around the NuGet package cache. To summarize, we are saying the reasons the xml doc setting were added in the first place no longer exists so we should investigate it's usefulness.

@mthalman, please investigate.

@mthalman
Copy link
Member

Not setting NUGET_XMLDOC_MODE=skip definitely has an impact on both restore time and disk space.

For my testing, I used the project for the C# TechEmpower benchmark: https://github.com/TechEmpower/FrameworkBenchmarks/blob/6769bf6dd9fde18c30e63f700689fc7e9b4232bd/frameworks/CSharp/aspnetcore/Benchmarks/Benchmarks.csproj. This project has 5 top-level package references. This expands out to a total of 116 nested packages that actually get restored. For my testing, I retargeted the Dockerfile to use the .NET 6 SDK.

Average restore time:

  • With NUGET_XMLDOC_MODE=skip: 14.5 seconds (0.125 seconds per package)
  • Without NUGET_XMLDOC_MODE=skip: 20.7 seconds (0.178 seconds per package)

This is a 42% increase in restore time when NUGET_XMLDOC_MODE=skip isn't set.

For disk space:

  • With NUGET_XMLDOC_MODE=skip: 306 MB (2.64 MB per package)
  • Without NUGET_XMLDOC_MODE=skip: 679 MB (5.85 MB per package)

This is a 121% increase in disk space when NUGET_XMLDOC_MODE=skip isn't set. This disk usage is only relevant to the image layer that has restored the packages (typically the build stage of a Dockerfile). That layer wouldn't typically be included in the final application image.

@MichaelSimons
Copy link
Member

[Triage] There is clearly value in maintaining the NUGET_XMLDOC_MODE=skip. We feel this is beneficial to the 80+ percentage use cases. In the cases where this isn't desired, the recommendation is to override this setting. Another options for use cases like this is to retrieve the xml doc file explicitly from the nupkg.

AArnott added a commit to AArnott/Library.Template that referenced this issue May 29, 2021
When using devcontainer for development, the xml files were not getting unpacked due to an environment variable being set in the docker container. This reverts that.

Workaround for dotnet/dotnet-docker#2790
@AArnott
Copy link

AArnott commented May 29, 2021

Another options for use cases like this is to retrieve the xml doc file explicitly from the nupkg.

The use case is .NET development within a container. I don't know how the compiler would be told to take this option.

In the cases where this isn't desired, the recommendation is to override this setting.

For others' reference, the way to override this setting is to add this to the Dockerfile:

ENV NUGET_XMLDOC_MODE=

@mthalman
Copy link
Member

mthalman commented Jun 1, 2021

The use case is .NET development within a container. I don't know how the compiler would be told to take this option.

Right, so there's a tension here between the two scenarios: build automation and developer environment. Which do we optimize for? To date, we've mostly been focused on satisfying the former. Perhaps there's an opportunity to provide a container curated for dev environment scenarios. This warrants further discussion.

@ghost
Copy link
Author

ghost commented Jun 1, 2021

Perhaps there's an opportunity to provide a container curated for dev environment scenarios. This warrants further discussion.

Possibly, but the differences should be incredibly minor -- otherwise you risk "it works on my machine but not in the lab"-itis.

eerhardt added a commit to eerhardt/images that referenced this issue Jan 31, 2024
Clear this environment variable so xml docs from NuGet packages are unpackaged. The default dotnet/sdk image sets it to 'skip'.

Without clearing this environment variable, the XML docs are not unpackaged and won't be used in intellisense.

See dotnet/dotnet-docker#2790
samruddhikhandale pushed a commit to devcontainers/images that referenced this issue Jan 31, 2024
Clear this environment variable so xml docs from NuGet packages are unpackaged. The default dotnet/sdk image sets it to 'skip'.

Without clearing this environment variable, the XML docs are not unpackaged and won't be used in intellisense.

See dotnet/dotnet-docker#2790
ericdotnet added a commit to ericdotnet/Streams-Nerdbank-pro that referenced this issue May 13, 2024
When using devcontainer for development, the xml files were not getting unpacked due to an environment variable being set in the docker container. This reverts that.

Workaround for dotnet/dotnet-docker#2790
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants