Subject: Re: bug#57608: Upstreaming KDE Plasma and rest of packages

From: Marius Bakke <[email-masked]>
Date: Tue, 22 Nov 2022 11:56:38 +0100

phodina <[email-masked]> skriver:

> Hi Marius,
>
> Thanks for the review and push to master!
>
>> Awesome work Petr. :-)
>> 
>> I went through the branch and applied most of the patches. With a few
>> changes:
>> 
>> * shortened multi-line URLs
>
> Sorry I used the formatter ('guix style') and it set the URLs as such).

Right.  It still has a few quirks that needs to be inched out...

>> * added (file-name ...) for git sources
>> * removed knetworkmanager -- AFAICT it is identical to networkmanager-qt
>> * switched to git-fetch for packages that were downloading tarballs from
>> the KDE GitLab. This is because such autogenerated tarballs are not
>> stable: they may get regenerated with a different hash.
>
> Is this issue somewhere described? I've lately used more tarballs to git download on other packages. Is the issue connected with any Gitlab/Forge or just the KDE and the release scripts?

We have this problem with all the forges.  There have been many-a bug
report about it, and the linter also warns about it (apparently for
GitHub only, that could be improved).

>> * dropped the !! commits
>> * Minor tweaks to synopses and descriptions
>
> My apologies as I noticed there were few packages with wrong license or missing description.

Oh, I did not license audit these packages as I assumed all the KDE
stuffs were using the same license.  Can you submit patches to update
the licenses?

>> * A few commits had a random edit to a different package: I reverted
>> those edits. An example:
>> https://github.com/phodina/guix/commit/5eaa9c49a78eed419db7847668a55c079bad5b71
>
> Caused due to rebasing and working on a large patchset. I'll split it next time and sent it by smaller parts.

No worries; this is what reviews are for.  :-)

>> * Removed use of direct variable references, i.e. #$qtbase. Always use
>> (search-input-file ...), (search-input-directory ...) or as a last
>> resort #$(this-package-input "foo").
>
> Thanks for the hint. I'll fix all the packages where I use this syntax!
>  
>> * Skipped commits that would trigger a lot of rebuilds, e.g. gpgme.
>
> I'll submit it in new ticket.

Note: I added a new version of gpgme and used it for the packages that
require it.  We already have the newest version on 'core-updates'.

>> * Skipped cosmetic commits such as using G-expressions in Qt packages;
>> mainly because of rebuilds, but also because they were not indented
>> properly. Some also introduced direct #$variable references.
>
> I'll fix the issues and submit it as a new ticket.

Great.

>> * Dropped the plasma-desktop-service since it was not working for me.
>
> Should we keep this ticket open to package also the plasma-desktop-service?
> I'll try it now and see what causes the Plasma not to work.

Let's keep this ticket open until we get past the finish line.  :-)

>> For later reference, when adding patches, please add a sentence or two
>> at the top of the patch describing what it does. I did not edit the
>> patches (except for a long file name), because I did not know what it
>> was for. Presumably you did; future you will thank you!
>> 
>> Pushed to 'master' as fe3be8d5e0..82804298ad !
>
> Sorry I didn't intend to add so many changes and it's been a challenge that taught me some new lessons.

It's easy to get lost in a huge patchset.  You did great!

> I'll definitely add more description for the reviewers and keep it smaller so I doesn't take long time to review.

Bite-sized PRs are more likely to get through, but I'm not sure we had a
lot of choice here given the scope of the project.

Ultimately someone just needs to muster enough courage to push it
forward.