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

Added methods to skip cleaning of output assemblies to ExtensionsMetadataGenerator. #6849

Merged
merged 1 commit into from
Nov 13, 2020

Conversation

gzuber
Copy link
Member

@gzuber gzuber commented Oct 28, 2020

Issue describing the changes in this PR

Resolves #5894

Pull request checklist

  • My changes do not require documentation changes
  • My changes should not be added to the release notes for the next release
    • Otherwise: I've added my notes to release_notes.md
  • My changes do not need to be backported to a previous version
    • Otherwise: Backport tracked by issue/PR #issue_or_pr
  • I have added all required tests (Unit tests, E2E tests)

Additional information

This adds a new property that users can specify in their csproj to define how ExtensionsMetadataGenerator handles output clean up.

They can add the following:

  <ItemGroup>
    <FunctionsPreservedDependencies Include="Microsoft.AspNetCore.Http.dll" />
    <FunctionsPreservedDependencies Include="Microsoft.AspNetCore.Http.Extensions.dll" />
    <FunctionsPreservedDependencies Include="Microsoft.AspNetCore.Http.Features.dll" />
  </ItemGroup>

For every assembly/file in the build output they don't want deleted during the cleaning step.

Notes about the PR:

  • I've made a PR for documenting this feature, but I'm unsure if it's really in the right place and would appreciate a review of the content/wording: Added documentation for build output settings MicrosoftDocs/azure-docs#65127
  • I'm open to suggestions on the name of these settings. I thought FunctionsPreserveOutputAssembly might be better, but went with what I did to match FunctionsSkipCleanOutput. I kept FunctionsSkipCleanOutput because I know some customers use and are familiar with _FunctionsSkipCleanOutput but that might not be a strong enough reason to keep it if we could come up with a better one.
  • I looked into adding tests. My idea was to create a new test function app project (TestProject_SkipClean for instance) with some FunctionsSkipCleanOutputAssembly defined. Then traverse the file system to make sure those assemblies are still in the build output. However, I'm not sure how I can specify that the project should use the ExtensionMetadataGenerator from the project and not the one from Microsoft.NET.Sdk.Functions. I could use some help with this.
  • I'm not sure if I should add to the Host's release notes. Isn't this on its own release cadence that's not tied to the host?

@brettsam
Copy link
Member

Adding Fabio as well to take a look. To answer some questions:

  1. I think that's a good place for them. @ggailey777 may have some feedback if not.
  2. I agree the names seem pretty verbose. @fabiocav may have some recommendations. I'm not sure if we have any other publicly settable properties like this so there may not be precedent for naming.
  3. You may want to add these to my set of end-to-end tests -- we run these before every release against staging packages. For example, here is a project that directly references ExtensionsMetadataGenerator (EMG for short). There's a common file where you can update the EMG version to whichever you want to test and run through tests that way. Feel free to ping me with questions and I can walk you through it.
  4. No, we'll add this to the release notes when we release EMG. Example.

@@ -21,8 +21,17 @@ public class RemoveRuntimeDependencies : Task
[Required]
public string OutputPath { get; set; }

[Required]
public ITaskItem[] FilesToSkip { get; set; }
Copy link
Member

Choose a reason for hiding this comment

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

How about IgnoreFiles ?

@@ -21,11 +22,11 @@
AssemblyFile="$(_FunctionsExtensionsTaskAssemblyFullPath)"/>

<Target Name="_FunctionsBuildCleanOutput" AfterTargets="_GenerateFunctionsExtensionsMetadataPostBuild" Condition="$(_FunctionsSkipCleanOutput) != 'true'" >
<RemoveRuntimeDependencies OutputPath="$(TargetDir)bin"/>
<RemoveRuntimeDependencies OutputPath="$(TargetDir)bin" FilesToSkip="@(FunctionsSkipCleanOutputAssembly)"/>
Copy link
Member

Choose a reason for hiding this comment

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

FunctionsSkipCleanOutputAssembly-> FunctionsPreservedDependencies

@@ -9,6 +9,7 @@
<_FunctionsExtensionsDir Condition="$(_IsFunctionsSdkBuild) == 'true'">$(_FunctionsExtensionsDir)bin</_FunctionsExtensionsDir>
<_ExtensionsMetadataGeneratorTargetsImported>true</_ExtensionsMetadataGeneratorTargetsImported>
<IsPackable>false</IsPackable>
<_FunctionsSkipCleanOutput Condition="$(FunctionsSkipCleanOutput) == 'true' Or $(_FunctionsSkipCleanOutput) == 'true'">true</_FunctionsSkipCleanOutput>
Copy link
Member

Choose a reason for hiding this comment

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

After chatting with @gzuber , let's not expose this as a public property/option right now.

@gzuber gzuber merged commit ca78f5c into dev Nov 13, 2020
@gzuber gzuber deleted the gzuber/skip-clean-output branch November 13, 2020 15:03
@fabiocav
Copy link
Member

@gzuber let's coordinate a NuGet release for this. We should be able to do that at any point

@sonic1981
Copy link

did this get released?

@brettsam
Copy link
Member

@sonic1981 -- yes, see details here: https://github.com/Azure/azure-functions-host/releases/tag/emg-v1.2.1. At the moment, you'll need to explicitly add a package reference to this version of the package (it's usually brought in as a dependency).

@sonic1981
Copy link

Great thanks. Is there a list of what the "assemblies that are shared with the functions runtime" are anywhere?

@sonic1981
Copy link

I still don't really understand how I'm supposed to use this functionality. I'd argue the docs are not really very clear.

Given the web of dependencies that get installed by Nuget and the lack of visibility in Azure of what dll's are available by default and what version they are,, etc it seems a big ask to somehow figure out which ones I want to clean and which I don't. I'm continuing to use the blunt hammer of FunctionsSkipCleanOutput because it just works.

@gzuber
Copy link
Member Author

gzuber commented Mar 26, 2021

Hi @sonic1981 sorry for the delayed response. That's a good point, the documentation isn't specific about the dlls functions is bringing in which can be frustrating. If it helps, we keep a list here that might help you (or at least give you a starting point to) determine which dlls you need to preserve for your project.

I believe that in general, when a customer needs to use this feature it's because they have a very specific requirement e.g. needing to lock themselves to a specific version of a dll. Or they've ran into an exception because the version of a dll they need can't be found, so they're aware of the specific dll that's causing the issue and this feature allows them to fix it without losing much efficiency. FunctionsSkipCleanOutput is still around for situations where that doesn't apply.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[ExtensionsMetadataGenerator] Improve configuration for cleaning build output
4 participants