Bug 2041917 - Review Request: gpuvis - GPU Trace Visualizer
Summary: Review Request: gpuvis - GPU Trace Visualizer
Keywords:
Status: CLOSED NOTABUG
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
unspecified
medium
Target Milestone: ---
Assignee: Nobody's working on this, feel free to take it
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks: FE-DEADREVIEW
TreeView+ depends on / blocked
 
Reported: 2022-01-18 14:16 UTC by Dorinda
Modified: 2024-02-19 00:45 UTC (History)
4 users (show)

Fixed In Version:
Clone Of:
Environment:
Last Closed: 2024-02-19 00:45:25 UTC
Type: ---
Embargoed:


Attachments (Terms of Use)

Description Dorinda 2022-01-18 14:16:01 UTC
Spec URL: https://raw.githubusercontent.com/dorindabassey/gpuvis/master/rpm/gpuvis.spec
SRPM URL: https://download.copr.fedorainfracloud.org/results/dorinda/gpuvis/fedora-34-x86_64/03491033-gpuvis/gpuvis-0.1-1.fc34.src.rpm
Description: Gpuvis is a Linux GPU profiler similar to GPUView on Windows.
Fedora Account System Username: dorinda

Comment 3 Elliott Sales de Andrade 2022-01-19 10:30:41 UTC
> Source1:	https://github.com/Tencent/rapidjson/archive/1c2c8e085a8b2561dff17bedb689d2eb0609b689.tar.gz

rapidjson is already packaged; why use a bundled copy?


> Source2:	https://github.com/dorindabassey/gpuvis/blob/master/rpm/%{name}.desktop
> Source3:	https://github.com/dorindabassey/gpuvis/blob/master/rpm/%{name}.metainfo.xml

You should send these files upstream.


> #-march=native option in makefile not supported on powerpc use -mtune=native
> %ifarch ppc64le
> sed -i 's/-march=native/-mtune=native/g' Makefile
> %endif

Packages cannot use -march=native; they must use standard compiler flags. You could fix the Makefile, but ...


> %build
> mkdir -p lib/rapidjson
> tar --strip-components=1 -xvf %{SOURCE1} -C lib/rapidjson
> make %{?_smp_mflags} USE_GTK3=0 ASAN=0 CFG=debug
> %make_build

... why not use Meson and the Meson build macros?
https://docs.fedoraproject.org/en-US/packaging-guidelines/Meson/


> %install
> #makefile has no install target so write install in the spec file

The Meson build will know how to install.

Comment 4 Dorinda 2022-01-19 18:29:09 UTC
Thank you for the review. Made some update to the spec file
link to successful koji builds:
https://koji.fedoraproject.org/koji/taskinfo?taskID=81461337
https://koji.fedoraproject.org/koji/taskinfo?taskID=81461323

Comment 5 Elliott Sales de Andrade 2022-01-25 11:09:10 UTC
> %global rapidjson_version 1.1.0
> %global rapidjson_release 16

Doesn't seem to be any need of these globals, as there is only one use of them, so they can just be inlined. Also, it'd be very surprising if you really need a specific release.

I think there's a bug in upstream's meson files as they don't appear to check for rapidjson?

> Source1:	%{name}.desktop
> Source2:	%{name}.metainfo.xml

These are in v0.1 tarball now, aren't they?

> BuildRequires:  desktop-file-utils libappstream-glib rapidjson

Already depending on rapidjson-devel, so no need for rapidjson too.

> BuildRequires:  meson SDL2-devel freetype-devel gtk3-devel libstdc++-static libstdc++-devel glibc-static pkg-config

Doubtful that the *-static files are needed. libstdc++-devel will be required by g++, so not needed.

Comment 6 Dorinda 2022-01-27 12:09:29 UTC
> These are in v0.1 tarball now, aren't they?

Yes those sources are in v0.1

> I think there's a bug in upstream's meson files as they don't appear to check for rapidjson?

Thanks for that highlight, it appear that the person that added rapidjson dependency in makefile and cmake didn't setup the meson build system for this yet. ref- https://github.com/mikesart/gpuvis/pull/64
so I've sent a fix upstream. applied patches to spec file.

Comment 8 Petr Menšík 2023-01-18 20:52:36 UTC
Note: reviews are usually processed by automated tool fedora-review. That uses Spec URL: and SRPM URL: lines to obtain sources and attempt builds on them.

These should lead to permanent store, which would remain accessible until the review is passed. Unfortunately scratch builds do not work this way, their built artifacts are deleted after few days. Use fedorapeople.org server to store your srpm if you do not have better place.

It is not clear where I could find the most recent srpm file. Because you use additional sources, spectool -g *.spec cannot download they just from provided spec file. Please update srpm to match your latest changes. Are URLs in comment #0 still valid and contain the latest source to review? Fedora 34 tag is suscpicious. Please add new comment with links to working files, which do not get autodeleted after 3 days. Scratch builds won't work, sorry.

(In reply to Dorinda from comment #6)
> > These are in v0.1 tarball now, aren't they?
> 
> Yes those sources are in v0.1
> 
> > I think there's a bug in upstream's meson files as they don't appear to check for rapidjson?
> 
> Thanks for that highlight, it appear that the person that added rapidjson
> dependency in makefile and cmake didn't setup the meson build system for
> this yet. ref- https://github.com/mikesart/gpuvis/pull/64
> so I've sent a fix upstream. applied patches to spec file.

It is a good practice to include such changes in Patch1: links and ideally provide link to its pull request in comment above the patch. Do not see that in linked spec.

Comment 9 Petr Menšík 2023-01-18 21:09:04 UTC
Consider linking extra sources from upstream source, where it was merged (thanks to you).

Source1:        %{url}/raw/3fe8fa89117ecb7aec591699ccb1e89f2fb80a2c/com.github.%{name}.Gpuvis.desktop
Source2:        %{url}/raw/3fe8fa89117ecb7aec591699ccb1e89f2fb80a2c/com.github.%{name}.Gpuvis.metainfo.xml
Source3:        %{url}/raw/3fe8fa89117ecb7aec591699ccb1e89f2fb80a2c/com.github.%{name}.Gpuvis.png

But since it uses custom commit instead of release version tag, then just remove Source1-Source3 and use commit at least 3fe8fa89117ecb7aec591699ccb1e89f2fb80a2c, which contains these extra files already in archive. Elliot already pointed to it in comment #5. Just adjust it from .svn to .png format during installation.

Comment 10 Package Review 2024-01-19 00:45:27 UTC
This is an automatic check from review-stats script.

This review request ticket hasn't been updated for some time. We're sorry
it is taking so long. If you're still interested in packaging this software
into Fedora repositories, please respond to this comment clearing the
NEEDINFO flag.

You may want to update the specfile and the src.rpm to the latest version
available and to propose a review swap on Fedora devel mailing list to increase
chances to have your package reviewed. If this is your first package and you
need a sponsor, you may want to post some informal reviews. Read more at
https://fedoraproject.org/wiki/How_to_get_sponsored_into_the_packager_group.

Without any reply, this request will shortly be considered abandoned
and will be closed.
Thank you for your patience.

Comment 11 Package Review 2024-02-19 00:45:25 UTC
This is an automatic action taken by review-stats script.

The ticket submitter failed to clear the NEEDINFO flag in a month.
As per https://fedoraproject.org/wiki/Policy_for_stalled_package_reviews
we consider this ticket as DEADREVIEW and proceed to close it.


Note You need to log in before you can comment on or make changes to this bug.