#3003 PR `merged`: Improvements of the Makefile distribution targets¶

Labels: enhancement, fixed / solved / done

pcahyna opened issue at 2023-06-02 15:35:¶

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.

pcahyna commented at 2023-06-02 15:36:¶

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

pcahyna commented at 2023-06-02 16:57:¶

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 commented at 2023-06-02 17:17:¶

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

pcahyna commented at 2023-06-02 18:42:¶

@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 commented at 2023-06-05 08:50:¶

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

pcahyna commented at 2023-06-05 12:21:¶

@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 commented at 2023-06-05 12:33:¶

@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 commented at 2023-06-05 17:15:¶

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

pcahyna commented at 2023-06-05 20:39:¶

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 commented at 2023-06-05 21:17:¶

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

schlomo commented at 2023-06-06 06:49:¶

@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 commented at 2023-06-06 06:50:¶

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

pcahyna commented at 2023-06-06 09:32:¶

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

pcahyna commented at 2023-06-06 10:01:¶

only CentOS 6 is still acting up

Then why do I see a green check mark ✔️ in https://github.com/pcahyna/rear/commit/1a842d2b73e985ac8c76480c006647f5b6078247 ? 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 https://github.com/rear/rear/pull/3003/commits/1a842d2b73e985ac8c76480c006647f5b6078247 and above.

pcahyna commented at 2023-06-06 10:12:¶

Can you please try to fix this?

I believe it should be fixed now.

pcahyna commented at 2023-06-06 10:13:¶

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 commented at 2023-06-06 11:12:¶

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 commented at 2023-06-06 11:28:¶

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 commented at 2023-06-06 11:35:¶

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 commented at 2023-06-06 11:39:¶

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 commented at 2023-06-06 11:40:¶

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

pcahyna commented at 2023-06-06 11:41:¶

rerunning to confirm.

pcahyna commented at 2023-06-06 11:43:¶

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 commented at 2023-06-06 11:53:¶

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 commented at 2023-06-06 13:02:¶

@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 commented at 2023-06-06 14:23:¶

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

pcahyna commented at 2023-06-06 16:31:¶

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