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

Remove bash dependency from init-compiler #77304

Merged
merged 12 commits into from
Dec 21, 2022

Conversation

am11
Copy link
Member

@am11 am11 commented Oct 21, 2022

Also fixes shell check warnings in System.Globalization.Native/local_build.sh.

Upstream PR: dotnet/arcade#11359

Also fixes shell check warnings in
`System.Globalization.Native/local_build.sh`.
@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Oct 21, 2022
@ghost
Copy link

ghost commented Oct 21, 2022

Tagging subscribers to this area: @dotnet/area-system-globalization
See info in area-owners.md if you want to be subscribed.

Issue Details

Also fixes shell check warnings in System.Globalization.Native/local_build.sh.

Upstream PR: dotnet/arcade#11359

Author: am11
Assignees: -
Labels:

area-System.Globalization, community-contribution

Milestone: -

@ghost
Copy link

ghost commented Oct 21, 2022

Tagging subscribers to this area: @dotnet/runtime-infrastructure
See info in area-owners.md if you want to be subscribed.

Issue Details

Also fixes shell check warnings in System.Globalization.Native/local_build.sh.

Upstream PR: dotnet/arcade#11359

Author: am11
Assignees: -
Labels:

area-Infrastructure, community-contribution

Milestone: -

@am11 am11 force-pushed the feature/shell-scripts branch from 3ed05b6 to 16cc538 Compare October 21, 2022 11:03
@am11 am11 force-pushed the feature/shell-scripts branch from 16cc538 to 0d8d146 Compare October 21, 2022 12:37
@am11 am11 marked this pull request as ready for review October 21, 2022 13:50
@am11 am11 requested a review from marek-safar as a code owner October 21, 2022 13:50
@am11
Copy link
Member Author

am11 commented Oct 21, 2022

cc @jkotas, @akoeplinger
This is in preparation for including init-compiler.sh to NativeAOT package: #77245 (comment). Some OS (e.g. Alpine Linux) don't have bash preinstalled, and we aren't using many bash features besides two usages of bash arrays, so it's simple to replace them and drop the dependency from this standalone script.

@jkotas
Copy link
Member

jkotas commented Oct 21, 2022

This is in preparation for including init-compiler.sh to NativeAOT package

I am not sure whether it is a good idea to include init-compiler.sh in the NativeAOT package. I see this script as specific to the .NET runtime internal build infrastructure setup. It is why it lives in Arcade repo. As a rule, bits that live in Arcade repo should be only used by .NET runtime internal build infrastructure. They should not ship as part of the product.

The current native AOT plan is: You have to have version-less tools installed. If you do not have version-less tools installed, you have to set CppCompilerAndLinker to point to the tools that you want to use.

@am11
Copy link
Member Author

am11 commented Oct 21, 2022

As a rule, bits that live in Arcade repo should be only used by .NET runtime internal build infrastructure.

We moved this script to arcade to avoid duplicating the code: #59018 (it is already included in Microsoft.DotNet.CMake.Sdk).

This is a freestanding script to locate compiler on the system. It works for all supported build environments (it does not look for prerequisite beyond a few compiler toolchain components, mainly the driver; clang or gcc).

you have to set CppCompilerAndLinker to point to the tools that you want to use.

I was experimenting (for a follow up PR) with keeping that experience. If user specified e.g. CppCompilerAndLinker=clang-15 or there is clang or gcc found in PATH, then we will not invoke the script. Without setting CppCompilerAndLinker or when there is no clang or gcc in PATH, we will invoke this script to find the best available versioned compiler found in PATH (descending order of their versions).

@jkotas
Copy link
Member

jkotas commented Oct 21, 2022

it is already included in Microsoft.DotNet.CMake.Sdk

This nuget package is not a shipping nuget package. It is not on nuget.org. It is for internal .NET build infrastructure use only.

@am11
Copy link
Member Author

am11 commented Oct 21, 2022

This nuget package is not a shipping nuget package. It is not on nuget.org. It is for internal .NET build infrastructure use only.

Right, my point was the only reason it's moved to arcade was to avoid maintaining two copies of same script, that locates versioned compiler in PATH.

@akoeplinger
Copy link
Member

It would still mean we're essentially signing up for shipping init-compiler.sh officially (even if it's just for this opt-in static ICU linking feature from local_build.sh).

That said, maybe that's the right choice given we need to hardcode clang version numbers which are specific to our build environment e.g. like this today:

<CppCompilerAndLinker Condition="'$(TargetArchitecture)' != '$(BuildArchitecture)' and '$(HostOS)' == 'linux' and '$(RuntimeIdentifier)' == 'linux-musl-arm64'">clang-15</CppCompilerAndLinker>
<CppCompilerAndLinker Condition="'$(TargetArchitecture)' != '$(BuildArchitecture)' and '$(HostOS)' == 'linux' and '$(RuntimeIdentifier)' != 'linux-musl-arm64'">clang-9</CppCompilerAndLinker>

@jkotas
Copy link
Member

jkotas commented Oct 25, 2022

That said, maybe that's the right choice given we need to hardcode clang version numbers which are specific to our build environment e.g. like this today:

We can call init-compiler.sh from our build environment to fix these hardcoded version numbers. It is not necessary to ship init-compiler.sh for that.

@am11 am11 force-pushed the feature/shell-scripts branch from 0d1d568 to 78769a8 Compare October 25, 2022 21:07
@build-analysis build-analysis bot mentioned this pull request Oct 26, 2022
2 tasks
@am11 am11 closed this Oct 26, 2022
@am11 am11 reopened this Oct 26, 2022
@am11
Copy link
Member Author

am11 commented Oct 26, 2022

Removed hard-coded versions in favor of similar target used in mono and DNNE test projs.

Condition="'$(TestNativeAot)' == 'true' and '$(HostOS)' != 'windows'"
BeforeTargets="SetupOSSpecificProps">
<PropertyGroup>
<CppCompilerAndLinker Condition="'$(CppCompilerAndLinker)' == ''">clang</CppCompilerAndLinker>
Copy link
Member

Choose a reason for hiding this comment

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

looking at the whole PR again, maybe it'd be better to not use this property name at all and go with what we use in the NativeExports.csproj:

    <PropertyGroup>
      <NativeCompiler>$(Compiler)</NativeCompiler>
      <NativeCompiler Condition="'$(NativeCompiler)' == ''">clang</NativeCompiler>
    </PropertyGroup>

that'd also support overriding via the $(Compiler) property.

Copy link
Member Author

Choose a reason for hiding this comment

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

Compiler == CppCompilerAndLinker here, see eng/build.sh

Copy link
Member

Choose a reason for hiding this comment

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

omg, so many abstractions 😄

Copy link
Member Author

Choose a reason for hiding this comment

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

:)

I am all for replacing CppCompilerAndLinker and CppLinker in favor of NativeCompiler as property name, but I think some consumers are relying on CppCompilerAndLinker.

@am11
Copy link
Member Author

am11 commented Dec 20, 2022

Failures are unrelated and known: #79843 (mono llvmfullaot), #79439 (win-x64) and #79569 (browser-wasm).

@akoeplinger akoeplinger merged commit 5fdc188 into dotnet:main Dec 21, 2022
directhex added a commit to directhex/runtime that referenced this pull request Jan 3, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Jan 20, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-Infrastructure community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants