Do we write in first person perspective? I think we generally write in second person or third person formal?

Thank you for writing this rule! It's really annoying when I encounter packages that do this...

There's basically only one valid case for it, where we have the main/freeworld split for The Other Repo(tm).

Do we write in first person perspective? I think we generally write in second person or third person formal?

Good catch, I'll switch that to second person.

:thumbsup:

:thumbsup: it would be good to have a guideline to point to, when suggesting this fix to people, like here: https://src.fedoraproject.org/rpms/rust-afterburn/pull-request/19#request_diff

This must have already been said somewhere, no?

In the bad example, all sources/patches would be numbered 1

Ok, there is this https://docs.fedoraproject.org/en-US/packaging-guidelines/#_no_arch_specific_sources_or_patches which has the same spirit, but is architecture specific.

It appears we indeed never say this. Thnaks!

Use %patch1 -p1 maybe?

Does $SOURCE1 work or is it a typo for %{SOURCE1}? Never seen it.

Does $SOURCE1 work or is it a typo for %{SOURCE1}? Never seen it.

It was a typo, thanks for catching it. I've made the other changes you requested and force-pushed.

Thanks for the review!

One concern I have is that since we already have https://docs.fedoraproject.org/en-US/packaging-guidelines/#_no_arch_specific_sources_or_patches, we should probably state all of the restrictions in the same place. Having one set of restrictions in the main guidelines and another restriction in the SourceURL document is bound to cause confusion.

To me it seems simplest to combine this with the existing section in the main guidelines, and just extend that to say "no conditionalization at all".

Yeah, I agree with the need for this, but I also agree with @tibbs that we better combine them together.

Maybe we can put those things together in a "SRPM files must be reproducible independently of the build environment" section of the main packaging guidelines? And there, mention that inclusion of SourceX and PatchX files MUST NOT be conditionalized on either the type / version of host system or the host architecture?

@decathorpe that'll work

Nit: Semantic newlines would be nice to use here and in the other prose paragraphs, for readability. i.e.

When building an SRPM, it is critical that all of the sources are present,
irrespective of the platform on which the SRPM is generated.
For example, the following code  *does not work* (the way you might expect):

You used Markdown code fencing on this one, instead of AsciiDoc.

I recently (until I understood the problem) ran into basically the same problem with a BuildRequires SRPM header I had defined conditionally using a %ifnarch conditional.

So the policy should really not just mention conditional PatchN and SourceN defintions (with the corresponding files possibly not ending up in the SRPM), but everything which ends up in an SRPM, which includes rpm tags like BuildRequires and possibly others.

And JFTR, the corresponding issue asking for corresponding rpmlintchecks appears to be https://github.com/rpm-software-management/rpmlint/issues/45 (from 2015).

Unfortunately, we need to be able to conditionalize BuildRequires. Otherwise, we're going to invalidate thousands of packages.

Unlike Patch and Source, conditional BuildRequires don't affect any modern build system in breaking ways. Even Mock will rebuild the SRPM for a SRPM to the target arch.

It might just be that specifically arch dependent conditionals are or at least can be a problem with BuildRequires when BuildArch: noarch is present.

I had an %ifnarched BuildRequires. So when the SRPM build happened to take place on a koji build arch which did not include the BuildRequires in the SRPM, and the resulting SRPM then happened be built on a koji builder arch where the spec file conditional includes the BuildRequires header in the SRPM build, the setup of the mock chroot will not have installed the package from the BuildRequires, and then the RPM build fails.

And regarding modern build system, I am talking about koji from 2022-08-19, trying to build the terminus-fonts package https://koji.fedoraproject.org/koji/buildinfo?buildID=2048753. I fixed this by adding an ExcludeArch and making the BuildRequires unconditional in https://src.fedoraproject.org/rpms/terminus-fonts/c/4db68408c398deeee20bc345925818fab8c5833f?branch=rawhide.

Maybe I was in my own special class of stupid by doing what I had done there, but if you are touching the guidelines on conditional things going into SRPMs anyway but without mentioning my case, I thought I should probably mention my case for consideration for inclusion.

