Skip to content

Avoid executing source generators outside of an Azure Functions project#2119

Merged
kshyju merged 6 commits intomainfrom
shkr/fix-exten-startup-gen
Dec 4, 2023
Merged

Avoid executing source generators outside of an Azure Functions project#2119
kshyju merged 6 commits intomainfrom
shkr/fix-exten-startup-gen

Conversation

@kshyju
Copy link
Copy Markdown
Member

@kshyju kshyju commented Dec 2, 2023

resolves #2114

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 PR information

@kshyju kshyju requested a review from fabiocav December 2, 2023 21:46
@jviau
Copy link
Copy Markdown
Contributor

jviau commented Dec 4, 2023

Reading the linked issue, the root cause is that the SDK package ref is transitively included in the xunit project, so it too includes all those targets. Disabling the source generators this way mitigates a piece of the problem, but it is larger than that. The better fix is to just not include that package transitively. This can be done via:

  1. Ask customers to add PrivateAssets="all" to their SDK package ref (we can also update templates to include this)
  2. Bigger change - we can look into using an actual SDK msbuild element. This would let us get our targets in and also avoid it being included transitively. However, there are other consequences of this we would need to explore (not to mention a complete shift in how customers reference our SDK).

So, I am not sold this is absolutely necessary because even with this I would still advise customers to perform option 1. This won't cause any harm including it, but it isn't right to say the root problem is solved with this change (there are still other SDK targets that could cause issues being brought in).

@fabiocav
Copy link
Copy Markdown
Member

fabiocav commented Dec 4, 2023

Reading the linked issue, the root cause is that the SDK package ref is transitively included in the xunit project, so it too includes all those targets. Disabling the source generators this way mitigates a piece of the problem, but it is larger than that. The better fix is to just not include that package transitively. This can be done via:

  1. Ask customers to add PrivateAssets="all" to their SDK package ref (we can also update templates to include this)
  2. Bigger change - we can look into using an actual SDK msbuild element. This would let us get our targets in and also avoid it being included transitively. However, there are other consequences of this we would need to explore (not to mention a complete shift in how customers reference our SDK).

So, I am not sold this is absolutely necessary because even with this I would still advise customers to perform option 1. This won't cause any harm including it, but it isn't right to say the root problem is solved with this change (there are still other SDK targets that could cause issues being brought in).

While I agree with the above, the fact that this update introduces an issue that will lead to confusion and require a project change leads to a poor experience, so we should avoid that. Whether the approach is what's scoped here (which I agree, shouldn't cause problems) or something else, that's less important, but we need to ensure an SDK reference update within the same major won't break users' builds.

Comment thread sdk/Sdk.Generators/Constants.cs
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.

Broken WorkerExtensionStartupCodeExecutor generation when FunctionsApp referenced by XUnit test project.

3 participants