Skip to content

Conversation

@mikebeaton
Copy link
Member

@mikebeaton mikebeaton commented Oct 2, 2024

Description

This PR proposes moving clang+llvm from -dev to -build, with a view to making a future PR offering CLANGDWARF and CLANGPDB CI to EDK 2.

I note that gcc in the Ubuntu image uses update-alternatives to link to a fixed version - while this is potentially somewhat fragile (i.e. there is unfortunately no automatic way to link all and only the bins provided by a given version), it does allow a fixed version. On the other hand the Fedora image doesn't use anything similar.

I haven't added a fixed version with update-alternatives for clang in the Ubuntu image, though am happy to update the PR do so; with either the bins which appear to be needed to build, or all bins which seem to be provided with the given version - although this is quite an extensive list in the case of llvm-14 (75 bins).

EDIT: Strikethoughs as per this comment.

Issue #99

Containers Affected

ubuntu-22-build
ubuntu-22-test
ubuntu-22-dev
fedora-40-build
fedora-40-test
fedora-40-dev

@mikebeaton
Copy link
Member Author

I believe fedora-37-test is currently used for all EDK 2 CI https://github.com/tianocore/edk2/blob/master/.azurepipelines/templates/defaults.yml#L12 - specifically, for the CI which runs as Ubuntu-GCC5 - so possibly only fedora-40 would need this update?

@osteffenrh
Copy link
Contributor

Here is the PR that switches the CI to the Fedora 40 image: tianocore/edk2#6261

@mikebeaton
Copy link
Member Author

mikebeaton commented Oct 2, 2024

The default gcc for Ubuntu 22.04 is 11, so I guess the update-alternatives links are required, if gcc 12 is required. On the other hand, AFACIT gcc 11 builds everything in EDK 2 fine... The default clang for Ubuntu 22.04, version 14, also builds EDK 2 fine, so I can't see a need to use update-alternatives there - but then again, I cannot yet clearly see what the original need was for update-alternatives for gcc. I don't know if @jgarver can comment?

@jgarver
Copy link
Contributor

jgarver commented Oct 2, 2024

It was just to match the version of GCC in the Fedora images.

@mikebeaton
Copy link
Member Author

It was just to match the version of GCC in the Fedora images.

Thanks for the response. Possibly it is more 'useful' (e.g. for CI), in that case, to find out whether things build or not in the default current gcc in Ubuntu? (In fact, they do!)

@jgarver
Copy link
Contributor

jgarver commented Oct 2, 2024

"Building" and "working" are two different things.

I don't recall any specific issues with the default GCC on Ubuntu22, but I know we (NVIDIA) couldn't use the default GCC-9 in Ubuntu20. That's probably where the habit of using update-alternative began. The code originated with Ubuntu20 and was carried forward to Ubuntu22.

Anyway, I recommend:

  • Keeping GCC as is in Ubuntu22.
  • Not using update-alternative for clang, until we find a reason to not like the default version.
  • Trying to move to the default GCC when we add Ubuntu24 support.

Of course, that means deviating from Fedora. But from what I've seen, Fedora is a faster moving target and Ubuntu won't keep up anyway.

@osteffenrh
Copy link
Contributor

Any intention to work on this again? Or can I close this PR?

@mikebeaton
Copy link
Member Author

Hi @osteffenrh - Actually I am hoping to return to this shortly.

@mikebeaton mikebeaton marked this pull request as draft November 22, 2025 20:48
@mikebeaton mikebeaton marked this pull request as ready for review November 22, 2025 23:46
@mikebeaton
Copy link
Member Author

@jgarver @osteffenrh - Hi! I'm back on this again. Initial attempt at integrating CLANGDWARF/PDB builds into CI is finding new genuine code errors tianocore/edk2#11785, pretty much as expected and - hopefully - confirming the justification for this work.

The above is a new one from actually trying to start to integrate CLANGXXX into EDK2 CI, but note that these less serious issues tianocore/edk2#11738 were found while running clang (including but not only XCODE5) builds, as part of this work. (And we have continued to find and submit fixes for similar bugs and issues going back over a few years now, from building on CLANGXXX and XCODE5 toolchains.)

@mikebeaton mikebeaton changed the title Migrate clang/llvm to build images Migrate clang/llvm to package build image Nov 23, 2025
@mikebeaton
Copy link
Member Author

mikebeaton commented Nov 23, 2025

For now, I've removed the clang change from the Ubuntu image, since AFAICT the only CI step in the build host which notices toolchain, namely running package tests (if any; in the ubuntu host, not the fedora guest), does not actually support using anything except GCC5 (it is still GCC5, not GCC, in CI). And it currently seems like it might be a better step (at least initially) to pin that, or allow pinnning it, to gcc, rather than updating it to support clang.

