https://bugzilla.redhat.com/show_bug.cgi?id=2041917
Bug ID: 2041917 Summary: Review Request: gpuvis - GPU Trace Visualizer Product: Fedora Version: rawhide Hardware: All OS: Linux Status: NEW Component: Package Review Severity: medium Assignee: nobody@fedoraproject.org Reporter: dbassey@redhat.com QA Contact: extras-qa@fedoraproject.org CC: package-review@lists.fedoraproject.org Target Milestone: --- Classification: Fedora
Spec URL: https://github.com/dorindabassey/gpuvis/blob/master/rpm/gpuvis.spec SRPM URL: https://github.com/dorindabassey/gpuvis/blob/master/rpm/gpuvis-0-1.fc34.src.... Description: Gpuvis is a Linux GPU profiler similar to GPUView on Windows. Fedora Account System Username: dorinda
https://bugzilla.redhat.com/show_bug.cgi?id=2041917
--- Comment #1 from Dorinda dbassey@redhat.com --- here are links to successful koji build https://koji.fedoraproject.org/koji/taskinfo?taskID=81406892 https://koji.fedoraproject.org/koji/taskinfo?taskID=81406701 https://koji.fedoraproject.org/koji/taskinfo?taskID=81406918 https://koji.fedoraproject.org/koji/taskinfo?taskID=81406196
https://bugzilla.redhat.com/show_bug.cgi?id=2041917
Dorinda dbassey@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Comment|0 |updated
--- Comment #0 has been edited ---
Spec URL: https://raw.githubusercontent.com/dorindabassey/gpuvis/master/rpm/gpuvis.spe... SRPM URL: https://github.com/dorindabassey/gpuvis/blob/master/rpm/gpuvis-0-1.fc34.src.... Description: Gpuvis is a Linux GPU profiler similar to GPUView on Windows. Fedora Account System Username: dorinda
https://bugzilla.redhat.com/show_bug.cgi?id=2041917
Dorinda dbassey@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Comment|0 |updated
--- Comment #0 has been edited ---
Spec URL: https://raw.githubusercontent.com/dorindabassey/gpuvis/master/rpm/gpuvis.spe... SRPM URL: https://github.com/dorindabassey/gpuvis/blob/master/rpm/gpuvis-0-1.20211204.... Description: Gpuvis is a Linux GPU profiler similar to GPUView on Windows. Fedora Account System Username: dorinda
https://bugzilla.redhat.com/show_bug.cgi?id=2041917
--- Comment #2 from Dorinda dbassey@redhat.com --- new koji scratch build update https://koji.fedoraproject.org/koji/taskinfo?taskID=81442752 https://koji.fedoraproject.org/koji/taskinfo?taskID=81442779 https://koji.fedoraproject.org/koji/taskinfo?taskID=81442788 https://koji.fedoraproject.org/koji/taskinfo?taskID=81442806
https://bugzilla.redhat.com/show_bug.cgi?id=2041917
Elliott Sales de Andrade quantum.analyst@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |quantum.analyst@gmail.com Doc Type|--- |If docs needed, set a value
--- Comment #3 from Elliott Sales de Andrade quantum.analyst@gmail.com ---
Source1: https://github.com/Tencent/rapidjson/archive/1c2c8e085a8b2561dff17bedb689d2e...
rapidjson is already packaged; why use a bundled copy?
Source2: https://github.com/dorindabassey/gpuvis/blob/master/rpm/%%7Bname%7D.desktop Source3: https://github.com/dorindabassey/gpuvis/blob/master/rpm/%%7Bname%7D.metainfo...
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.
https://bugzilla.redhat.com/show_bug.cgi?id=2041917
Dorinda dbassey@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Comment|0 |updated
--- Comment #0 has been edited ---
Spec URL: https://raw.githubusercontent.com/dorindabassey/gpuvis/master/rpm/gpuvis.spe... SRPM URL: https://github.com/dorindabassey/gpuvis/blob/master/rpm/gpuvis-0-1.20220119.... Description: Gpuvis is a Linux GPU profiler similar to GPUView on Windows. Fedora Account System Username: dorinda
https://bugzilla.redhat.com/show_bug.cgi?id=2041917
--- Comment #4 from Dorinda dbassey@redhat.com --- 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
https://bugzilla.redhat.com/show_bug.cgi?id=2041917
Dorinda dbassey@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Comment|0 |updated
--- Comment #0 has been edited ---
Spec URL: https://raw.githubusercontent.com/dorindabassey/gpuvis/master/rpm/gpuvis.spe... SRPM URL: https://github.com/dorindabassey/gpuvis/blob/master/rpm/gpuvis-0.1-1.fc34.sr... Description: Gpuvis is a Linux GPU profiler similar to GPUView on Windows. Fedora Account System Username: dorinda
https://bugzilla.redhat.com/show_bug.cgi?id=2041917
Dorinda dbassey@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Comment|0 |updated
--- Comment #0 has been edited ---
Spec URL: https://raw.githubusercontent.com/dorindabassey/gpuvis/master/rpm/gpuvis.spe... SRPM URL: https://github.com/dorindabassey/gpuvis/blob/master/rpm/gpuvis-0.1-1.fc34.sr... Description: Gpuvis is a Linux GPU profiler similar to GPUView on Windows. Fedora Account System Username: dorinda
Update: latest koji scratch build https://koji.fedoraproject.org/koji/taskinfo?taskID=81753216 https://koji.fedoraproject.org/koji/taskinfo?taskID=81753230
https://bugzilla.redhat.com/show_bug.cgi?id=2041917
Dorinda dbassey@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Comment|0 |updated
--- Comment #0 has been edited ---
Spec URL: https://raw.githubusercontent.com/dorindabassey/gpuvis/master/rpm/gpuvis.spe... SRPM URL: https://github.com/dorindabassey/gpuvis/blob/master/rpm/gpuvis-0.1-1.fc34.sr... Description: Gpuvis is a Linux GPU profiler similar to GPUView on Windows. Fedora Account System Username: dorinda
https://bugzilla.redhat.com/show_bug.cgi?id=2041917
Dorinda dbassey@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Comment|0 |updated
--- Comment #0 has been edited ---
Spec URL: https://raw.githubusercontent.com/dorindabassey/gpuvis/master/rpm/gpuvis.spe... SRPM URL: https://github.com/dorindabassey/gpuvis/blob/master/gpuvis-0.1-1.fc34.src.rp... Description: Gpuvis is a Linux GPU profiler similar to GPUView on Windows. Fedora Account System Username: dorinda
https://bugzilla.redhat.com/show_bug.cgi?id=2041917
--- Comment #5 from Elliott Sales de Andrade quantum.analyst@gmail.com ---
%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.
https://bugzilla.redhat.com/show_bug.cgi?id=2041917
Dorinda dbassey@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Comment|0 |updated
--- Comment #0 has been edited ---
Spec URL: https://raw.githubusercontent.com/dorindabassey/gpuvis/master/rpm/gpuvis.spe... SRPM URL: https://copr-be.cloud.fedoraproject.org/results/dorinda/gpuvis/fedora-34-x86... Description: Gpuvis is a Linux GPU profiler similar to GPUView on Windows. Fedora Account System Username: dorinda
https://bugzilla.redhat.com/show_bug.cgi?id=2041917
Dorinda dbassey@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Comment|0 |updated
--- Comment #0 has been edited ---
Spec URL: https://raw.githubusercontent.com/dorindabassey/gpuvis/master/rpm/gpuvis.spe... SRPM URL: https://download.copr.fedorainfracloud.org/results/dorinda/gpuvis/fedora-34-... Description: Gpuvis is a Linux GPU profiler similar to GPUView on Windows. Fedora Account System Username: dorinda
https://bugzilla.redhat.com/show_bug.cgi?id=2041917
--- Comment #6 from Dorinda dbassey@redhat.com ---
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.
https://bugzilla.redhat.com/show_bug.cgi?id=2041917
--- Comment #7 from Dorinda dbassey@redhat.com --- https://koji.fedoraproject.org/koji/taskinfo?taskID=82007245 https://koji.fedoraproject.org/koji/taskinfo?taskID=82006909 https://koji.fedoraproject.org/koji/taskinfo?taskID=82005874 https://koji.fedoraproject.org/koji/taskinfo?taskID=82005299 Links to koji scratch build
https://bugzilla.redhat.com/show_bug.cgi?id=2041917
Dorinda dbassey@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Comment|0 |updated
--- Comment #0 has been edited ---
Spec URL: https://raw.githubusercontent.com/dorindabassey/gpuvis/master/rpm/gpuvis.spe... SRPM URL: https://download.copr.fedorainfracloud.org/results/dorinda/gpuvis/fedora-34-... Description: Gpuvis is a Linux GPU profiler similar to GPUView on Windows. Fedora Account System Username: dorinda
https://bugzilla.redhat.com/show_bug.cgi?id=2041917
Petr Menšík pemensik@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |pemensik@redhat.com
--- Comment #8 from Petr Menšík pemensik@redhat.com --- 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.
https://bugzilla.redhat.com/show_bug.cgi?id=2041917
--- Comment #9 from Petr Menšík pemensik@redhat.com --- 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.
package-review@lists.fedoraproject.org