#564 Do not download unused sources during command 'sources'
Merged by onosek. Opened by oturpe.
oturpe/rpkg no-unused-sources  into  master

Download 564.patch

Command 'sources' used to download all files
listed in the sources file.
This is waste of resources, because
it is a common packager workflow to
first update the specfile
then get the new source with 'spectool -g *.spec'
and then do 'fedpkg mockbuild'.
In that situation,
thesources file always lists stale files,
downloading those never achieves anything useful.
This commit improves the situation
by avoiding download of files not actually used in the specfile.

The test suite had a strange configuration
where specfile did not have any sources,
but sources file had an entry for 'readme.patch'.
This led to failures with the new feature.
Fixed by adding 'readme.patch' as a source in the specfile.

Resolves #559

Signed-off-by: Otto Urpelainen oturpe@iki.fi

The desired stdout is not completely clear for me.
Before this pull request,
a row was printed for each file that was downloaded.
Since this pull request complicates the things
by creating two different reasons for not downloading a file
I felt it was clearer to print out the result and reason for each file.

Tests need to be updated. I will do that as soon as possible.

rebased onto a6427cd508a9ea52cf3cf2ce24de03f2388bd675

Failing tests fixed and test suite for the new SpecFile class added. This PR is complete now.

Commit 49c0378b fixes this pull-request

Pull-Request has been merged by onosek

Thanks for the contribution.
I did some small changes including one variable rename, description formatting and specfile path modification.

Hi @oturpe
released version rpkg with this feature has some troubles with certain complex specfiles. I was notified about it here:
https://bodhi.fedoraproject.org/updates/FEDORA-2021-30e0273efd
in centpkg fails sources command, because rpmspec is not able to parse grub2.spec (located here https://gitlab.com/redhat/centos-stream/rpms/grub2).
The specfile includes a file with macros - grub.macros and it seems rpmspec is not able to locate it although it is in the same directory.
Do you know, if there is some simple solution for this? Like adding some additional argument to rpmspec -P. Maybe I will have to deactivate the feature with patch until suitable solution is found.

Fallback to the old functionality can look like:

+try:
    specf = SpecFile(os.path.join(self.path, self.spec))
+except rpkgError:
+    specf = None
...
...
+if specf and (entry.file not in specf.sources):
-if (entry.file not in specf.sources):

Uh, sorry about that.
I did not test with local sources at all.
Also, I used spectool as a base when implementing this,
but looked at pagure.io/spectool source.
That is not the spectool that is commonly used,
rather it comes from rpmdevtools.

Anyhow, the fix is not difficult,
I created a pull request #574 for that.

I think there is a bigger issue with this change. It's currently not possible to download sources if the spec file cannot be parsed on a machine where fedpkg sources command runs.

Granted -- this is probably not an issue when it is maintainers/developers running the command on their laptops, but for example Fedora CI runs fedpkg sources when creating SRPMs from pull requests. And the CI machine is CentOS 8.

I opened https://pagure.io/fedora-infrastructure/issue/9728 so we don't need to build SRPMs in CI "manually", but that ticket was blocked before Koji implemented certain missing functionality.

I am wondering if it would make sense to restore the previous behavior via some option, for example: fedpkg sources --force-download or something like that.

To be more specific, this is an example of a spec file for which Fedora CI fails to download sources: https://src.fedoraproject.org/rpms/rust-coreos-installer/blob/rawhide/f/rust-coreos-installer.spec#_18

How about making that fallback automatic?
If the specfile parsing fails,
assume that all sources have to be downloaded.

Just for understanding this problem,
does the problem appear because the CI machine does not know how to evaluate the %{crates_source} macro?

I think there is a bigger issue with this change. It's currently not possible to download sources if the spec file cannot be parsed on a machine where fedpkg sources command runs.

I logged the issue as #583, it is easier to track this that way.
I have a fix ready as I outlined in the previous commit,
I will add it to #581 which fixes yet other problems in the same feature
— the variety of ways rpkg is used has surprised me,
not I am learning it the hard way when all these problems appear.

Thank you for the quick response @oturpe :thumbsup: :wink:

Can this be extended to check if uploaded files are not listed as patches? I have a case where a big patch (700Kb packed, 4,5Mb unpacked) is uploaded into lookaside cache and listed as a patch. This works just fine when building in Koji or Brew, but currently fails in CentOS Stream CI, because the patch is not downloaded and thus not found during build.

Can this be extended to check if uploaded files are not listed as patches? I have a case where a big patch (700Kb packed, 4,5Mb unpacked) is uploaded into lookaside cache and listed as a patch. This works just fine when building in Koji or Brew, but currently fails in CentOS Stream CI, because the patch is not downloaded and thus not found during build.

This was an oversight,
I did not consider storing patches in the lookaside cache originally.
Reported as #584,
the fix is already in the master branch.

Metadata