#1163 Sources: Add section about conditionalization
Opened 6 months ago by sgallagh. Modified 4 months ago
sgallagh/packaging-committee condsource  into  master

@@ -231,3 +231,39 @@ 

   # https://dev.mysql.com/downloads/mysql/5.1.html

   Source0: mysql-5.1.31.tar.gz

  ....

+ 

+ == Do not conditionalize Sources

+ 

+ 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):

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):

+ 

+ ....

+ %if 0%{?fedora} < 35

+ Source1: mysource-old.tar.bz2

+ Patch1: oldpatch.patch

+ %else

+ Source1: mysource-ng.tar.bz2

+ Patch1: ngpatch.patch

+ %endif

+ ....

+ 

+ If you were to build this SRPM from a Fedora 34 host (`rpmbuild -bs mysource.spec`), the resulting SRPM would carry `mysource-old.tar.bz2` and `oldpatch.patch`. However, if you then try to use this SRPM to build for Fedora 35, the operation would fail because `mysource-ng.tar.bz2` is not available.

+ 

+ The correct behavior is to always carry all of the sources that might be used in the build and then conditionalize the *usage* of them instead. For example:

+ 

+ ```

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

+ Source1: mysource-old.tar.bz2

+ Source2: mysource-ng.tar.bz2

+ 

+ Patch1: oldpatch.patch

+ Patch2: ngpatch.patch

+ ...

+ %prep

+ ...

+ %if 0%{?fedora} < 35

+ tar xf %{SOURCE1}

+ %patch1 -p1

+ %else

+ tar xf %{SOURCE2}

+ %patch2 -p1

+ %endif

+ ``` 

\ No newline at end of file

A common mistake when writing multi-distro/multi-release spec files is
to conditionalize the source tarball or patch entries. While this may
generally work in Fedora because of how we always rebuild the SRPM for
the target build, it's bad practice and prevents the reuse of SRPMs on
other releases.

This new section requires that all sources be shipped in the SRPM and
then their use (rather than presence) is conditionalized.

Signed-off-by: Stephen Gallagher sgallagh@redhat.com

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.

rebased onto c4122b7985eb9dbaca9e7cb6f83ab8df2adb39c0

6 months ago

: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!

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

rebased onto 2097c88

6 months ago

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".

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

5 months ago

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?

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.