@mikebeaton mikebeaton marked this pull request as draft November 23, 2025 08:40
@mikebeaton mikebeaton marked this pull request as ready for review November 23, 2025 09:13
@osteffenrh
Copy link
Contributor

osteffenrh commented Nov 24, 2025

What is your plan? Replace gcc with clang in all Linux CI jobs?

Yeah, the Linux CI still uses the GCC5 toolchain option (although with a gcc15 now).
I guess we could switch to the GCC toolchain in most cases?

... namely running package tests ...

Which ones do you mean?

@mikebeaton
Copy link
Member Author

mikebeaton commented Nov 25, 2025

What is your plan? Replace gcc with clang in all Linux CI jobs?

Not at all. The plan is to clone GCC5 CI to CLANGDWARF and CLANGPDB, and run it as well as GCC5 CI. Likely, this will be far to much to include, so at least minimal CLANGXXX CI clones of .azurepipelines/Ubuntu-GCC.yml, ArmVirtPkg/PlatformCI/.azurepipelines/Ubuntu-GCC.yml and OvmfPkg/PlatformCI/.azurepipelines/Ubuntu-GCC.yml, or similar.

The reason being, we have found many many real bugs by building with clang toolchains (CLANGPDB, CLANGDWARF, XCODE5), which we have been submitting back to EDK2 for some years, and which we have to fix for our CI (including CLANGDWARF, CLANGPDB and our own fully working fork of XCODE5 (including relocations, using some additional non-standard tools)) to complete. But it is not just for our sake, or just to run XCODE5, these are real bugs which would not be entering the EDK2 codebase if at least basic clang builds were in CI. (In addition, of course, this also keeps these officially supported CLANG toolchains building.)

I am open to discussion as to exactly which CI steps would be cloned to also run in CLANG, but I would very much like it to be some of them

Yeah, the Linux CI still uses the GCC5 toolchain option (although with a gcc15 now). I guess we could switch to the GCC toolchain in most cases?

... namely running package tests ...

Which ones do you mean?

I may be wrong about why some of my CLANG package builds are stopping during CI, with cryptic, low-information errors, even though some are building and completing. I was under the impression that it might be to do with the CI unit test runner (which I believe runs in the CI Ubuntu 'host' vm, not the CI Fedora 'guest' vm, and which I believe only supports GCC). I am still investigating this.

It might help to clarify that the Acidanthera CI that I am referring to (which includes CLANG) uses GitHub runners, with scripts controlling traditional EDK2 builds, not the stuart infrastructure in Azure Pipelines - hence the work to get something to offer as a PR here is new, and not a matter of submitting something we already had working.


Here are some example bugfixes:

tianocore/edk2#11794
tianocore/edk2#11785
tianocore/edk2@8e90e2c
tianocore/edk2@6bb2423
tianocore/edk2@c75a8d0
tianocore/edk2@51b58f4
tianocore/edk2@3c6960f
tianocore/edk2@5185e6c
tianocore/edk2@11d4edc
tianocore/edk2@ae83c6b
tianocore/edk2@e548e1c
tianocore/edk2@90fb3c6
tianocore/edk2@90fb3c6

The ones attributed to being found in XCODE5 are also found in CLANG builds (but are not found in GCC), if the warnings are turned back on tianocore/edk2#11761.

Oh, and by the way, I am finding more, now, as I clone and enable CLANG CI in my own private runners, while working on this (e.g. tianocore/edk2#11785, but there are others I have not had time to try to make fixes for yet). From past and current experience, some of these when I check will turn out to be fixed but never reported in Acidanthera, some will be new (where your CI hits any code which ours doesn't)

@osteffenrh
Copy link
Contributor

Sounds good, nice work.

@osteffenrh osteffenrh merged commit b562821 into tianocore:main Nov 25, 2025
1 check passed
@osteffenrh
Copy link
Contributor

Merged.

You probably want

diff --git a/.azurepipelines/templates/defaults.yml b/.azurepipelines/templates/defaults.yml
index e94e6351cbb3..e67bb9bed441 100644
--- a/.azurepipelines/templates/defaults.yml
+++ b/.azurepipelines/templates/defaults.yml
@@ -9,4 +9,4 @@
 
 variables:
   default_python_version: "3.12"
-  default_linux_image: "ghcr.io/tianocore/containers/fedora-43-test:bde74f8"
+  default_linux_image: "ghcr.io/tianocore/containers/fedora-43-test:b562821"

since the CI does nut run on the "latest" tag.

@mikebeaton
Copy link
Member Author

Tyvm - I'm currently running in my own Azure Pipelines with my own fork of both containers and edk2, so I am indeed running off :latest in mine, but I guess I can drop two out of three parts of those changes now. :-)

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants