Improvements of the Makefile distribution targets #3003

pcahyna · 2023-06-02T15:35:48Z

Pull Request Details:

Type: Bug Fix
Impact: Low
Reference to related issue (URL):
How was this pull request tested?
make srpm and make rpm
Brief description of the changes in this pull request:
Several improvements to the Makefile targets that generate distribution packages:
- The dist tarball (dist/$(name)-$(distversion).tar.gz) has been considered always up to date by make, and thus not remade when any of the files changed. This has caused "make srpm" (and other targets depending on the dist tarball - "make rpm", "make deb" etc.) to build packages without newer changes to the sources.
  Fix by marking dist/$(name)-$(distversion).tar.gz as a phony target that will be always rebuilt.
- The source RPM has been specified as a glob pattern: dist/$(name)-$(version)-*.src.rpm. This breaks when there are multiple source RPMs under dist/ - leftovers from previous builds. It forces us to use clean or package-clean always before using "make rpm", which is not ideal.
  Fix by determining the actual full package name (with the dist tag) by parsing the spec file and using that.
- Do not %define rpmrelease in the spec file, define it on the command line in Makefile instead. Allows us to eliminate one sed transform of the spec file to set rpmrelease to the desired value.

The dist tarball (dist/$(name)-$(distversion).tar.gz) has been considered always up to date by make, and thus not remade when any of the files changed. This has caused "make srpm" (and other targets depending on the dist tarball - "make rpm", "make deb" etc.) to build packages without newer changes to the sources. Fix by marking dist/$(name)-$(distversion).tar.gz as a phony target that will be always rebuilt.

The source RPM has been specified as a glob pattern: dist/$(name)-$(version)-*.src.rpm This breaks when there are multiple source RPMs under dist/ - leftovers from previous builds. It forces us to use clean or package-clean always before using "make rpm", which is not ideal. Fix by determining the actual full package name (with the dist tag) by parsing the spec file and using that. Use the full name also for an informational message on terminal.

Define it on the command line in Makefile instead. Allows us to eliminate one sed transform of the spec file. Keep `%define debug_package %{nil}`, it needs to be set when building the RPM without the Makefile.

pcahyna · 2023-06-02T15:36:33Z

@schlomo, I think you touched the Makefile and especially the package generation recently, can you please have a look?

schlomo

About always rebuilding the tar.gz archive: I disabled that because I wanted to build all the distro packages off the same tar.gz and save some time when running tools/run-in-docker -- make package.

Can you find a way to keep that "feature"?

pcahyna · 2023-06-02T16:57:20Z

Sorry, I did now know that it was intentional. Can you please point me to the commit where you made the change? I don't see anything related in the commit messages.

schlomo · 2023-06-02T17:17:27Z

I'm not yet as good with my commit messages... It was when I returned the makefile for the package build GH action

Make it depend on all its files instead.

pcahyna · 2023-06-02T18:42:36Z

@schlomo I think it is a bit unusual - the autotools - generated Makefiles also always rebuild the dist tarball. But I am trying. Please test the latest commit.

pcahyna · 2023-06-05T08:50:03Z

@schlomo can you please test whether the latest commit resolves your concern?

pcahyna · 2023-06-05T12:21:45Z

@schlomo why are you merging 'master' into this branch, is there some particular new feature there that you want to test together with my change?

schlomo · 2023-06-05T12:33:26Z

@pcahyna can you have a look at https://github.com/pcahyna/rear/actions/runs/5176475050 please? It doesn't work with your change, while https://github.com/rear/rear/actions/runs/5176703833 worked (current master without your change).

In general, for any change to the Makefile I kindly ask to keep this GH Action working and to check that the tar.gz is the same (identical) for every OS.

@schlomo why are you merging 'master' into this branch, is there some particular new feature there that you want to test together with my change?

Yes, I removed Fedora Rawhide from the list of distros as that was broken and prevented the Build Packages GH Action from working and your change needs to be tested against that.

pcahyna · 2023-06-05T17:15:35Z

@schlomo sure, looking. Could you please add this GitHub action to the CI somehow? Right now I see
"All checks have passed
20 successful checks"
and the only indication that something is wrong is a tiny red ❌ next to the commit ID.

