Skip to content
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

[wasm] Check if python is installed when installing the workload #54638

Closed
lewing opened this issue Jun 23, 2021 · 7 comments
Closed

[wasm] Check if python is installed when installing the workload #54638

lewing opened this issue Jun 23, 2021 · 7 comments
Assignees
Labels
arch-wasm WebAssembly architecture area-Build-mono
Milestone

Comments

@lewing
Copy link
Member

lewing commented Jun 23, 2021

Right now the build expects python to be in the path on linux but ends up checking this by running emcc --version and not surfacing the error text.

@lewing lewing added arch-wasm WebAssembly architecture area-Build-mono labels Jun 23, 2021
@lewing lewing added this to the 6.0.0 milestone Jun 23, 2021
@ghost
Copy link

ghost commented Jun 23, 2021

Tagging subscribers to 'arch-wasm': @lewing
See info in area-owners.md if you want to be subscribed.

Issue Details

Right now the build expects python to be in the path on linux but ends up checking this by running emcc --version and not surfacing the error text.

Author: lewing
Assignees: radical
Labels:

arch-wasm, area-Build-mono

Milestone: 6.0.0

@dotnet-issue-labeler dotnet-issue-labeler bot added the untriaged New issue has not been triaged by the area owner label Jun 23, 2021
@lewing
Copy link
Member Author

lewing commented Jun 23, 2021

Likely the cause of #54342

@lewing lewing removed the untriaged New issue has not been triaged by the area owner label Jun 23, 2021
@lewing lewing modified the milestones: 6.0.0, 7.0.0 Aug 17, 2021
@lewing lewing modified the milestones: 7.0.0, 6.0.x Dec 2, 2021
@radical
Copy link
Member

radical commented Dec 2, 2021

  1. [wasm] Build improvements #61276 surfaces the output of emcc --version correctly, so the error would show up for the user
  2. But we should also check for python when installing the workload

@radical radical changed the title [wasm] Add descriptive error when python is not in path on linux [wasm] Check if python is installed when installing the workload May 27, 2022
@radical radical removed this from the 6.0.x milestone May 27, 2022
@ghost ghost added the untriaged New issue has not been triaged by the area owner label May 27, 2022
@radical radical added this to the 7.0.0 milestone May 27, 2022
@ghost ghost removed the untriaged New issue has not been triaged by the area owner label May 27, 2022
@radical
Copy link
Member

radical commented Jun 16, 2022

But we should also check for python when installing the workload

This needs to be done on the sdk side, so closing this in favor of - dotnet/sdk#26050 .

@radical radical closed this as completed Jun 16, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Jul 16, 2022
@lewing lewing reopened this Jul 27, 2022
@radical
Copy link
Member

radical commented Jul 27, 2022

Doing it in WorkloadManifest.targets would mean that we do it for every build. Though maybe we can add some sentinel file in obj, so we don't have to do that more than once for incremental builds.

@lewing
Copy link
Member Author

lewing commented Jul 27, 2022

I was thinking something simple like <OnError ExecuteTargets="SanityCheckEmsdk"/>

@radical
Copy link
Member

radical commented Aug 7, 2022

I was thinking something simple like <OnError ExecuteTargets="SanityCheckEmsdk"/>

That we essentially do:

<Exec Command="$(_EmccVersionCommand)" WorkingDirectory="$(_WasmIntermediateOutputPath)" EnvironmentVariables="@(EmscriptenEnvVars)" ConsoleToMsBuild="true" StandardOutputImportance="Low" IgnoreExitCode="true">
<Output TaskParameter="ConsoleOutput" ItemName="_VersionLines" />
<Output TaskParameter="ExitCode" PropertyName="_EmccVersionExitCode" />
</Exec>
<!-- If `emcc -version` failed, then run it again, so we can surface the output as *Errors*. This allows the errors to show up correctly,
versus trying to use the output lines with the Error task -->
<Exec Condition="$(_EmccVersionExitCode) != '0'"
Command="$(_EmccVersionCommand)"
WorkingDirectory="$(_WasmIntermediateOutputPath)"
EnvironmentVariables="@(EmscriptenEnvVars)"
CustomErrorRegularExpression=".*"
/>

First we run emcc --version, and if that fails then we run it again but surface the output to the user as errors.

This will show emcc --version failing due to missing python:

emcc --version
unable to find python in $PATH

So, if we are not adding a check at workload install time, then I think there is nothing else to be done here. But please re-open if you don't agree.

@radical radical closed this as completed Aug 7, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
arch-wasm WebAssembly architecture area-Build-mono
Projects
None yet
Development

No branches or pull requests

2 participants