https://bugzilla.redhat.com/show_bug.cgi?id=1927033 --- Comment #5 from Robert-André Mauchin 🐧 <zebob.m@xxxxxxxxx> --- (In reply to Jijun Wang from comment #4) > 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. Ok > So we cannot directly use the source > tarball. > I don't follow. Even if you generate the spe file with a script, why can't you use your tool to generate the correct archive url? Using macro like %{version} would automatically keep the archive up to date. I don't understand why you need to use a manually created archive while the github url would work as much. > - 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. > Could you use the %ghost directive to reset the files generated by the user that are not present at install? > - 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. > I believe the 13 should be part of the version not release, its ok for now but please reset it next release. -- 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
