Skip to content

Make the Python pack optional on Linux #31761

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

Merged
merged 1 commit into from
Apr 14, 2021
Merged

Make the Python pack optional on Linux #31761

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion src/Components/WebAssembly/BlazorManifest/acquire/Program.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,7 +113,11 @@ static void Move(string source, string destination)
throw new NotSupportedException("Unsupported OS platform.");
}
}
packs.Add((packageName, item.Value.Version));

if (!string.IsNullOrEmpty(packageName))
{
packs.Add((packageName, item.Value.Version));
}
}

return packs;
Expand Down
2 changes: 1 addition & 1 deletion src/Components/WebAssembly/BlazorManifest/src/WorkloadManifest.json.in
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@
"alias-to": {
"win-x86": "Microsoft.NET.Runtime.Emscripten.2.0.12.Python.win-x86",
"win-x64": "Microsoft.NET.Runtime.Emscripten.2.0.12.Python.win-x64",
"linux-x64": "Microsoft.NET.Runtime.Emscripten.2.0.12.Python.linux-x64",
"linux-x64": null,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dsplaisted / @mhutch we have this weird case where a workload pack is optional specifically on Linux. Importing the SDK based on the OS seems easy. However, I was trying to figure out what it would look like for the workload install tool. We want to signal to it, that there truly does not exist a version of this pack for this platform so ignore this entry.

Using a null appears to do the job. Thoughts on this? Other options I should consider?

Copy link
Member

Choose a reason for hiding this comment

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

The idea with the null is to distinguish an unneeded pack from a completely unsupported arch?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. I was trying to extrapolate what the workload installer would do. Right now, the assumption is that in the absence of a alias for a platform, the SDK falls back to using the default ID: https://github.com/dotnet/sdk/blob/cf1db25c688a37cfad7adbfb608f1db930eb1412/src/Resolvers/Microsoft.NET.Sdk.WorkloadManifestReader/WorkloadResolver.cs#L143. Having an explicit null value would let you differentiate between, this pack does not exist for that platform vs use the "default" pack for this platform.

Copy link
Member

Choose a reason for hiding this comment

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

I don't think we meant to support null here.

Is this pack optional on Linux or does it not exist at all? If it's optional (and exists), then you wouldn't want to set this to null as there wouldn't be any way to use it if it was installed.

My understanding is that the pack is optional on Windows too, and that you would opt in to using it by setting a property in the project. If that's right and applies here then I don't think Linux would be any different than Windows (ie they'd both alias to the appropriate pack).

Copy link
Member

@lewing lewing Apr 15, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

The python pack doesn't currently exist at all on linux because upstream emsdk doesn't produce it, whereas it does for window and macos. @akoeplinger can add details.

Copy link
Member

Choose a reason for hiding this comment

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

@mhutch dotnet/sdk#16746 seems to have broken this workaround, what should we be doing here?

"osx-x64": "Microsoft.NET.Runtime.Emscripten.2.0.12.Python.osx-x64"
}
},
Expand Down
2 changes: 1 addition & 1 deletion src/Components/WebAssembly/BlazorManifest/src/WorkloadManifest.targets
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
<Import Project="Sdk.props" Sdk="Microsoft.NET.Runtime.MonoAOTCompiler.Task" />
<Import Project="Sdk.targets" Sdk="Microsoft.NET.Runtime.WebAssembly.Sdk" />
<Import Project="Sdk.props" Sdk="Microsoft.Netcore.App.Runtime.Aot.Cross.browser-wasm" />
<Import Project="Sdk.props" Sdk="Microsoft.NET.Runtime.Emscripten.Python" />
<Import Project="Sdk.props" Sdk="Microsoft.NET.Runtime.Emscripten.Python" Condition="!$([MSBuild]::IsOsPlatform('Linux'))" />
<Import Project="Sdk.props" Sdk="Microsoft.NET.Runtime.Emscripten.Node" />
<Import Project="Sdk.props" Sdk="Microsoft.NET.Runtime.Emscripten.Sdk" />
</ImportGroup>
Expand Down