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

Calculate version range in init-compiler.sh script #105129

Closed
wants to merge 4 commits into from
Closed

Conversation

am11
Copy link
Member

@am11 am11 commented Jul 19, 2024

Guesstimations based on https://releases.llvm.org/ and https://gcc.gnu.org/releases.html, which is close enough.
Testing CI, shall upstream once green.

@dotnet-issue-labeler dotnet-issue-labeler bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Jul 19, 2024
@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Jul 19, 2024
@am11 am11 added area-Infrastructure and removed needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels Jul 19, 2024
Copy link
Contributor

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

@am11 am11 marked this pull request as ready for review July 19, 2024 09:19
@am11
Copy link
Member Author

am11 commented Jul 19, 2024

cc @janvorli, upstream PR dotnet/arcade#14963. This will allow us to switch to latest version in a periodic manner (without needing to update this file).

@am11 am11 force-pushed the patch-6 branch 2 times, most recently from b40ed6c to da072ea Compare July 19, 2024 09:40
@am11 am11 force-pushed the patch-6 branch 2 times, most recently from 84069bf to 2426daa Compare July 19, 2024 11:24
@akoeplinger
Copy link
Member

Clever, but one downside is that we'd automatically opt-in to newer versions even in servicing branches.

@am11
Copy link
Member Author

am11 commented Jul 19, 2024

one downside is that we'd automatically opt-in to newer versions even in servicing branches.

When someone will update the container reference in servicing branch (all linux legs are using containers these days), at that time it will show up red with the compiler error. This is same as what we do today. The only difference with this change is that we won't need to update this file in addition to backporting the compilation fixes.

Containers only have one toolchain version installed at a time.

@janvorli
Copy link
Member

I am also worried about a new version automatically. I don't see a big problem in the case when the build fails. The problematic case is when the new compiler has some subtle bug that causes random or rare runtime failures. We have had this kind of problem recently on Windows with MSVC where it was generating instructions that were not supported on some of the intel CPUs that are otherwise supported. Due to those we have decided to stay on a fixed version of the compiler until the new one is fully tested. I'd prefer not getting into this problem on Linux too, especially not with servicing branches.

@am11
Copy link
Member Author

am11 commented Jul 19, 2024

@janvorli can you please explain how that is even possible with dotnet-buildtools-prereqs-docker flow? If we think of how all the piece are flowing, it is not possible to pick up the unintended version. We are using these images: https://github.com/dotnet/runtime/blob/5fd965d7bf0196fad051a9cc1e8ebe093ed94fa8/docs/workflow/building/coreclr/linux-instructions.md#docker-images all of them are azurelinux 3.0 based (or the old ones with CBL Mariner). Both Azure Linux and CBL mariner images are skipping this branch because we have our "custom" clang without suffix. The suffix version is only relavant for distro source-builds and local environments. Therefore, servicing branches won't be affected.

done

if [ -z "$majorVersion" ]; then
if ! command -v "$compiler" > /dev/null; then
echo "Error: No usable version of $compiler found."
echo "Error: No compatible version of $compiler was found within the range of $minVersion to $maxVersion. Please upgrade your toolchain or specify the compiler explicitly using CLR_CC and CLR_CXX environment variables."
exit 1
fi

CC="$(command -v "$compiler" 2> /dev/null)"
CXX="$(command -v "$cxxCompiler" 2> /dev/null)"
set_compiler_version_from_CC
Copy link
Member Author

Choose a reason for hiding this comment

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

@janvorli this is the branch which all the official/service pipelines hit., which remained untouched.

@janvorli
Copy link
Member

@am11 I didn't remember that we are using self-built clang of a specific version in the CI and for the official builds. So what I was afraid of is not a problem.

Then I don't have any worries about the change other than that people building .NET on their own machines can bump into subtle issues with their builds that we would not know about. Long time ago, I was trying to verify each newly added clang version using the coreclr / libraries tests to make sure they pass. But I've stopped doing that around version 12 and we've kept adding new versions possibly without full verification (unless you were doing that, of course). So this change doesn't change much of the risks.

@am11
Copy link
Member Author

am11 commented Jul 20, 2024

@janvorli, yup, since dotnet/dotnet-buildtools-prereqs-docker#832, we moved the official and other CI legs to just use clang without version suffix. Now it is so when we update clang in cbl/azurelinux crossdeps-builder file, it just gets updated everywhere and we rely on testing (initial version was 12 now azurelinux images have v18). Also, we are now using floating container tags e.g. azurelinux-3.0-cross-arm64-net9.0 instead of pinned azurelinux-3.0-cross-arm64-net9.0-20240712200413-56767ed, which presents much higher chance of subtle breakages than the old approach; pinned clang version and container tags. The current approach is to fix the issues as they are discovered.

Compared to that, the local or source build selecting latest clang (iff multiple clang versions are installed) poses no risk as those user can pass -clang18 or -gcc14 etc. to build.sh on the off-chance there is a need to do so.

Copy link
Member

@janvorli janvorli left a comment

Choose a reason for hiding this comment

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

LGTM, thank you!

@akoeplinger
Copy link
Member

I merged dotnet/arcade#14963, thanks!

@github-actions github-actions bot locked and limited conversation to collaborators Aug 21, 2024
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
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants