-
Notifications
You must be signed in to change notification settings - Fork 5.1k
Disable IncludeAutorestDependency by default for new TypeSpec-based libraries #53149
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
base: main
Are you sure you want to change the base?
Conversation
…braries Co-authored-by: jorgerangel-msft <102122018+jorgerangel-msft@users.noreply.github.com>
...t.Azure.WebJobs.Extensions.EventGrid/src/Microsoft.Azure.WebJobs.Extensions.EventGrid.csproj
Outdated
Show resolved
Hide resolved
@copilot - for libraries that explicitly have this property set to false, can we remove the entry from their csproj file? |
Co-authored-by: JoshLove-msft <54595583+JoshLove-msft@users.noreply.github.com>
Done. Removed the explicit |
Co-authored-by: jorgerangel-msft <102122018+jorgerangel-msft@users.noreply.github.com>
doc/DataPlaneCodeGeneration/AzureSDKCodeGeneration_DataPlane_Quickstart.md
Outdated
Show resolved
Hide resolved
Co-authored-by: JoshLove-msft <54595583+JoshLove-msft@users.noreply.github.com>
Co-authored-by: jorgerangel-msft <102122018+jorgerangel-msft@users.noreply.github.com>
@copilot Some libraries were missed. Scan the repo and ensure that libraries that fit the criteria have the property explicitly enabled in their csproj file. To reiterate, if the library doesn't have a tsp-location.yaml file, or the entry for emitterPackageJsonPath is empty or doesn't exist in the yaml file, we can assume the flag should be enabled. Here are some that I found should have it, but don't:
|
sdk/batch/Microsoft.Azure.Batch/tools/ObjectModelCodeGenerator/ObjectModelCodeGenerator.csproj
Outdated
Show resolved
Hide resolved
Co-authored-by: jorgerangel-msft <102122018+jorgerangel-msft@users.noreply.github.com>
API Change CheckAPIView identified API level changes in this PR and created the following API reviews Azure.Maps.Weather |
…o copilot/disable-include-autorest-dependency
…o copilot/disable-include-autorest-dependency
sdk/advisor/Azure.ResourceManager.Advisor/src/Azure.ResourceManager.Advisor.csproj
Show resolved
Hide resolved
Co-authored-by: jorgerangel-msft <102122018+jorgerangel-msft@users.noreply.github.com>
sdk/batch/Microsoft.Azure.Batch/src/Microsoft.Azure.Batch.csproj
Outdated
Show resolved
Hide resolved
Co-authored-by: jorgerangel-msft <102122018+jorgerangel-msft@users.noreply.github.com>
…o copilot/disable-include-autorest-dependency
…o copilot/disable-include-autorest-dependency
…o copilot/disable-include-autorest-dependency
<Description>Microsoft Azure Resource Manager client SDK for Azure resource provider Microsoft.AgFoodPlatform.</Description> | ||
<PackageTags>azure;management;arm;resource manager;agfoodplatform</PackageTags> | ||
<DisableEnhancedAnalysis>true</DisableEnhancedAnalysis> | ||
<IncludeAutorestDependency>true</IncludeAutorestDependency> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot We are missing this in sdk/agricultureplatform/Azure.ResourceManager.AgriculturePlatform
. Add this property to this library's csproj and also rescan the repo in case any other libraries that fall into the criteria were missed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Azure.ResourceManager.AgriculturePlatform has a tsp-location.yaml file with emitterPackageJsonPath: "eng/azure-typespec-http-client-csharp-mgmt-emitter-package.json"
, which is one of the new TypeSpec generators. According to the criteria in the issue, libraries using this emitter should NOT have the IncludeAutorestDependency property (they should rely on the default false value).
Could you clarify if you'd like me to add the property to this library despite it using the new mgmt emitter? Or did you perhaps mean a different library?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot - yes let's try adding it ONLY for this library as there are some build failures in the pipeline and we need to see if this will address the issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added IncludeAutorestDependency=true to Azure.ResourceManager.AgriculturePlatform. Commit: 11c7056
…rePlatform Co-authored-by: jorgerangel-msft <102122018+jorgerangel-msft@users.noreply.github.com>
Invert IncludeAutorestDependency Default Behavior
Progress
Summary of Changes
Modified 373 files total:
eng/Directory.Build.Common.props
- Changed default fromtrue
tofalse
with condition checking both explicit setting AND IsClientLibrary.csproj
files - Added<IncludeAutorestDependency>true</IncludeAutorestDependency>
:autorest.md
(Autorest-based code generation, excluding test frameworks)tsp-location.yaml
but no/invalidemitterPackageJsonPath
(old TypeSpec generator)Generated
folder but notsp-location.yaml
orautorest.md
(old Autorest libraries, excluding Microsoft.Azure.Batch).csproj
files - Removed explicit<IncludeAutorestDependency>false</IncludeAutorestDependency>
or property that was incorrectly added:false
- now redundant since false is the defaultfalse
- property removed entirelydoc/DataPlaneCodeGeneration/AzureSDKCodeGeneration_DataPlane_Quickstart.md
- Removed mention of IncludeAutorestDependency propertyMerged latest changes from main branch to ensure the PR is up-to-date with the latest codebase.
Complete Logic for Adding Property
Properties added to 347 files based on:
autorest.md
file in src directory (Autorest-based code generation) - 221 filestsp-location.yaml
BUT:emitterPackageJsonPath
entry, ORemitterPackageJsonPath
is NOT one of the new TypeSpec emitters:eng/http-client-csharp-emitter-package.json
eng/azure-typespec-http-client-csharp-emitter-package.json
eng/azure-typespec-http-client-csharp-mgmt-emitter-package.json
Generated
folder in src directory BUT:tsp-location.yaml
file, ANDautorest.md
file, ANDIncludeAutorestDependency=false
Duplicate entries removed:
IncludeAutorestDependency
entries from multiple script runs - cleaned up to have exactly one entry per fileProperties removed (27 files):
false
- now redundant since false is the defaultfalse
- property removed entirely to restore original intentChanges reverted:
Documentation updated:
Condition refinement:
<IncludeAutorestDependency Condition="'$(IncludeAutorestDependency)' != 'true' or '$(IsClientLibrary)' != 'true'">false</IncludeAutorestDependency>
true
in the .csproj file, ANDIsClientLibrary
==true
)No changes:
false
setting - property removed to rely on new defaultVerification
✅ Built Azure.ResourceManager.Advisor (has autorest.md, property=true, IsClientLibrary=true) - Success
✅ Built Azure.AI.Projects (removed explicit false, relies on default, new TypeSpec generator) - Success
✅ Built Azure.AI.Translation.Text (added property, has tsp-location without emitter) - Success
✅ Built Azure.AI.Personalizer (added property, has Generated but no config files) - Success
✅ Built Azure.AI.OpenAI (removed duplicates, has single property) - Success
✅ Built Microsoft.ClientModel.TestFramework (property removed, relies on default false) - Success
✅ Built Azure.Core.TestFramework (property removed, relies on default false) - Success
✅ Built Azure.Security.KeyVault.Administration (removed explicit false, relies on default) - Success
✅ Built Azure.AI.Inference (property added per request) - Success
✅ Built Azure.ResourceManager.AgriculturePlatform (property added to address build failures) - Success
✅ Merged latest changes from main branch without conflicts
✅ Verified condition properly checks both IncludeAutorestDependency and IsClientLibrary
✅ Reverted all changes to Microsoft.Azure.Batch folder
Fixes #53148
Original prompt
Fixes #53148
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.