There is no guarantee that "make srpm" will trigger rebuild of the dist tarball dist/$(name)-$(distversion).tar.gz - the file may already exist. In this case the temporary file does not get saved. Extract the spec file from the dist tarball instead.

pcahyna · 2023-06-05T20:39:15Z

to check that the tar.gz is the same (identical) for every OS

do you mean that all the OSes should have the same .tar.gz ? How to check that?

pcahyna · 2023-06-05T21:17:16Z

Anyway @schlomo please have a look at the last commit again, the GH action now passes.

schlomo · 2023-06-06T06:49:23Z

@pcahyna much better, only CentOS 6 is still acting up (as usual):

The binary RPM was not generated, here is the log for it:

********** centos:6                                 **********
rm -f dist/*.rpm dist/*.deb dist/*.pkg.*
== Building SRPM package rear-2.7-git.5181.1a842d2b.makesrpmimprovements ==
if test "/tmp/tmp.qQUdmt3OmH.spec"; then tar -xzOf dist/rear-2.7-git.5181.1a842d2b.makesrpmimprovements.tar.gz rear-2.7-git.5181.1a842d2b.makesrpmimprovements/packaging/rpm/rear.spec > "/tmp/tmp.qQUdmt3OmH.spec"; fi
rm -rf /var/tmp/build-rear-2.7
mkdir -p /var/tmp/build-rear-2.7
cp dist/rear-2.7-git.5181.1a842d2b.makesrpmimprovements.tar.gz /var/tmp/build-rear-2.7/
rpmbuild -ts --clean --nodeps \
		--define="_sourcedir /rear/dist" \
		--define="_srcrpmdir /rear/dist" \
		--define="_topdir /var/tmp/build-rear-2.7/rpmbuild" --define="rpmrelease .git.5181.1a842d2b.makesrpmimprovements" --define="debug_package %{nil}" \
		/var/tmp/build-rear-2.7/rear-2.7-git.5181.1a842d2b.makesrpmimprovements.tar.gz
Wrote: /rear/dist/rear-2.7-1.git.5181.1a842d2b.makesrpmimprovements.el6.src.rpm
Executing(--clean): /bin/sh -e /var/tmp/rpm-tmp.MNJrfr
+ umask 022
+ cd /var/tmp/build-rear-2.7/rpmbuild/BUILD
+ rm -rf rear-2.7-git.5181.1a842d2b.makesrpmimprovements
+ exit 0
/bin/bash: rpmspec: command not found
/bin/bash: rpmspec: command not found
== Building RPM package  ==
rpmbuild --rebuild --clean \
		--define="_rpmdir /rear/dist" \
		--define "_rpmfilename %%{NAME}-%%{VERSION}-%%{RELEASE}.%%{ARCH}.rpm" \
		--define="_topdir /var/tmp/build-rear-2.7/rpmbuild" --define="rpmrelease .git.5181.1a842d2b.makesrpmimprovements" --define="debug_package %{nil}" \
		dist/.src.rpm
error: cannot open dist/.src.rpm: No such file or directory
make: *** [rpm] Error 1
tar: Removing leading `/' from member names
/var/tmp/build-rear-2.7/
/var/tmp/build-rear-2.7/rear-2.7-git.5181.1a842d2b.makesrpmimprovements.tar.gz
/var/tmp/build-rear-2.7/rpmbuild/
/var/tmp/build-rear-2.7/rpmbuild/SPECS/
/var/tmp/build-rear-2.7/rpmbuild/BUILD/
/var/tmp/build-rear-2.7/rpmbuild/RPMS/
/var/tmp/build-rear-2.7/rpmbuild/BUILDROOT/
********** Copying dist to dist-all/centos-6

Can you please try to fix this?

The reason it is not part of the CI checks is 1) it runs very long and 2) I want to see it stabilize more before adding it.

I thought to maybe run it nightly instead of after every commit, I'll also add support to publish the resulting binaries via GH.

schlomo · 2023-06-06T06:50:02Z

Oh, and you can use the wrapped up build area for CentOS 6 to look into it, maybe it helps debugging

pcahyna · 2023-06-06T09:32:50Z

The required rpmspec tool does not exist on EL 6 yet.

pcahyna · 2023-06-06T10:01:09Z

only CentOS 6 is still acting up

Then why do I see a green check mark ✔️ in pcahyna@1a842d2 ? Something is wrong with error reporting in your GitHub action (there is that error hidden deep in the logs indeed, but it does not affect the visible result).
By the way, I don't see the result of the GitHub action at 1a842d2 and above.

It is more elegant than rpm, as it can print the source package information, which is what we are interested in. Unfortunately, it does not exist on EL 6 yet.

pcahyna · 2023-06-06T10:12:35Z

Can you please try to fix this?

I believe it should be fixed now.

pcahyna · 2023-06-06T10:13:38Z

The reason it is not part of the CI checks is 1) it runs very long

I saw it running for half a hour, which is not that long.

and 2) I want to see it stabilize more before adding it.

Agreed.

schlomo · 2023-06-06T11:12:42Z

https://github.com/pcahyna/rear/actions/runs/5187314401/jobs/9349534521#step:7:31 (sorry for again pointing to the logs) has changed in the dist archive name, this indicates that there was a change in the source tree that is not committed to git.

The ls -lR step is good for a quick check: There should be no tar.gz archives on the top level and in every subdirectory there should be the same tar.gz (at least of the same size). See https://github.com/rear/rear/actions/runs/5186427841/jobs/9347522192#step:7:38 for a "good" example.

Maybe I should add more sanity checks to it, to automatically validate the stuff I just said?

pcahyna · 2023-06-06T11:28:20Z

has changed in the dist archive name, this indicates that there was a change in the source tree that is not committed to git.

Do you know what the change is? The previous commit did not have this problem and I don't see what in the last commit could have broken it.

pcahyna · 2023-06-06T11:35:06Z

I see this difference:

== Prepare manual ==
make -C doc man
make[1]: Entering directory '/home/runner/work/rear/rear/doc'
make[1]: Nothing to be done for 'man'.
make[1]: Leaving directory '/home/runner/work/rear/rear/doc'

in https://github.com/pcahyna/rear/actions/runs/5181590712/jobs/9337304522#step:5:30
vs

== Prepare manual ==
make -C doc man
make[1]: Entering directory '/home/runner/work/rear/rear/doc'
asciidoctor -b manpage -d manpage rear.8.adoc
make[1]: Leaving directory '/home/runner/work/rear/rear/doc'

in https://github.com/pcahyna/rear/actions/runs/5187314401/jobs/9349534521#step:5:30

so, the modified file might be just the manual page.

schlomo · 2023-06-06T11:39:04Z

Yes, I'd love to change the way how we generate the man page so that it doesn't happen accidentally when we don't need it.

pcahyna · 2023-06-06T11:40:49Z

I suspect this is just random, depending on the relative order of timestamp on rear.8 vs. rear.8.adoc created during checkout.

pcahyna · 2023-06-06T11:41:46Z

rerunning to confirm.

pcahyna · 2023-06-06T11:43:25Z

Maybe I should add more sanity checks to it, to automatically validate the stuff I just said?

Sure, although I would not consider it a priority if you are willing to help me debugging this PR manually, I suspect we won't touch the Makefile for some time after we finish this.

schlomo · 2023-06-06T11:53:31Z

Happy to help here of course. I think that before making the build packages workflow part of the CI I should upgrade it to better check for errors and explain them so that other have a fair chance of quickly fixing it.

pcahyna · 2023-06-06T13:02:08Z

@schlomo the rerun https://github.com/pcahyna/rear/actions/runs/5187314401/jobs/9351273206 seems OK (please check again), so the problem is not reproducible indeed and I believe it has not been caused by my changes.

schlomo · 2023-06-06T14:23:18Z

Thanks a lot for this improvement, I especially like how you create the dependency via the DIST_FILES

pcahyna · 2023-06-06T16:31:38Z

I home the dependency won't break anything, it does not seem very standard.

Yes, I removed Fedora Rawhide from the list of distros as that was broken and prevented the Build Packages GH Action from working and your change needs to be tested against that.

What's the problem with Fedora Rawhide by the way?

schlomo · 2023-06-06T16:33:24Z

fedora-cloud/docker-brew-fedora#109

pcahyna · 2023-06-06T16:37:58Z

https://lists.fedoraproject.org/archives/list/devel@lists.fedoraproject.org/thread/RIIOCKWR2MBYARJNKVZXT3YLXBYGRJAF/
https://pagure.io/fedora-infrastructure/issue/11358

it seems you can use images from another registry.

pcahyna added 4 commits June 2, 2023 15:30

Extract common RPM defines in Makefile to a var

f63785d

Eliminate a %define from RPM spec

f8c77f6

Define it on the command line in Makefile instead. Allows us to eliminate one sed transform of the spec file. Keep `%define debug_package %{nil}`, it needs to be set when building the RPM without the Makefile.

pcahyna requested a review from schlomo June 2, 2023 15:35

schlomo reviewed Jun 2, 2023

View reviewed changes

Do not rebuild the dist tarball always

149653c

Make it depend on all its files instead.

Merge branch 'master' into make-srpm-improvements

7122a91

schlomo mentioned this pull request Jun 5, 2023

Merge / Rebase master before merging PRs? #3004

Closed

Do not use the rpmspec tool for parsing RPM spec

a7a51df

It is more elegant than rpm, as it can print the source package information, which is what we are interested in. Unfortunately, it does not exist on EL 6 yet.

schlomo merged commit 29f5185 into rear:master Jun 6, 2023
16 of 20 checks passed

jsmeix added enhancement Adaptions and new features fixed / solved / done labels Jun 7, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improvements of the Makefile distribution targets #3003

Improvements of the Makefile distribution targets #3003

pcahyna commented Jun 2, 2023 •

edited

pcahyna commented Jun 2, 2023

schlomo left a comment

pcahyna commented Jun 2, 2023

schlomo commented Jun 2, 2023

pcahyna commented Jun 2, 2023

pcahyna commented Jun 5, 2023

pcahyna commented Jun 5, 2023

schlomo commented Jun 5, 2023 •

edited

pcahyna commented Jun 5, 2023

pcahyna commented Jun 5, 2023

pcahyna commented Jun 5, 2023

schlomo commented Jun 6, 2023

schlomo commented Jun 6, 2023

pcahyna commented Jun 6, 2023

pcahyna commented Jun 6, 2023

pcahyna commented Jun 6, 2023

pcahyna commented Jun 6, 2023

schlomo commented Jun 6, 2023

pcahyna commented Jun 6, 2023

pcahyna commented Jun 6, 2023

schlomo commented Jun 6, 2023

pcahyna commented Jun 6, 2023

pcahyna commented Jun 6, 2023

pcahyna commented Jun 6, 2023

schlomo commented Jun 6, 2023

pcahyna commented Jun 6, 2023

schlomo commented Jun 6, 2023

pcahyna commented Jun 6, 2023

schlomo commented Jun 6, 2023

pcahyna commented Jun 6, 2023

Improvements of the Makefile distribution targets #3003

Improvements of the Makefile distribution targets #3003

Conversation

pcahyna commented Jun 2, 2023 • edited

Pull Request Details:

pcahyna commented Jun 2, 2023

schlomo left a comment

Choose a reason for hiding this comment

pcahyna commented Jun 2, 2023

schlomo commented Jun 2, 2023

pcahyna commented Jun 2, 2023

pcahyna commented Jun 5, 2023

pcahyna commented Jun 5, 2023

schlomo commented Jun 5, 2023 • edited

pcahyna commented Jun 5, 2023

pcahyna commented Jun 5, 2023

pcahyna commented Jun 5, 2023

schlomo commented Jun 6, 2023

schlomo commented Jun 6, 2023

pcahyna commented Jun 6, 2023

pcahyna commented Jun 6, 2023

pcahyna commented Jun 6, 2023

pcahyna commented Jun 6, 2023

schlomo commented Jun 6, 2023

pcahyna commented Jun 6, 2023

pcahyna commented Jun 6, 2023

schlomo commented Jun 6, 2023

pcahyna commented Jun 6, 2023

pcahyna commented Jun 6, 2023

pcahyna commented Jun 6, 2023

schlomo commented Jun 6, 2023

pcahyna commented Jun 6, 2023

schlomo commented Jun 6, 2023

pcahyna commented Jun 6, 2023

schlomo commented Jun 6, 2023

pcahyna commented Jun 6, 2023

pcahyna commented Jun 2, 2023 •

edited

schlomo commented Jun 5, 2023 •

edited