-
Notifications
You must be signed in to change notification settings - Fork 12
Change to project SDK #99
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
| <PackageReference Include="Microsoft.DotNet.ILCompiler.LLVM" PrivateAssets="None" /> | ||
| <ItemGroup> | ||
| <PackageReference Include="Microsoft.DotNet.ILCompiler.LLVM" IsImplicitlyDefined="true" Version="$(NativeAotLlvmVersion)" /> | ||
| <PackageReference Include="runtime.$(NETCoreSdkPortableRuntimeIdentifier).microsoft.dotnet.ilcompiler.llvm" IsImplicitlyDefined="true" Version="$(NativeAotLlvmVersion)" /> |
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.
IsImplicitlyDefined is needed because of:
componentize-dotnet/Directory.Packages.props
Lines 2 to 4 in b132a2a
| <PropertyGroup> | |
| <ManagePackageVersionsCentrally>true</ManagePackageVersionsCentrally> | |
| </PropertyGroup> |
Otherwise, it is complaining that the version is not managed by PackageVersion.
|
I've changed it to |
| | `WitBindgenVersion` | Version of the `BytecodeAlliance.Componentize.DotNet.WitBindgen` package to use. | Current SDK version | | ||
| | `RegisterExperimentalNuGetSource` | Whether to register the [dotnet-experimental feed](https://dev.azure.com/dnceng/public/_artifacts/feed/dotnet-experimental/NuGet) to find the NativeAOT-LLVM package. Set this to `false` if you have a NuGet.config that already includes this feed. | `true` | | ||
| | `WitBindgenAdditionalArgs` | Additional arguments to pass to `wit-bindgen` when generating C# bindings. Separate multiple arguments with spaces. For example: `--with-wit-results --features tls`. | (empty) | | ||
| | `WitBindgenAddtionalArgs` | Additional arguments to pass to `wit-bindgen` when generating C# bindings. Separate multiple arguments with spaces. For example: `--with-wit-results --features tls`. | (empty) | |
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.
I think there is a typo in property item WitBindgenAddtionalArgs:
componentize-dotnet/src/WitBindgen/build/BytecodeAlliance.Componentize.DotNet.WitBindgen.targets
Line 95 in b132a2a
| <Exec Command="$(WitBindgenExe) c-sharp %(Wit.Identity) %(Wit.WitWorldArg) --runtime $(WitBindgenRuntime) $(WitBindgenAddtionalArgs) --out-dir $(WitGeneratedFilesRoot)" /> |
Shouldn't this be WitBindgenAdditionalArgs?
|
This looks like what I was trying to accomplish awhile back! I need to read through https://learn.microsoft.com/en-us/dotnet/core/project-sdk/overview but upon a quick look it seems like this will work. Are there trade-off of being an SDK version a nuget package? It seems in this case we really are an SDK |
|
The biggest downside that I can find is that NuGet doesn't support updating SDK versions (from NuGet). When a new version comes out, you have to manually update the version. See NuGet/Home#13127 I'm not sure if Dependabot supports notifications for new SDK versions. |
PoC to drop
runtime.$(NETCoreSdkPortableRuntimeIdentifier).microsoft.dotnet.ilcompiler.llvmin the template.The new template is as following:
Important
This would need a minor bump (to 0.8) since it's a breaking change.