Skip to content

Refactor package structure for Azure Functions usage #31

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

Closed
wants to merge 3 commits into from

Conversation

TylerLeonhardt
Copy link
Member

@TylerLeonhardt TylerLeonhardt commented Sep 6, 2018

This puts the worker.config.json and the Modules folder under the publish/workers/powershell directory.

The actual powershell worker dlls and dependency dlls will be dropped in the publish directory. I worked with @pragnagopa on this.

This means that all the core-tools has to do is reference the PowerShell Worker nuget package and everything works as expected.

This PR also addresses #18 by renaming everything from Azure.Functions.PowerShell.Worker to Microsoft.Azure.Functions.PowerShellWorker

@TylerLeonhardt
Copy link
Member Author

I just realize that this change means I need to change the README

daxian-dbw
daxian-dbw previously approved these changes Sep 7, 2018
Copy link
Contributor

@daxian-dbw daxian-dbw left a comment

Choose a reason for hiding this comment

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

I sign off on this PR. However, having worker.dll and its dependency assemblies in publish while having worker.config.json and Modules in publish\powershell\worker feels hacky, and it makes the local testing harder -- consider copying the publish folder directly to azure-functions-core-tools\bin\workers\powershell currently vs. copying parts of the artifacts to azure-functions-core-tools\bin and parts to azure-functions-core-tools\bin\worker\powershell.

If we cannot get the assemblies copied to publish\worker\powershell by generating the nupkg along with dotnet publish, then we probably should consider doing the nuget pack in a different step, treating all the artifacts produced by dotnet publish as the content of the nuget package, so that all powershell worker related files can be placed together under worker\powershell. It doesn't hurt to have a build script to chain dotnet publish and dotnet pack.

On a second thought, we probably have to do the packaing in a different step anyway, because we need to sign the assemblies first, and the generated nupkg should contain the signed assemblies. By generating the nupkg along with dotnet publish, the nupkg contains the unsigned assemblies.

@TylerLeonhardt
Copy link
Member Author

I can change it so that you do a nuget pack after a nuget publish but the biggest problem still remains.

It's not possible to have dlls and dependencies placed in another location than the publish dir. We would have to redistribute all dlls as content Files... Including the dependencies of the worker.

@daxian-dbw
Copy link
Contributor

daxian-dbw commented Sep 7, 2018

We would have to redistribute all dlls as content Files... Including the dependencies of the worker.

Yes, that's what I'm suggesting -- we treat all files under publish dir as content files.
This sole purpose of this nupkg is for the azure function host to consume. The Microsoft.Azure.Functions.PowreShellWorker.dll is actually an EXE. It won't be consumed by anything else as a library.

@TylerLeonhardt
Copy link
Member Author

I'm fine with doing what you're mentioning, It's just kinda abusing NuGet though. At that point, we might as well grab a release from Github releases.

I've kinda had different conversations about this with @pragnagopa and @SteveL-MSFT. I'd appreciate your input here.

I'm fine with redistributing everything and at least that follows the Java worker at the moment.

@pragnagopa
Copy link
Member

The Microsoft.Azure.Functions.PowreShellWorker.dll is actually an EXE. It won't be consumed by
anything else as a library.

@daxian-dbw has a good point. Shipping it as exe makes more sense. If you guys agree, I can look into functions host on adding support for running a language worker as exe

@daxian-dbw
Copy link
Contributor

daxian-dbw commented Sep 7, 2018

@tylerl0706 I wouldn't say it's abusing nuget package -- that's how Install-Package works. PowerShell release daily builds as nuget packages to powershell-core-daily feed on myget.org, and they are consumed by install-package, see https://github.com/PowerShell/PowerShell/blob/master/tools/install-powershell.ps1#L108-L127 in our Install-PowerShell.ps1.

@pragnagopa I don't think we should ship an EXE directly, because that means we create a self-contained application including all .NET Core framework assemblies, which will increate the package size greatly. We still want to keep the powershell worker a Fx dependent application, which means it's a dll, but essentially acts like an EXE with dotnet <worker-name>.dll. In this way, we can share the dotnet core runtime with the Azure Function Host on box and shrink the package size a lot.

@TylerLeonhardt
Copy link
Member Author

TylerLeonhardt commented Sep 7, 2018

Ok so now, there's a new folder called package at the root of the directory. It has a nuspec and a csproj in it. The only reason the csproj is there is because dotnet pack only works with csprojs

Here are the steps you would do:

cd azure-functions-powershell-worker
dotnet publish
cd package
dotnet pack

that will give you a nupkg in:

azure-functions-powershell-worker/package/bin/Debug

It pulls the contents of the publish folder in:

azure-functions-powershell-worker/src/bin/Debug/netcoreapp2.1/publish

if you specify a different Configuration or TargetFramework that will be honored.

@daxian-dbw
Copy link
Contributor

daxian-dbw commented Sep 7, 2018

Can we use a separate PR for the packaging changes? The existing changes in this PR are mainly renaming, and it can be merged already.

@TylerLeonhardt
Copy link
Member Author

:(

@TylerLeonhardt
Copy link
Member Author

@daxian-dbw I sent a rename PR out here:
#35

When that goes in, I'll rebase that to this.

@daxian-dbw
Copy link
Contributor

@tylerl0706 I know that requests extra work 😄 but it makes the PR more scoped, and will also make it easier to review the packing changes you are making.

You can git stash save the second and third commits locally. Once the first commit gets merged, you can git stash pop to get back your changes.

@daxian-dbw daxian-dbw dismissed their stale review September 7, 2018 18:42

New changes were pushed

@TylerLeonhardt
Copy link
Member Author

closing in favor of #36

@TylerLeonhardt TylerLeonhardt deleted the more-nuget branch September 7, 2018 19:13
davidmrdavid added a commit that referenced this pull request Apr 7, 2022
# This is the 1st commit message:

separate DF SDK classes from DF worker classes

# This is the commit message #2:

fix typo

# This is the commit message #3:

DurableSDK now compiles by itself

# This is the commit message #4:

Allow ExternalSDK to handle orchestration

# This is the commit message #5:

document next steps

# This is the commit message #6:

allow external SDK to set the user-code's input. Still need to refactor this logic for the worker to continue working with old SDK

# This is the commit message #7:

add import module

# This is the commit message #8:

supress traces

# This is the commit message #9:

avoid nullptr

# This is the commit message #10:

pass tests

# This is the commit message #11:

fix E2E tests

# This is the commit message #12:

develop E2E tests

# This is the commit message #13:

Enabled external durable client (#765)

Co-authored-by: Michael Peng <[email protected]>
# This is the commit message #14:

bindings work

# This is the commit message #15:

conditional binding intialization

# This is the commit message #16:

conditional import

# This is the commit message #17:

Added exception handling logic

# This is the commit message #18:

Revert durableController name to durableFunctionsUtils

# This is the commit message #19:

Ensure unit tests are functioning properly

# This is the commit message #20:

Corrected unit test names

# This is the commit message #21:

Turned repeated variables in unit tests into static members

# This is the commit message #22:

Fixed issue with building the worker

# This is the commit message #23:

Fix E2E test

# This is the commit message #24:

Fixed unit test setup

# This is the commit message #25:

Fixed another unit test setup

# This is the commit message #26:

Remove string representation of booleans

# This is the commit message #27:

patch e2e test
# This is the commit message #28:

remove typo in toString
# This is the commit message #29:

Update PowerShell language worker pipelines (#750)

* Install .Net to a global location

* Remove .Net installation tasks

* Update install .Net 6 task

* Update Windows image to use windows-latest
# This is the commit message #30:

Make throughput warning message visible for tooling diagnosis (#757)


# This is the commit message #31:

Update grpc.tools to version 2.43.0

# This is the commit message #32:

Update Google.Protobuf.Tools to version 3.19.4

# This is the commit message #33:

Revert "Update Google.Protobuf.Tools to version 3.19.4"

This reverts commit bcbd022.

# This is the commit message #34:

Revert "Update grpc.tools to version 2.43.0"

This reverts commit ccb323a.

# This is the commit message #35:

Update Google.Protobuf to 3.19.4 and grpc.tools to  2.43.0 (#762)

* Update grpc.tools to version 2.43.0

* Update Google.Protobuf.Tools to version 3.19.4
# This is the commit message #36:

Switch from Grpc.Core to Grpc.Net.Client (#758)

* Upgraded protobuf versions and removed Grpc.Core dependency

* Updated channel and option types used

* Change channel credentials

* Added http prefix to url

* Add valid URL check and explicitly include credentials
# This is the commit message #37:

Update pipeline logic to generate the SBOM for release builds (#767)
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