Skip to content

Generate correct value for defaultWorkerPath in worker.config when AOT publishing is enabled#1980

Merged
kshyju merged 4 commits intomainfrom
shkr/1053-aot-worker-executable-name
Nov 16, 2023
Merged

Generate correct value for defaultWorkerPath in worker.config when AOT publishing is enabled#1980
kshyju merged 4 commits intomainfrom
shkr/1053-aot-worker-executable-name

Conversation

@kshyju
Copy link
Copy Markdown
Member

@kshyju kshyju commented Oct 23, 2023

resolves #1053

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)

@kshyju kshyju requested review from brettsam, fabiocav and satvu October 23, 2023 14:50
Comment thread sdk/Sdk/Targets/Microsoft.Azure.Functions.Worker.Sdk.targets Outdated
Copy link
Copy Markdown
Contributor

@jviau jviau left a comment

Choose a reason for hiding this comment

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

See comment for using $(TargetName)$(TargetExt)

Comment thread sdk/Sdk/Targets/Microsoft.Azure.Functions.Worker.Sdk.targets Outdated
@kshyju
Copy link
Copy Markdown
Member Author

kshyju commented Oct 24, 2023

See comment for using $(TargetName)$(TargetExt)

Yea. Switched to the variable in the latest iteration. Thank you!

@kshyju kshyju requested a review from jviau October 24, 2023 00:43
Comment thread sdk/Sdk/Targets/Microsoft.Azure.Functions.Worker.Sdk.targets Outdated
Copy link
Copy Markdown
Contributor

@jviau jviau left a comment

Choose a reason for hiding this comment

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

I won't block this PR on this, but I think the best approach would be to do something like:

    <PropertyGroup>
      <_FunctionsTarget>dotnet $(TargetName)$(TargetExt)</_FunctionsTarget>
      <_FunctionsTarget Condition="('$(SelfContained)' == 'true' OR '$(PublishAot)' == 'true') AND '%(Identity)' == '$(AppHostIntermediatePath)'">%(None.Link)</_FunctionsTarget>
    </PropertyGroup>

With that, we can then consolidate all of the different ways to produce worker.config with just a single replacement via _FunctionsTarget

@jviau jviau self-requested a review October 24, 2023 21:30
@fabiocav fabiocav closed this Oct 25, 2023
@fabiocav fabiocav reopened this Oct 25, 2023
Copy link
Copy Markdown
Contributor

@jviau jviau left a comment

Choose a reason for hiding this comment

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

Approving this if it works for your use case. I will re-create this behavior in the SDK refactor PR to keep this scenario working (as the changes there will be beyond a simple merge).

@kshyju kshyju force-pushed the shkr/1053-aot-worker-executable-name branch from 4e6fbf9 to 1a18653 Compare November 14, 2023 18:43
@kshyju kshyju merged commit 31f3a8b into main Nov 16, 2023
@kshyju kshyju deleted the shkr/1053-aot-worker-executable-name branch November 16, 2023 00:35
@jviau jviau mentioned this pull request Dec 4, 2023
7 tasks
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.

[AOT] Update worker.config generation to have correct executable name

4 participants