PR#1066: Update compiler guidelines for compiler policy change - packaging-committee

packaging-committee

#1066 Update compiler guidelines for compiler policy change

Merged 2 years ago by churchyard. Opened 3 years ago by tstellar.

tstellar/packaging-committee compiler-policy into master

Update compiler guidelines for compiler policy change

Tom Stellard • 2 years ago

97b3d2a

guidelines/modules/ROOT/pages/index.adoc

file modified

+28 -1

		`@@ -738,7 +738,34 @@`
		`[#compiler]`
		`== Compiler`

		`- Fedora uses gcc as the compiler (for all languages that gcc supports). Packages may only build with an alternative compiler to gcc if upstream does not support gcc.`
		`+ Fedora packages should default to using gcc as the compiler (for all languages that gcc supports) or clang if upstream does not support building with gcc. However, if there is a good technical reason, packagers may choose not to use the default compiler. Examples of valid technical reasons to not use the default compiler, include but are not limited to:`
		`+`
		`+ * The default compiler cannot build a package correctly.`
		`+ * The packager needs to disable a compiler feature (e.g. LTO) in order for the default compiler to correctly compile their package.`
		`+ * The default compiler takes significantly longer to build a package.`
		`+ * The default compiler is missing a feature that would benefit the package.`
		`+`
		`+ Packagers choosing to use a non-default compiler should document the reason for this decision in a comment in the spec file.`
		`+`
		`+ == Compiler macros`
		`+`
		`+ If clang is being used to build a package, packagers must set the %toolchain macro to clang:`
		`+`
		`+ ....`
		`+ %global toolchain clang`
		`+ ....`
		`+`
		`+ This ensures that Fedora's clang-specific compiler flags are used when compiling.`
		`+`
		`+ If a packager wants to use conditional macros in a spec file to make it easier to switch between two different compilers, then the following macros names should be used:`
		`+`
		`+ ....`
		`+ %bcond_with toolchain_clang`
		`+ %bcond_with toolchain_gcc`
		`+ ....`
		`+`
		`+ Packagers may also use the %build_cc, %build_cxx, or %build_cpp macros in the spec file in place of hard-coding the compiler name. The values of these variables are controled by setting the %toolchain macro to either clang or gcc.`
		`+`

		`== Compiler flags`

tstellar commented 3 years ago

https://fedoraproject.org/wiki/Changes/CompilerPolicy

This is a proposed change to the compiler policy to go along with the Fedora change proposal: https://fedoraproject.org/wiki/Changes/CompilerPolicy

It should not be merged unless the change proposal is accepted.

ngompa commented 3 years ago

There is a missing documentation requirement here. Deviating from our defaults should be documented and tracked. We require this with things like ExcludeArch/ExclusiveArch, so we should require it for this case too.

rebased onto 515dae02fd44354a9aa51e80047323e69c26c43d

3 years ago

tstellar commented 3 years ago

There is a missing documentation requirement here. Deviating from our defaults should be documented and tracked. We require this with things like ExcludeArch/ExclusiveArch, so we should require it for this case too.

OK, I've added this to the new policy.

zbyszek commented 2 years ago

Do we really need a package bug and a tracking bug? Taking the concrete example of Firefox, if Firefox maintainer switches the package for Firefox 89 to clang, and for Firefox 92 to gcc, and then maybe back to clang on arm64 at some point, I don't see how the dozen messages in the tracking bug would help with anything. First, the reasons for the switch would be very specific to the package, so "tracking" it together with other packages which switched to clang would do nothing to help other maintainers. Second, there is nothing to "fix", the package may well use the alternative compiler for the next 10 years without any intent to change bug, until the technical situation changes.

And if we want to find packages which require e.g. clang, repoquery --arch=src --disablerepo=\* --enablerepo=fedora-source --whatrequires clang works just as well, without wasting any additional maintainer cycles.

So overall, I'd say that the decision should be documented somewhere (in particular in the the spec file and/or dist-git commit message), and that's all.

tstellar commented 2 years ago

OK, I'm fine with either approach.

tstellar commented 2 years ago

I will move forward without the tracking bug requirement unless there are major objections.

Metadata Update from @tibbs:
- Pull-request tagged with: meeting

2 years ago

rebased onto 4a2b92b174f27afd6a0ad0b52d0f5725258cdbca

2 years ago

decathorpe commented 2 years ago

Looks good to me.

ngompa commented 2 years ago

:thumbsup:

tstellar commented 2 years ago

I've discovered that rpm requires macros to be at least 3 characters, so we can't use %cc. Is %ccc a good alternative?

ngompa commented 2 years ago

I've discovered that rpm requires macros to be at least 3 characters, so we can't use %cc. Is %ccc a good alternative?

Is this a restriction that could be easily fixed? Maybe @pmatilai knows?

pmatilai commented 2 years ago

The restriction is strictly in the "user side" of things, to guarantee automatic macros (ie `%*, %1 and the like) cannot be messed with. Removing the limitation would be easy enough but not a good idea, in otherwords: not going to happen.

Edited 2 years ago by pmatilai

churchyard commented 2 years ago

FWIW %ccc seems weird.

pmatilai commented 2 years ago

Digging deeper into the archived memories zone, the name length was once used to detect automatic macros. We have more sophisticated methods for that these days, but the limit is still used for preventing automatic macro overrides. If the override prevention was handled by some other means we could probably consider changing the limit as well, but this is not going to happen overnight and would need discussing upstream anyhow.

fweimer commented 2 years ago

The macros should be called %build_cc etc., to match %build_cflags. They also need to be documented in buildflags.md. We might need %extension_cc eventually, so the prefix makes sense from that perspective as well.

tstellar commented 2 years ago

@fweimer That seems OK, to me. Does anyone object to changing the macros to %build_cc, %build_cxx, %build_cpp ?

ngompa commented 2 years ago

Fine with me, go ahead and make the PR for it.

tstellar commented 2 years ago

Updated PR for redhat-rpm-config: https://src.fedoraproject.org/rpms/redhat-rpm-config/pull-request/142

rebased onto f601143eedbd499fff473c5c5e0e083b937ed2c9

2 years ago

pmatilai commented 2 years ago

FWIW, I ended up opening a PR to remove the limit anyhow: https://github.com/rpm-software-management/rpm/pull/1757

churchyard commented 2 years ago

I have formatting changes committed locally but I cannot push to @tstellar's fork.

Could you please grant me access?

https://pagure.io/fork/tstellar/packaging-committee/adduser

rebased onto 07b8f25751cc6acdc868d70e9ce7dc8eb09c8672

2 years ago

rebased onto 43865a73814ef7d417a05185cd11e13404104192

2 years ago

churchyard commented 2 years ago

Formatting changes pushed.

Consider me +1.

carlwgeorge commented 2 years ago

Can we add a SHOULD requirement to include a comment in the spec file documenting the "valid technical reason" for not using the default compiler? This would be similar to our existing SHOULD requirement for upstream patch status.

Edited 2 years ago by carlwgeorge

rebased onto 273c370846db67c7354a0df9fc5cbe24cea02f09

2 years ago

rebased onto 97b3d2a

2 years ago

tstellar commented 2 years ago

Can we add a SHOULD requirement to include a comment in the spec file documenting the "valid technical reason" for not using the default compiler? This would be similar to our existing SHOULD requirement for upstream patch status.

OK, I've added this to the documentation.

decathorpe commented 2 years ago

+1, Looks good to me, thanks!