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

Create Sdk.props in AOT compilers with a template #53685

Merged
merged 3 commits into from
Jun 7, 2021

Conversation

directhex
Copy link
Contributor

Fixes #53653

Example:

sudo cat Sdk/Sdk.props
<Project>
  <ItemGroup>
    <MonoAotCrossCompilerPath Include="$(MSBuildThisFileDirectory)..\tools\mono-aot-cross" RuntimeIdentifier="iossimulator-arm64" />
  </ItemGroup>
</Project>

Fixes dotnet#53653

Example:

```
sudo cat Sdk/Sdk.props
<Project>
  <ItemGroup>
    <MonoAotCrossCompilerPath Include="$(MSBuildThisFileDirectory)..\tools\mono-aot-cross" RuntimeIdentifier="iossimulator-arm64" />
  </ItemGroup>
</Project>
```
@directhex
Copy link
Contributor Author

runtime-staging is green, dunno why it's stuck on yellow

@radical
Copy link
Member

radical commented Jun 3, 2021

We should replace the uses of $(MonoAotCrossCompilerPath) in dotnet/runtime with the item, to be consistent.

And can we rename it to something that would suggest that it's not a property? I can't think of a good name though - maybe @(MonoAotCrossCompiler) ? :/

@directhex
Copy link
Contributor Author

@radical where are we actually consuming the property from an Sdk.props, within the runtime repo? I've got a rune to select the right path from the items (@(MonoAotCrossCompiler->WithMetadataValue('RuntimeIdentifier','browser-wasm'))), but for the most part it seems we're writing the property ourselves, in multiple places (eng/testing/tests.targets, src/libraries/sendtohelixhelp.proj, src/mono/Directory.Build.props, src/mono/wasm/build/WasmApp.LocalBuild.props)

@radical
Copy link
Member

radical commented Jun 3, 2021

where are we actually consuming the property from an Sdk.props, within the runtime repo?

we don't.

but for the most part it seems we're writing the property ourselves, in multiple places

Yep! Um so, in the repo we use compiler from the artifacts directly, so Sdk.props wouldn't come into play (till we get some local workload based testing). I'm suggesting to follow the same item+metadata thing, in all these places where we are writing+using it ad-hoc.

@directhex
Copy link
Contributor Author

@radical ok, understood, wanted to make sure I was on the same page.

That sounds like an exciting and awesome job for TomorrowMan.

@steveisok
Copy link
Member

We should replace the uses of $(MonoAotCrossCompilerPath) in dotnet/runtime with the item, to be consistent.

And can we rename it to something that would suggest that it's not a property? I can't think of a good name though - maybe @(MonoAotCrossCompiler) ? :/

I think that can be done in a follow up PR.

@directhex can you run an official build to make sure we're ok there?

@radical
Copy link
Member

radical commented Jun 4, 2021

You will at least need to update the wasm targets in src/mono/wasm/build, or that will break building with workloads. And try this out as a workload pack.

@steveisok
Copy link
Member

You will at least need to update the wasm targets in src/mono/wasm/build, or that will break building with workloads. And try this out as a workload pack.

You're right. Since iOS and Android are the ones that have > 1 AOT compiller, can we isolate this change to them and follow up with the other changes (wasm/workload and intree prop/item refactoring)?

@directhex
Copy link
Contributor Author

@steveisok I think it should all go into the one PR. It'll just be confusing if it's not unified.

@directhex directhex requested a review from marek-safar as a code owner June 4, 2021 12:52
@directhex
Copy link
Contributor Author

@radical any further concerns?

@directhex directhex merged commit ccec848 into dotnet:main Jun 7, 2021
@lambdageek
Copy link
Member

Attn: @rolfbjarne iOS workloads will need to change uses of $(MonoAotCrossCompilerPath) to selecting from the @(MonoAotCrossCompiler) item:

https://github.com/xamarin/xamarin-macios/blob/8ca5d5ea5c062093220cf289f1992edbcb45dcec/dotnet/targets/Xamarin.Shared.Sdk.targets#L463

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Change MonoAotCrossCompiler property into ItemGroup in installer project
4 participants