Perhaps a non-negated "Make Sources unconditional" or "Make Source: and Patch: definitions unconditional" could be easier to read?

The orginal file ends with a newline (0x0a).

Following up to myself:

It might just be that specifically arch dependent conditionals are or at least can be a problem with BuildRequires when BuildArch: noarch is present.
[...]
Maybe I was in my own special class of stupid by doing what I had done there, but if you are touching the guidelines on conditional things going into SRPMs anyway but without mentioning my case, I thought I should probably mention my case for consideration for inclusion.

I was doing my own special class of stupid things there, the noarch releated Guidelines already describe what I should have done; and this PR using the general word "conditionalize" in the heading should catch anybody who thinks of making a SourceN: or PatchN: conditional, whether with a %if 0%{?fedora} >= 35 or with %ifnarch.

Sorry for distracting you with my noarch-%ifnarch-BuildRequires nonsense. That really does not belong here.

This limits us, because at the moment we conditionalize two patches for ELN in openssl (see https://src.fedoraproject.org/rpms/openssl/pull-request/25) and use %autosetup.

We apply 53 patches and are planning to move to src-git, so a conditional %patch directive is not really a good alternative here.

Please reconsider this use case.

This limits us, because at the moment we conditionalize two patches for ELN in openssl (see https://src.fedoraproject.org/rpms/openssl/pull-request/25) and use %autosetup.

We apply 53 patches and are planning to move to src-git, so a conditional %patch directive is not really a good alternative here.

Please reconsider this use case.

This isn't a policy decision (exactly), it's a limitation of the RPM packaging format. If you put a conditional around a patch, then that patch will not end up in the SRPM on some platforms. Meaning I can't download the SRPM from Fedora Rawhide and use it to build the package for Fedora ELN, because it will be missing the necessary patch.

Maybe a better option would be to enhance %autosetup to gain some additional controls on which patches it applies so we could skip certain ones.

This limits us, because at the moment we conditionalize two patches for ELN in openssl (see https://src.fedoraproject.org/rpms/openssl/pull-request/25) and use %autosetup.

Switch from %autosetup to %setup -q + %autopatch.

%autopatch supports patch ranges:

# Apply patches using %autosetup configured SCM.
# Typically used with no arguments to apply all patches in the order
# introduced in the spec, but alternatively can be used to apply indvidual
# patches in arbitrary order by passing them as arguments.
# -v        Verbose
# -p<N>     Prefix strip (ie patch -p argument)
# -m<min>       Apply patches with number >= min only (if no arguments)
# -M<max>       Apply patches with number <= max only (if no arguments)
%autopatch(vp:m:M:)

We apply 53 patches and are planning to move to src-git, so a conditional %patch directive is not really a good alternative here.

Please reconsider this use case.

No. The downsides are too high.

This isn't a policy decision (exactly), it's a limitation of the RPM packaging format. If you put a conditional around a patch, then that patch will not end up in the SRPM on some platforms. Meaning I can't download the SRPM from Fedora Rawhide and use it to build the package for Fedora ELN, because it will be missing the necessary patch.

I'm aware that it's a limitation of the packaging format. I'm wondering whether the goal of having the SRPM build the same on different platforms is worth having considering mock can be used to get an SRPM as built on a different system without a lot of additional effort. The same doesn't apply to the currently existing rules, because it's not as simple to switch the architecture.

Switch from %autosetup to %setup -q + %autopatch.

%autopatch supports patch ranges:

This expects us to specify the patch number, which isn't easily predictable when using src-git and would require manually specifying it in commit messages with potential breakage on rebases.

What would happen if rpm were to unconditionally include all files defined with Source: or SourceN: or Patch: or PatchN: into the SRPM file, but still only conditionally "see" them while building binary RPMs?

Then we would need to have a different discussion. But the chance of that happening is essentially zero, and I see very little point in that type of hypothetical.

We talked about this in this weeks meeting, and it'd be nice if someone added info/links about the autopatch range stuff near this ... and/or fixed up the line breaks, but better to have it as is.

IMO, it is bad practice to use tar in specfiles. What do you all think about amending this example to use the proper %setup invocation?