Bug 2175818

Taking this review.

I did some minor changes :)

SPEC Url: https://download.copr.fedorainfracloud.org/results/aekoroglu/fedora/fedora-rawhide-x86_64/05602818-input-leap/input-leap.spec
SRPM Url: https://download.copr.fedorainfracloud.org/results/aekoroglu/fedora/fedora-rawhide-x86_64/05602818-input-leap/input-leap-2.4.0-0.1.20230306gite71fbbfe.fc39.src.rpm

Created attachment 1948668 [details]
The .spec file difference from Copr build 5598422 to 5602834

Initial spec review:

> Version:		2.4.0
> %if %{with_snapshot}
> Release:		%autorelease -p -s %{gitdate}git%{shortcommit}
> %else
> Release:		%autorelease

Please move the snapshot versioning to "Version", like so: "2.4.0%{?with_snapshot:^%{gitdate}git%{shortcommit}}"

Cf. https://docs.fedoraproject.org/en-US/packaging-guidelines/Versioning/#_snapshots

> 	-DINPUTLEAP_BUILD_TESTS=OFF \
> 	-DINPUTLEAP_USE_EXTERNAL_GTEST=false

We do have gtest-devel for this...

> desktop-file-install --delete-original \
> 	--dir %{buildroot}%{_datadir}/applications \
> 	--set-icon=%{_datadir}/icons/hicolor/scalable/apps/%{name}.svg \
> 	%{buildroot}%{_datadir}/applications/%{name}.desktop

I looked at the original desktop file, and I don't see a reason to rewrite the file? https://github.com/input-leap/input-leap/blob/master/res/input-leap.desktop

Can we just replace this with desktop-file-validate in %check?

Cf. https://docs.fedoraproject.org/en-US/packaging-guidelines/#_desktop_file_install_usage

We also want validation of the AppStream metainfo file.

Cf. https://docs.fedoraproject.org/en-US/packaging-guidelines/AppData/

Shouldn't this package *replace* barrier with something like Obsoletes?

