Skip to content

Conversation

@jviau
Copy link
Contributor

@jviau jviau commented Aug 24, 2023

Issue describing the changes in this PR

resolves #1834

Pull request checklist

  • My changes do not require documentation changes
    • Otherwise: Documentation issue linked to PR
  • 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 PR is a near complete overhaul of our SDK targets. The aim of the changes is to integrate more closely with existing dotnet SDK targets to eliminate redundant work.

  1. Most Copy tasks have been removed and instead we augment specific item groups so that CopyToOutputDirectory and CopyToPublishDirectory tasks will copy our files for us.
  2. Incremental build support has been improved, we now use Inputs and Outputs.
  • Some incremental build optimizations are thwarted by certain targets from the extension inner build. For example, extensions.json is always written to, so we always see it as changed and also write to it.
  1. Targets have been shifted to perform all incremental steps into the IncrementalOutputPath. Only copying the final files in one go as part of the existing CopyToDirectoryDirectory (or publish dir).
  2. For publish, we never actually call publish on the inner WorkerExtensions.csproj. Instead, we can rely on the existing build targets and collecting of files to accomplish the same thing more efficiently.

Concerns

This is a breaking change for anyone that has customized our targets. Hence the draft state. I am going to go through this and see if I can rename targets back to existing ones to minimize or eliminate the breaking change. See comment below

@jviau
Copy link
Contributor Author

jviau commented Aug 24, 2023

Actually, after reviewing the changes technically this is not breaking. The convention in MSBuild is any property, target, or item starting with _ is considered private. All of our targets start with _ so nothing public is changing.

@jviau jviau marked this pull request as ready for review August 24, 2023 20:51
@jviau jviau changed the title Sdk publish Refactor SDK targets to address multiple writes and flaky builds Aug 24, 2023
Copy link
Member

@brettsam brettsam left a comment

Choose a reason for hiding this comment

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

Reviewing these msbuild targets is tough! I think my only comment is that I'd like to see some comments sprinkled throughout the new Targets explaining what they're doing so it's easier for other folks to find the right place to update in the future.

@jviau jviau merged commit 44388e0 into Azure:feature/ext-proj Sep 29, 2023
@jviau jviau deleted the sdk-publish branch September 29, 2023 00:08
jviau added a commit that referenced this pull request Jan 18, 2024
…d nuget feed issues (#1946)

* Worker Extension project incremental build support (#1749)

* Merge main to feature/ext-proj (#1832)

* Refactor SDK targets to address multiple writes and flaky builds (#1861)

* Ensure ExtensionsCsProjDirectory is a full path (#1938)

* Pass RestoreSources to inner-build restore task (#1937)

* Rev SDK version to 1.16.0-preview1 (#1941)

* Skip func targets during design time build (#1954)

* Correct merge mistakes

* Update sln

* Fix properties

* update worker.config.json generation (#2122)

* [SDK] Remove properties from inner-build (#2161)

* Expand on RemoveProperties for inner build. Fix source gen default

* Add PublishProfile to remove props

* Un-remove some properties from inner build
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.

3 participants