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

rpm-4.18.0 embeds build machine CPU count #2343

Closed
bmwiedemann opened this issue Jan 10, 2023 · 24 comments
Closed

rpm-4.18.0 embeds build machine CPU count #2343

bmwiedemann opened this issue Jan 10, 2023 · 24 comments
Milestone

Comments

@bmwiedemann
Copy link
Contributor

While working on reproducible builds for openSUSE, I found that the recent upgrade to rpm-4.18.0
with #2047 / #1241 caused spec files with

make %{?_smp_mflags}

to embed the build machine CPU count into .src.rpm files, making them hard to bit-reproduce.
binary rpm files embed the .src.rpm header checksum, so suffer as well.

It might also introduce other hard-to-reproduce details, but I have not looked for these, yet.

@pmatilai
Copy link
Member

Right. This is very similar to the LTO case (7faf8ed), with probably a similar solution. The simple and sane solution is to have _smp_mflags expand to -j${RPM_BUILD_NCPUS}

Which ... we already did for slightly different reasons, but apparently I completely forgot about it in the meanwhile 😆 - see 0576d24. We just need to backport it to 4.18.

@pmatilai pmatilai added this to the 4.18.1 milestone Jan 10, 2023
@bmwiedemann
Copy link
Contributor Author

I tried

===================================================================
--- rpm-4.18.0.orig/platform.in
+++ rpm-4.18.0/platform.in
@@ -57,7 +57,7 @@
         if [ -n "$ncpus_max" ] && [ "$ncpus_max" -gt 0 ] && [ "$RPM_BUILD_NCPUS" -gt "$ncpus_max" ]; then RPM_BUILD_NCPUS="$ncpus_max"; fi; \\\
         echo "$RPM_BUILD_NCPUS";)
 
-%_smp_mflags -j%{_smp_build_ncpus}
+%_smp_mflags -j${RPM_BUILD_NCPUS}
 
 # Maximum number of threads to use when building, 0 for unlimited
 #%_smp_nthreads_max 0

But somehow I still get this diff

-make -j4
+make -j1

the produced /usr/lib/rpm/platform/x86_64-linux/macros has

%_smp_mflags -j%{_RPM_BUILD_NCPUS}

Why is that?

@bmwiedemann
Copy link
Contributor Author

bmwiedemann commented Jan 11, 2023

I checked git log and found some related commits.
#2344 now does the trick for me.
Edit: no, it still does not work.
Edit again : it works in one context, but not another with an older rpm-4.14.3 on the build host.

@pmatilai
Copy link
Member

pmatilai commented Jan 11, 2023

As mentioned in the PR, -j${RPM_BUILD_NCPUS} cannot possibly macro-expand to -j<number> because the macro engine does not expand environment variables from by $, it's all just a literal string to the macro engine. You appear to have a redefinition of %_smp_mflags someplace, ~/.rpmmacros being one of the possibilities.

@bmwiedemann
Copy link
Contributor Author

Right something writes it into ~/.rpmmacros . That is our problem then and the -j%{_RPM_BUILD_NCPUS} rpm patch should still be good.

@pmatilai
Copy link
Member

Ack. rpmdev-setuptree < 8.6 liked to put a value in there, but that was dropped in 2015 so it's a fairly old thing, something else seems more likely.

@pmatilai
Copy link
Member

pmatilai commented Jan 11, 2023

-j%{_RPM_BUILD_NCPUS} rpm patch should still be good.

Note, it's -j${RPM_BUILD_NCPUS}, not with % (and without the preceeding underscore too, I messed up that part in the above - oops)

@bmwiedemann
Copy link
Contributor Author

I have it with -j${RPM_BUILD_NCPUS} in https://build.opensuse.org/package/view_file/home:bmwiedemann:reproducible/rpm/0576d24756f.patch , but somehow it gets replaced by %_smp_mflags -j%{_RPM_BUILD_NCPUS} in platform files, which then breaks because the macro is unknown.
With the move to macros.in, it works. Is there some special rewriting?

@pmatilai
Copy link
Member