(In reply to Brian J. Murrell from comment #5)
> Shouldn't this package *replace* barrier with something like Obsoletes?

Just to clarify why... but my understanding is that upstream barrier's development has all moved to input-leap and thus barrier is effectively abandoned and accordingly, packaging effort for barrier has also ceased with the packaging effort for this package taking over where that stopped.  That seems to me to be exactly what Obsoletes was created for.

It means people will have their barrier upgraded to input-leap once the package is in the repo without people needing to know that they would otherwise have to manually remove barrier and manually install input-leap.  I would guess that most people are likely never going to realize that barrier is effectively dead and they this manual upgrade step is necessary.

Spec file has changed as Neal requested.

SPEC Url: https://download.copr.fedorainfracloud.org/results/aekoroglu/fedora/fedora-rawhide-x86_64/05603716-input-leap/input-leap.spec
SRPM Url: https://download.copr.fedorainfracloud.org/results/aekoroglu/fedora/fedora-rawhide-x86_64/05603716-input-leap/input-leap-2.4.0%5E20230306gite71fbbfe-1.fc39.src.rpm

Still no Obsoletes and no comment about it either way.

Created attachment 1948799 [details]
The .spec file difference from Copr build 5602834 to 5603815

Copr build:
https://copr.fedorainfracloud.org/coprs/build/5603815
(succeeded)

Review template:
https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2175818-input-leap/fedora-rawhide-x86_64/05603815-input-leap/fedora-review/review.txt

Please take a look if any issues were found.

---
This comment was created by the fedora-review-service
https://github.com/FrostyX/fedora-review-service

If you want to trigger a new Copr build, add a comment containing new
Spec and SRPM URLs or [fedora-review-service-build] string.

Hello Brian, can we discuss this after the package approval ?

Hi Ali.

It seems like adding Obsoletes: would be part of the package approval process (it's going to modify the specfile after all) and discussing it afterward would be like discussing how to get the toothpaste out of the tube after it's already all been squeezed out.

Or am I misunderstanding something?

No, the package replacement needs to be part of this package review, since it is intended to replace barrier and synergy.

That means we need "Obsoletes+Provides" stanza for barrier (and potentially synergy too...)

Ahh.  Good catch on replacing synergy as well Neal.

Ok, we can obsolete barrier (primary developers moved to inputleap) but not sure about synergy.

https://github.com/symless/synergy-core/commits/master
https://src.fedoraproject.org/rpms/synergy/commits/rawhide

Heh, yes.  I tend to forget that synergy is dead to me, but not everyone.

So I agree that probably only Obsoletes+Provides for barrier is appropriate.

SPEC Url: https://download.copr.fedorainfracloud.org/results/aekoroglu/fedora/fedora-rawhide-x86_64/05615430-input-leap/input-leap.spec
SRPM Url: https://download.copr.fedorainfracloud.org/results/aekoroglu/fedora/fedora-rawhide-x86_64/05615430-input-leap/input-leap-2.4.0%5E20230306gite71fbbfe-1.fc39.src.rpm

Created attachment 1949127 [details]
The .spec file difference from Copr build 5603815 to 5615441

Copr build:
https://copr.fedorainfracloud.org/coprs/build/5615441
(succeeded)

Review template:
https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2175818-input-leap/fedora-rawhide-x86_64/05615441-input-leap/fedora-review/review.txt

Please take a look if any issues were found.

---
This comment was created by the fedora-review-service
https://github.com/FrostyX/fedora-review-service

If you want to trigger a new Copr build, add a comment containing new
Spec and SRPM URLs or [fedora-review-service-build] string.

> %if %{with_snapshot}
> Version:		2.4.0%{?with_snapshot:^%{gitdate}git%{shortcommit}}
> %else
> Version:		2.4.0
> %endif

You can drop the %if conditional. it's redundant.

I've missed that.

SPEC Url: https://download.copr.fedorainfracloud.org/results/aekoroglu/fedora/fedora-rawhide-x86_64/05619358-input-leap/input-leap.spec
SRPM Url: https://download.copr.fedorainfracloud.org/results/aekoroglu/fedora/fedora-rawhide-x86_64/05619358-input-leap/input-leap-2.4.0%5E20230306gite71fbbfe-1.fc39.src.rpm

Created attachment 1949414 [details]
The .spec file difference from Copr build 5615441 to 5619428

Copr build:
https://copr.fedorainfracloud.org/coprs/build/5619428
(succeeded)

Review template:
https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2175818-input-leap/fedora-rawhide-x86_64/05619428-input-leap/fedora-review/review.txt

Please take a look if any issues were found.

---
This comment was created by the fedora-review-service
https://github.com/FrostyX/fedora-review-service

If you want to trigger a new Copr build, add a comment containing new
Spec and SRPM URLs or [fedora-review-service-build] string.

> %cmake . \

You can drop the ".", as it's handled by the %cmake macro now.

SPEC Url: https://download.copr.fedorainfracloud.org/results/aekoroglu/fedora/fedora-rawhide-x86_64/05651699-input-leap/input-leap.spec
SRPM Url: https://download.copr.fedorainfracloud.org/results/aekoroglu/fedora/fedora-rawhide-x86_64/05651699-input-leap/input-leap-2.4.0%5E20230306gite71fbbfe-1.fc39.src.rpm

Created attachment 1951302 [details]
The .spec file difference from Copr build 5619428 to 5651771

Copr build:
https://copr.fedorainfracloud.org/coprs/build/5651771
(succeeded)

Review template:
https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2175818-input-leap/fedora-rawhide-x86_64/05651771-input-leap/fedora-review/review.txt

Please take a look if any issues were found.

---
This comment was created by the fedora-review-service
https://github.com/FrostyX/fedora-review-service

If you want to trigger a new Copr build, add a comment containing new
Spec and SRPM URLs or [fedora-review-service-build] string.

Things look good now!

PACKAGE APPROVED.

The Pagure repository was created at https://src.fedoraproject.org/rpms/input-leap

Thank you guys :)

I do have a COPR project building this for F37->rawhide if anyone wants early access on current-release Fedora.

https://copr.fedorainfracloud.org/coprs/brianjmurrell/input-leap/

dnf copr enable brianjmurrell/input-leap

I've installed it.  Won't be able to test it until tomorrow when I go to work.