Skip to content

Revert "Pin SHAs for container images" #113248

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

Merged
merged 3 commits into from
Mar 13, 2025
Merged

Revert "Pin SHAs for container images" #113248

merged 3 commits into from
Mar 13, 2025

Conversation

richlander
Copy link
Member

Reverts #113185

Reverting enables us to safely test Clang 20 being added to these images per dotnet/dotnet-buildtools-prereqs-docker#1369

@Copilot Copilot AI review requested due to automatic review settings March 7, 2025 06:44
@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 Mar 7, 2025
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

PR Overview

This PR reverts the previous change that pinned container image SHAs in the pipeline file, allowing the use of unpinned image tags to facilitate testing of Clang 20 additions.

  • Reverts SHA pinning in container image references
  • Enables testing of updated images with Clang 20 support

Reviewed Changes

File Description
eng/pipelines/common/templates/pipeline-with-resources.yml Removed SHA pins from container image definitions

Copilot reviewed 1 out of 1 changed files in this pull request and generated no comments.

Comments suppressed due to low confidence (1)

eng/pipelines/common/templates/pipeline-with-resources.yml:20

  • Removing the SHA pin from container image references may introduce non-determinism in builds if the image is updated unexpectedly. Consider verifying through tests or safeguards that these images remain compatible and stable.
image: mcr.microsoft.com/dotnet-buildtools/prereqs:azurelinux-3.0-net10.0-cross-arm

@richlander
Copy link
Member Author

/azp run runtime-libraries-coreclr outerloop-linux

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@am11
Copy link
Member

am11 commented Mar 7, 2025

Noticed here https://dev.azure.com/dnceng-public/public/_build/results?buildId=971589&view=logs&jobId=62b70ecb-649d-5f05-1138-74c7e6e4705f&j=2f68d9b0-7564-5966-3317-2e2aa7d356c0&t=eee86911-880c-5ea0-1825-5072a102d343, it's still picking up llvm 19 images:

  -- The C compiler identification is Clang 19.1.6
  -- The CXX compiler identification is Clang 19.1.6
  -- Detecting C compiler ABI info
  -- Detecting C compiler ABI info - done

dotnet/dotnet-buildtools-prereqs-docker#1369 (comment)

@richlander
Copy link
Member Author

richlander commented Mar 7, 2025

Odd. I opened this PR after seeing this change pushed: dotnet/versions@917ff98. That diff suggests (to me) that there was a source change not just a re-build. I think it was from this change: dotnet/dotnet-buildtools-prereqs-docker@4ac71d0. That's strange since that was 2 days ago.

Hopefully the expected changes are published today. If not, someone will need to take a look at the publish pipeline.

@mthalman

@mthalman
Copy link
Member

mthalman commented Mar 7, 2025

The changes from dotnet/dotnet-buildtools-prereqs-docker#1369 didn't get properly published because crossdeps-llvm was never rebuilt: dotnet/dotnet-buildtools-prereqs-docker#1369 (comment). I've started a new build about an hour ago to fix that. These builds take like 7 hrs so it'll be a while.

@richlander
Copy link
Member Author

Thanks @mthalman! Nothing is ever simple around here!

@richlander
Copy link
Member Author

This looks better:

$ docker run --rm mcr.microsoft.com/dotnet-buildtools/prereqs:azurelinux-3.0-net10.0-cross-amd64-musl clang --version
clang version 20.1.0
Target: x86_64-unknown-linux-gnu
Thread model: posix
InstalledDir: /usr/local/bin

Per: dotnet/versions@25648fc

@richlander
Copy link
Member Author

/azp run runtime-libraries-coreclr outerloop-linux

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@richlander
Copy link
Member Author

Looks like our conservative approach with pinning was warranted and saved a bunch repo-wide hassle.

There are a few patterns that are now disallowed (at least by default) by the compiler, I'm guessing.

Lots of errors like:

 In file included from <built-in>:1:
  In file included from /__w/1/s/artifacts/obj/coreclr/linux.x86.Checked/vm/wks/CMakeFiles/cee_wks_mergeable.dir/cmake_pch.hxx:5:
  In file included from /__w/1/s/src/coreclr/vm/common.h:302:
  In file included from /__w/1/s/src/coreclr/vm/ceeload.h:40:
  In file included from /__w/1/s/src/coreclr/vm/readytoruninfo.h:14:
  In file included from /__w/1/s/src/coreclr/vm/nativeformatreader.h:15:
  In file included from /usr/local/lib/clang/20/include/emmintrin.h:17:
  /usr/local/lib/clang/20/include/xmmintrin.h:148:3: error: statement not allowed in constexpr function
    148 |   return (__m128)((__v4sf)__a - (__v4sf)__b);
        |   ^
  /__w/1/s/src/coreclr/inc/debugreturn.h:101:16: note: expanded from macro 'return'
    101 | #define return for (;1;__ReturnOK::safe_to_return()) return
        |                ^

@mthalman
Copy link
Member

What is the status of getting this working? By keeping runtime pinned on digests, it's important that blocking issues from newer images are addressed in a timely manner so that newer updates to the images (outside of the clang changes) can be incorporated.

@mthalman
Copy link
Member

Ok, I see that #113279 has the proposed fix.

@richlander
Copy link
Member Author

/azp run runtime-libraries-coreclr outerloop-linux

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@richlander
Copy link
Member Author

These look like infra related issues. Fair @sbomer @jkoritzinsky?

@sbomer
Copy link
Member

sbomer commented Mar 13, 2025

Yes, the failures look like timeouts. LGTM!

@richlander richlander merged commit c3d95b4 into main Mar 13, 2025
148 of 151 checks passed
@richlander richlander deleted the revert-113185-shaken branch March 13, 2025 23:49
@richlander
Copy link
Member Author

richlander commented Mar 13, 2025

FYI: The implication of this revert is that we're now pulling new build images with clang 20, where we were previously pinned to images including clang 19.

https://discourse.llvm.org/t/llvm-20-1-0-released/85122

@MichalStrehovsky
Copy link
Member

The docker toolset update broke native AOT on ARM32. I submitted a revert of this at #113598 to confirm; ARM32 works again with a revert.

Filed #113609 for tracking, it's not clear to me why unaligned memory access worked before and now stopped working.

It is my understanding we're snapping dotnet/runtime for Preview 3 today.

steveisok pushed a commit that referenced this pull request Mar 17, 2025
richlander added a commit that referenced this pull request Mar 18, 2025
jkotas pushed a commit that referenced this pull request Mar 19, 2025
@github-actions github-actions bot locked and limited conversation to collaborators Apr 17, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants