https://bugzilla.redhat.com/show_bug.cgi?id=1927033 --- Comment #4 from Jijun Wang <jijun.wang@xxxxxxxxx> --- Thanks for the feedback. Please see below for my response. I also updated github with the suggested changes, and did a koji build (https://koji.fedoraproject.org/koji/taskinfo?taskID=63599945). Please take a look and let me know any improvement you want me to do and whether you want me have another pre-release in github. - Any reason not to use the tarball from GitHub directly? eth-fast-fabric is for fabric management. It contains scripts that depend on other system tools. These tools cannot be autodetected as lib requires, so we need to explicitly defines Requires in spec file. In addition, we also need to specify BuildRequires. We intend to support different OSes. Given the lib names can be slightly different on different OSes, we use a script to generate spec file based on OS. So we cannot directly use the source tarball. - Libraries required should be autodetected: Please wee above comment. I also removed the lib reqs that can be autodetected. - Please split your BuildRequires one per line: Changed as suggested. - Why do you need to set Epoch? During opa-ff distro inclusion RH set the epoch to 1. We carried that convention forward in eth-fast-fabric. Since we have released a version of eth-fast-fabric from github with epoch of 1, all future versions must now have epoch 1 or higher otherwise rpm upgrade will misinterpret which rpm is newer. - You mist respect the default Fedora CFLAGS and LDFLAGS during the build. It doesn't seem that your build script respect them. Changed as suggested. - What is $BUILD_ARGS? It’s for build arguments. We needn’t it in spec file. Removed it. - Should be in lib64: > > LIBDIR=%{_lɨbdir} > > The files in /lib looks weird though, it doesn't seem to be libraries? So maybe it's okay. In that case use: > > LIBDIR=%{_prefix}/lib Changed to LIBDIR=%{_prefix}/lib as suggested. - You must use macros for directories: Changed as suggested. - Simplify this in %files fastfabric Changed as suggested. - Glob man pages extension as the compression might change in the future: Changed as suggested. - Just glob the man pages, no need to to specify all files manually: Changed as suggested. - Just include the whole directory too Changed as suggested. - own %{_sysconfdir}/eth-tools in basic Changed as suggested. - What are the files in /usr/lib/eth-tools ? They don't seem to be libraries. They are supporting scripts which are not intended for direct user invocation, but are needed by eth-fast-fabric. We had a similar situation in opa-ff and this is where RH requested we put them. - What's this: > > %preun fastfabric > cd /usr/src/eth/mpi_apps >/dev/null 2>&1 > make -k clean >/dev/null 2>&1 || : # suppress all errors and return codes from the make clean. > > Running make clean in preun on a system directory makes no sense/is suspicious. eth-fast-fabric includes some mpi_apps in source form so end users can select the desired MPI to build them against such as openmpi, Intel MPI, or others. During uninstall, the makefile the user would use to clean the binaries is gone, so best to clean now. - Add a BuildRequires for make Changed as suggested. - Separate your changelog entries by a newline Changed as suggested. - I assume this is a typo and that you meant openssl: > > BuildRequires: penssl-devel Do not see it. Please let me know if it’s still a issue. - Release should start at 1 We have released eth-fast-fabric. The version here intends to be our current release umber + 1, that is 163. - Changelog should contain the release after the version: Changed as suggested. -- You are receiving this mail because: You are on the CC list for the bug. You are always notified about changes to this product and component _______________________________________________ package-review mailing list -- package-review@xxxxxxxxxxxxxxxxxxxxxxx To unsubscribe send an email to package-review-leave@xxxxxxxxxxxxxxxxxxxxxxx Fedora Code of Conduct: https://docs.fedoraproject.org/en-US/project/code-of-conduct/ List Guidelines: https://fedoraproject.org/wiki/Mailing_list_guidelines List Archives: https://lists.fedoraproject.org/archives/list/package-review@xxxxxxxxxxxxxxxxxxxxxxx Do not reply to spam on the list, report it: https://pagure.io/fedora-infrastructure
