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

Update Ubuntu 22.04 crossdeps to install llvm 17 #944

Merged
merged 7 commits into from
Jan 8, 2024

Conversation

am11
Copy link
Member

@am11 am11 commented Dec 29, 2023

Copy link

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

@akoeplinger akoeplinger enabled auto-merge (squash) January 8, 2024 18:32
@akoeplinger akoeplinger merged commit f22565c into dotnet:main Jan 8, 2024
36 checks passed
@am11
Copy link
Member Author

am11 commented Jan 9, 2024

@akoeplinger, thanks for merging this. Do you know why https://github.com/dotnet/versions/blob/0412008aa77369c93ae8e702e2a7029f818659b5/build-info/docker/image-info.dotnet-dotnet-buildtools-prereqs-docker-main.json#L3510 hasn't picked it up? Usually it takes an hour or two after the PR merge here when new tags are published. This seems to be stuck in some internal leg. Probably worth checking if one of the recent PR regressed the build?

@am11 am11 deleted the patch-7 branch January 9, 2024 06:30
@akoeplinger
Copy link
Member

akoeplinger commented Jan 9, 2024

@am11 yup it was blocked internally (unrelated to the PRs), I fixed the issue and we should see the update in about an hour.

@akoeplinger
Copy link
Member

@am11 btw. we should probably look into adding a cbl-mariner based cross image so it matches what we use for the main architectures (x64, arm64 etc)

Looks like only riscv64, ppc64le and s390x are still using the ubuntu-based image: https://github.com/dotnet/runtime/blob/44fd111025edf69907e2a1fd1de8b2ebbbe1e6b3/eng/pipelines/common/templates/pipeline-with-resources.yml#L67-L80

@am11
Copy link
Member Author

am11 commented Jan 10, 2024

Latest staging tag ubuntu-22.04-cross-riscv64-20230215165123-0ea3466 still doesn't have clang-17 yet, so I guess it built the previous commit and not the latest one (yet), or something else went wrong? We should see /usr/bin/clang-17 instead of /usr/bin/clang-15 in the container.

btw. we should probably look into adding a cbl-mariner based cross image so it matches what we use for the main architectures (x64, arm64 etc)

True, I think we should definitely switch over ppc64le and s390x to cbl-mariner. The only reason for sticking the evolving riscv64 to Ubuntu base layer was that we needed the latest llvm at the time (and there was no cbl-mariner option). If we can update cbl-mariner to use llvm17, that would be good. I think llvm16 is also fine; risc-v is relatively a new architectures with interesting extensions still being ratified (e.g. dotnet/runtime#92974 (comment)); llvm-toolchain adds support for new extensions only in the bleeding-edge version (new features aren't backported in llvm-project either 😀).

@akoeplinger
Copy link
Member

@am11 ubuntu-22.04-cross-riscv64-20240109142831-3fc5553 should have the change now.

If we can update cbl-mariner to use llvm17, that would be good. I think llvm16 is also fine; risc-v is relatively a new architectures with interesting extensions still being ratified (e.g. dotnet/runtime#92974 (comment)); llvm-toolchain adds support for new extensions only in the bleeding-edge version (new features aren't backported in llvm-project either 😀).

Looks like we're already using LLVM 16 since #864, I don't know if upgrading to 17 would be a big deal (probably requires a bit of coordination with dotnet/runtime at least).

@am11
Copy link
Member Author

am11 commented Jan 10, 2024

@am11 ubuntu-22.04-cross-riscv64-20240109142831-3fc5553 should have the change now.

Unfortunately not:

$ docker run --rm \
  mcr.microsoft.com/dotnet-buildtools/prereqs:ubuntu-22.04-cross-riscv64-20240109142831-3fc5553 \
  sh -c 'find /usr/bin -name clang*'

/usr/bin/clangd-15
/usr/bin/clang-cpp-15
/usr/bin/clang-15
/usr/bin/clang++-15

@akoeplinger
Copy link
Member

Hmm weird, the build log for the crossdeps image shows it getting installed:

#6 9.091 Get:6 https://apt.llvm.org/jammy llvm-toolchain-jammy-17/main amd64 clang-17 amd64 1:17.0.6~++20231209124227+6009708b4367-1~exp1~20231209124336.77 [144 kB]

@am11
Copy link
Member Author

am11 commented Jan 10, 2024

Ah we probably need to touch the file under riscv directory too so it doesn’t skip the build. There is also a way to evict the cache and force build all the layers.

@am11
Copy link
Member Author

am11 commented Jan 10, 2024

@mthalman, should I submit a PR with dummy change in https://github.com/dotnet/dotnet-buildtools-prereqs-docker/blob/main/src/ubuntu/22.04/cross/riscv64/Dockerfile to trigger the build or could you force build it somehow (e.g. by evicting the cache)?

@mthalman
Copy link
Member

I've kicked off a build with the cache disabled.

@mthalman
Copy link
Member

There are many errors in various build legs of the pipeline that are preventing a successful publish:
#950
#951
#952
#953

I haven't looked whether any of these issues are from the Dockerfile you care about. Please check up on that. If your Dockerfile is unaffected by these errors, then I would recommend making a change to the Dockerfile (such as whitespace) to cause it to trigger a build without needing to disable the cache (which is causing all these other failures).

@am11
Copy link
Member Author

am11 commented Jan 10, 2024

Thanks. None of these are related to Ubuntu 22.04, so it's safe to assume that it will go through with a dummy change in riscv Dockerfile. I'll open a PR shortly.

@am11 am11 mentioned this pull request Jan 10, 2024
@am11
Copy link
Member Author

am11 commented Jan 11, 2024

New image is built, now we have clang-17:

$ docker run --rm \
  mcr.microsoft.com/dotnet-buildtools/prereqs:ubuntu-22.04-cross-riscv64-20240110235310-34800da \
  sh -c 'find /usr/bin -name 'clang*''

/usr/bin/clang-17
/usr/bin/clang-cpp-17
/usr/bin/clangd-17
/usr/bin/clang++-17

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

Successfully merging this pull request may close these issues.

3 participants