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.

Sign up for GitHub

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

Jump to bottom

Enable ConfigurationBinder source gen on components by default #535

Merged
merged 3 commits into from
Oct 27, 2023
Merged

Enable ConfigurationBinder source gen on components by default #535

merged 3 commits into from
Oct 27, 2023

Conversation

eerhardt
Copy link
Member

Also enable IsAotCompatible, so we get warnings when using non-AOT compatible code.

@eerhardt
Enable ConfigurationBinder source gen on all components.
e5aaf76 
Also enable IsAotCompatible, so we get warnings when using non-AOT compatible code.
@eerhardt eerhardt marked this pull request as draft October 26, 2023 20:06
@DamianEdwards
Copy link
Member

Seems good. Is there a reason this is still draft?

@eerhardt
Copy link
Member Author

Seems good. Is there a reason this is still draft?

Yes. Doesn't work yet. I need to explicitly reference the 8.0 config binder package, and then work around issues.

@eerhardt
Make the Source Generator actually work by referencing Microsoft.Exte…
ad1ffb5 
…nsions.Configuration.Binder, which brings in the latest version that contains the source generator.
@eerhardt eerhardt marked this pull request as ready for review October 26, 2023 23:36
@eerhardt eerhardt requested a review from ericstj October 26, 2023 23:36
@eerhardt
Copy link
Member Author

I needed to work around a couple bugs in the Source Generator, which I linked in the code. I was blocked on 3 Azure Components though. I couldn't find a workaround to get it enabled.

eerhardt
eerhardt commented Oct 26, 2023
View reviewed changes
Directory.Packages.props Outdated
@@ -11,6 +11,7 @@
<PackageVersion Include="Azure.Extensions.AspNetCore.Configuration.Secrets" Version="1.2.2" />
<PackageVersion Include="Azure.Identity" Version="1.10.0" />
<PackageVersion Include="Azure.Messaging.ServiceBus" Version="7.15.0" />
<PackageVersion Include="Azure.Messaging.EventHubs" Version="5.9.2" />
Copy link
Member Author

Choose a reason for hiding this comment

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

This is needed until Xabaril/AspNetCore.Diagnostics.HealthChecks#2038 is shipped in a version on nuget. The ServiceBus health check brings in EventHubs, and I needed to add an explicit alias to work around the source generator bug.

@eerhardt
Copy link
Member Author 

  PDB0007: token 0x35000002: An extern alias was not defined for assembly reference 23000002
  PDB0007: token 0x35000002: An extern alias was not defined for assembly reference 23000003
  PDB0007: token 0x35000006: An extern alias was not defined for assembly reference 23000002
  PDB0007: token 0x35000006: An extern alias was not defined for assembly reference 23000003
  PDB0007: token 0x3500000A: An extern alias was not defined for assembly reference 23000002
  PDB0007: token 0x3500000A: An extern alias was not defined for assembly reference 23000003
  PDB0007: token 0x35000002: An extern alias was not defined for assembly reference 23000002
  PDB0007: token 0x35000002: An extern alias was not defined for assembly reference 23000003
  PDB0007: token 0x35000006: An extern alias was not defined for assembly reference 23000002
  PDB0007: token 0x35000006: An extern alias was not defined for assembly reference 23000003
  PDB0007: token 0x35000002: An extern alias was not defined for assembly reference 23000002
  PDB0007: token 0x35000002: An extern alias was not defined for assembly reference 23000003
D:\a\_work\1\s\.packages\microsoft.dotnet.arcade.sdk\8.0.0-beta.23516.4\tools\SymStore.targets(64,5): error MSB3073: The command ""D:\a\_work\1\s\.packages\microsoft.diasymreader.pdb2pdb\1.1.0-beta2-19575-01\tools\Pdb2Pdb.exe" "D:\a\_work\1\s\artifacts\bin\Aspire.Azure.Messaging.ServiceBus\Release\net8.0\Aspire.Azure.Messaging.ServiceBus.dll" /out "D:\a\_work\1\s\artifacts\SymStore\Release\Aspire.Azure.Messaging.ServiceBus\net8.0\Aspire.Azure.Messaging.ServiceBus.pdb" /srcsvrvar SRC_INDEX=public" exited with code -1. [D:\a\_work\1\s\src\Components\Aspire.Azure.Messaging.ServiceBus\Aspire.Azure.Messaging.ServiceBus.csproj]

@ericstj - have you ever seen this error before? It is coming from the 2 Azure Components that I tried the work around by using an extern alias.

@eerhardt
Copy link
Member Author

Also tagging @tmat - have you ever seen the above error coming from Pdb2Pdb.exe?

@eerhardt
Copy link
Member Author

I ended up reverting the Azure Components changes all together. None of them will use the ConfigBinder Source Generator due to the above Pdb2Pdb.exe error. When the underlying issues are fixed in the Source Generator, we will re-enable it for the Azure components.

@eerhardt eerhardt changed the title Enable ConfigurationBinder source gen on all components. Enable ConfigurationBinder source gen on components by default Oct 27, 2023
eerhardt
eerhardt commented Oct 27, 2023
View reviewed changes
src/Components/Aspire.Azure.Data.Tables/AspireTablesExtensions.cs
@@ -76,6 +78,18 @@ private sealed class TableServiceComponent : AzureComponent<AzureDataTablesSetti
}, requiresCredential: false);
}

protected override void BindClientOptionsToConfiguration(IAzureClientBuilder<TableServiceClient, TableClientOptions> clientBuilder, IConfiguration configuration)
{
#pragma warning disable IDE0200 // Remove unnecessary lambda expression - needed so the ConfigBinder Source Generator works
Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe I should just make IDE0200 a suggestion? This seems like a pretty aggressive warning.

@eerhardt eerhardt merged commit b484a8a into main Oct 27, 2023
4 checks passed
@eerhardt eerhardt deleted the UseBinderSG branch October 27, 2023 13:48
@ericstj
Copy link
Member

ericstj commented Oct 27, 2023

Yeah, let's just get the issues fixed in configuration to unblock these scenarios.

eerhardt added a commit that referenced this pull request Oct 27, 2023
@eerhardt
Enable ConfigurationBinder source gen on components by default (#535)
18c7e77 
* Enable ConfigurationBinder source gen on all components.

Also enable IsAotCompatible, so we get warnings when using non-AOT compatible code.

* Make the Source Generator actually work by referencing Microsoft.Extensions.Configuration.Binder, which brings in the latest version that contains the source generator.

* Revert Azure components due to Pdb2Pdb error for extern alias.
@danmoseley danmoseley added the area-integrations Issues pertaining to Aspire Integrations packages label Nov 16, 2023
eerhardt added a commit to eerhardt/aspire that referenced this pull request Jan 10, 2024
@eerhardt
Update to latest Configuration Binder source generator
fca6c31 
Now that 8.0.1 has shipped, we can remove the workarounds added in dotnet#535.
@eerhardt eerhardt mentioned this pull request Jan 10, 2024
Merged
eerhardt added a commit that referenced this pull request Jan 11, 2024
@eerhardt
Update to latest Configuration Binder source generator (#1616)
b83408f 
Now that 8.0.1 has shipped, we can remove the workarounds added in #535.
@github-actions github-actions bot locked and limited conversation to collaborators Apr 30, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@davidfowl davidfowl Awaiting requested review from davidfowl

@DamianEdwards DamianEdwards Awaiting requested review from DamianEdwards

@ericstj ericstj Awaiting requested review from ericstj

Assignees
No one assigned
Labels
area-integrations Issues pertaining to Aspire Integrations packages
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@eerhardt @DamianEdwards @ericstj @danmoseley