Oh. That's installplatform being over-eager (-e '/\${\w*:-/!s,\${,%{_,' presumably). So better to include the move to macros then, it's the right thing anyhow.

@pmatilai
Copy link
Member

To whomever ends up doing 4.18.1 release: this needs commits 5049fc7 and 0576d24

@Conan-Kudo
Copy link
Member

Right something writes it into ~/.rpmmacros . That is our problem then and the -j%{_RPM_BUILD_NCPUS} rpm patch should still be good.

This is probably obs-build, which redefines a bunch of stuff in ~/.rpmmacros when it shouldn't.

@bmwiedemann
Copy link
Contributor Author

@bmwiedemann
Copy link
Contributor Author

There are four .spec files in openSUSE, that use %{_smp_build_ncpus} directly, e.g.
https://code.opensuse.org/package/python3-pyside6/blob/ad8e8792e6ed522cb99332637de53ce6ff5b93f1/f/python3-pyside6.spec#_226

Maybe instead of 0576d24, we could redefine %_smp_build_ncpus to $RPM_BUILD_NCPUS ?

@pmatilai
Copy link
Member

pmatilai commented Jan 13, 2023

That would break the macro for anybody using it outside the build scriptlets, which is an entirely legit use. You'll need to fix those specs instead.

@bmwiedemann
Copy link
Contributor Author

bmwiedemann commented Jan 20, 2023

It seems, $RPM_BUILD_NCPUS is not set in our older distributions that have rpm-4.14.3. What would be the best way to have https://github.com/mesonbuild/meson/blob/c754f9076/data/macros.meson#L43 be working and reproducible on both 4.14 and 4.18 ?

@Conan-Kudo
Copy link
Member

Backport $RPM_BUILD_NCPUS.

@pmatilai
Copy link
Member

Yep, it's not exactly a big change: d97d7b7

bmwiedemann added a commit to bmwiedemann/obs-build that referenced this issue Jan 20, 2023
because with rpm-4.18 that ends up expanded into .src.rpm headers

rpm-software-management/rpm#2343
@bmwiedemann
Copy link
Contributor Author

bmwiedemann commented Jan 29, 2023

Found another problematic case:
https://code.opensuse.org/package/ocaml-rpm-macros/blob/819e56/f/ocaml-rpm-macros.spec#_464

'%%{?_smp_mflags}'
becomes
'-j${RPM_BUILD_NCPUS}' and the variable does not get expanded now, causing a compilation failure. I guess, using double-quotes should fix it.

Edit: submitted https://build.opensuse.org/request/show/1061850

@bmwiedemann
Copy link
Contributor Author

And more trouble from
https://codeberg.org/cunix/vendored_licenses_packager/src/branch/main/macros.vendored_licenses_packager#L18
that injects a random tmp path into the .src.rpm of dnscrypt-proxy

@pmatilai
Copy link
Member

binary rpm files embed the .src.rpm header checksum, so suffer as well.

Ugh, I hadn't realized the src.rpm header md5 (another ugh) ends up in the binary headers too. It only happens with -ba (iirc) so not all build-systems exhibit that, but still.

This would be nice case for placing the parsed spec outside the checksummed part of the header, but that in turn causes other problems...

@pmatilai
Copy link
Member

The reported and most glaring issue with %{_smp_mflags} was addressed in 4.18.x, closing but acknowledging that we may need something further around this.

@bmwiedemann
Copy link
Contributor Author

Found another reproducibility problem with expanded .spec in
https://code.opensuse.org/package/texlive/blob/d820a867c61524278af352b4febbb115e9dad08f/f/texlive.spec#_287

%{expand: %%global options %(mktemp /tmp/texlive-opts.XXXXXXXX)}

@bmwiedemann
Copy link
Contributor Author

found another breakage:
sed -i -e 's/-j2/%{?_smp_mflags}/' setup.py
in https://github.com/bmwiedemann/openSUSE/blob/master/packages/p/python-python-poppler/python-python-poppler.spec#L61

@pmatilai
Copy link
Member

Yes, there will be any number of ways the parsed spec inclusion can trip up bit-for-bit differences. "Breakage" is a strange and strong word for it, another angle to look at it is to use it as a tool to help reproducability: if the parsed spec (which is what the build will actually use, after all) differs it's acts as a warning that requires investigation. Some of which will be false positives, like mktemp.

bmwiedemann pushed a commit to bmwiedemann/rpm that referenced this issue Nov 13, 2023
because some usages of macros are hard to make reproducible

Fixes rpm-software-management#2343
bmwiedemann pushed a commit to bmwiedemann/rpm that referenced this issue Nov 13, 2023
because some usages of macros are hard to make reproducible

Fixes rpm-software-management#2343

This patch was done while working on reproducible builds for openSUSE.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants