Created attachment 1756051 [details] spec file URL: https://github.com/intel/eth-fast-fabric SPEC and SRPM: Please clone https://github.com/intel/eth-fast-fabric against tag 11.0.0.0-pre, and then follow README to generate spec and srpm file. Description: Intel Ethernet Fabric Suite basic tools and libraries for fabric management eth-fast-fabric is based on opa-ff. I have followed https://bugzilla.redhat.com/show_bug.cgi?id=1333531 and fixed issues commented in the ticket.
I don't know why I'm assigned this, but I'm not intending to review this.
- Any reason not to use the tarball from GitHub directly? # tarball created by: # git clone -b <tagname> https://github.com/intel/eth-fast-fabric.git # cd eth-fast-fabric # tar czf <workspace>/eth-tools.tgz --exclude-vcs --transform "s,^,eth-tools-$(grep '^Version:' eth-tools.spec.in | cut -d' ' -f2)/," . Source: eth-tools.tgz Why not: Version: 11.0.0.0~pre […] Source: %url/archive/%{version_no_tilde}/%{name}-%{version_no_tilde}.tar.gz - Libraries required should be autodetected: Requires: rdma Requires: expect%{?_isa}, tcl%{?_isa}, openssl%{?_isa}, expat%{?_isa}, libibumad%{?_isa}, libibverbs%{?_isa}, net-snmp%{?_isa}, net-snmp-utils%{?_isa} - Please split your BuildRequires one per line: BuildRequires: expat-devel, gcc-c++, openssl-devel, ncurses-devel, tcl-devel, zlib-devel, rdma-core-devel, ibacm-devel, net-snmp-devel - Why do you need to set Epoch? - You mist respect the default Fedora CFLAGS and LDFLAGS during the build. It doesn't seem that your build script respect them. - What is $BUILD_ARGS? - 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 - You must use macros for directories: /usr/share/ → %{_datadir} /usr/src/ → %{_usrsrc} /usr/lib → %{_prefix}/lib - Simplify this in %files fastfabric %{_datadir}/eth-tools/ %exclude %{_datadir}/eth-tools/samples/mgt_config.xml-sample - Glob man pages extension as the compression might change in the future: %{_mandir}/man1/ethcapture.1* and so on - Just glob the man pages, no need to to specify all files manually: %{_mandir}/man8/eth*.8* - Just include the whole directory too %{_usrsrc}/eth/ - own %{_sysconfdir}/eth-tools in basic %dir %{_sysconfdir}/eth-tools - What are the files in /usr/lib/eth-tools ? They don't seem to be libraries. - 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. - Add a BuildRequires for make - Separate your changelog entries by a newline - I assume this is a typo and that you meant openssl: BuildRequires: penssl-devel - Release should start at 1 - Changelog should contain the release after the version: %changelog * Fri Feb 05 2021 Jijun Wang <jijun.wang> - 11.0.0.0~pre-1 - Cleaned up for upstream * Mon Feb 26 2018 Jijun Wang <jijun.wang> - 10.8.0.0-1 - Added epoch for RHEL address-resolution, basic and fastfabric - Added component information in description for all rpms * Thu Apr 13 2017 Scott Breyer <scott.j.breyer> - 10.5.0.0-1 - Updates for spec file cleanup * Fri Oct 10 2014 Erik E. Kahn <erik.kahn> - 1.0.0-ifs-1 - Initial version - Here's a cleaned up SPEC. You still ne to explain the preun thing, the files in /usr/lib, and how to çake the build respect Fedora's default build flags (CFLAGS, CXXFLAGS, LDFLAGS). Name: eth-tools Version: 11.0.0.0~pre Release: 1%{?dist} Summary: Intel Ethernet Fabric Suite basic tools and libraries for fabric management License: BSD Url: https://github.com/intel/eth-fast-fabric Source: %url/archive/%{version_no_tilde}/%{name}-%{version_no_tilde}.tar.gz # The Intel(R) Ethernet Fabric Suite product line is only available on x86_64 # platforms at this time. ExclusiveArch: x86_64 %description This package contains the tools necessary to manage an Intel Ethernet fabric. %package basic Summary: Management level tools and scripts Requires: bc BuildRequires: expat-devel BuildRequires: gcc-c++ BuildRequires: make BuildRequires: openssl-devel BuildRequires: ncurses-devel BuildRequires: tcl-devel BuildRequires: zlib-devel BuildRequires: rdma-core-devel BuildRequires: ibacm-devel BuildRequires: net-snmp-devel %description basic Contains basic tools for fabric managment necessary on all compute nodes. %package fastfabric Summary: Management level tools and scripts Requires: eth-tools-basic%{?_isa} >= %{version}-%{release} Requires: cronie %description fastfabric Contains tools for managing fabric on a management node. %prep %autosetup -n eth-fast-fabric-%{version_no_tilde} %build %set_build_flags cd OpenIb_Host OPA_FEATURE_SET= ./ff_build.sh %{_builddir} $BUILD_ARGS %install BUILDDIR=%{_builddir} DESTDIR=%{buildroot} LIBDIR=%{_prefix}/lib DSAP_LIBDIR=%{_libdir} ./OpenIb_Host/ff_install.sh %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. %files basic %{_sbindir}/ethcapture %{_prefix}/lib/eth-tools/setup_self_ssh %{_prefix}/lib/eth-tools/usemem %{_prefix}/lib/eth-tools/ethipcalc %{_prefix}/lib/eth-tools/stream %{_mandir}/man1/ethcapture.1* %{_datadir}/eth-tools/samples/mgt_config.xml-sample %dir %{_sysconfdir}/eth-tools/ %config(noreplace) %{_sysconfdir}/eth-tools/mgt_config.xml %files fastfabric %{_sysconfdir}/eth-tools/ethmon.si.conf # Replace ethmon.si.conf, as it's a template config file. %config(noreplace) %{_sysconfdir}/eth-tools/ethfastfabric.conf %config(noreplace) %{_sysconfdir}/eth-tools/ethmon.conf %config(noreplace) %{_sysconfdir}/eth-tools/allhosts %config(noreplace) %{_sysconfdir}/eth-tools/chassis %config(noreplace) %{_sysconfdir}/eth-tools/hosts %config(noreplace) %{_sysconfdir}/eth-tools/switches %{_sbindir}/* %exclude %{_sbindir}/ethcapture %{_prefix}/lib/eth-tools/* %config(noreplace) %{_prefix}/lib/eth-tools/osid_wrapper %exclude %{_prefix}/lib/eth-tools/setup_self_ssh %exclude %{_prefix}/lib/eth-tools/usemem %exclude %{_prefix}/lib/eth-tools/ethipcalc %exclude %{_prefix}/lib/eth-tools/stream %{_datadir}/eth-tools/ %exclude %{_datadir}/eth-tools/samples/mgt_config.xml-sample %{_mandir}/man8/eth*.8* %{_usrsrc}/eth/ %changelog * Fri Feb 05 2021 Jijun Wang <jijun.wang> - 11.0.0.0~pre-1 - Cleaned up for upstream * Mon Feb 26 2018 Jijun Wang <jijun.wang> - 10.8.0.0-1 - Added epoch for RHEL address-resolution, basic and fastfabric - Added component information in description for all rpms * Thu Apr 13 2017 Scott Breyer <scott.j.breyer> - 10.5.0.0-1 - Updates for spec file cleanup * Fri Oct 10 2014 Erik E. Kahn <erik.kahn> - 1.0.0-ifs-1 - Initial version
- This: export CLOCAL="%build_cflags" export CCLOCAL="%build_cxxflags" export LDLOCAL="%build_ldflags" seems to work for Fedora default flags
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.
(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.
Please see my response below. I also updated github with the suggested changes, and did a koji build (https://koji.fedoraproject.org/koji/taskinfo?taskID=63926205). >> 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. Got what you meant. Changed spec file to use referencing source. >> 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? Removed %preun. A user supposes copy mpi-apps src to another location and build. The make clean here is unnecessary. Below is the generated spec file: Name: eth-tools Version: 11.0.0.0~pre Release: 163%{?dist} Summary: Intel Ethernet Fabric Suite basic tools and libraries for fabric management License: BSD Url: https://github.com/intel/eth-fast-fabric Source: %url/archive/%{version_no_tilde}/eth-fast-fabric-%{version_no_tilde}.tar.gz ExclusiveArch: x86_64 # The Intel(R) Ethernet Fabric Suite product line is only available on x86_64 platforms at this time. %description This package contains the tools necessary to manage an Intel Ethernet fabric. %package basic Summary: Management level tools and scripts Requires: rdma bc Requires: expect%{?_isa}, tcl%{?_isa}, net-snmp-utils%{?_isa} BuildRequires: make BuildRequires: expat-devel BuildRequires: gcc-c++ BuildRequires: openssl-devel BuildRequires: ncurses-devel BuildRequires: tcl-devel BuildRequires: zlib-devel BuildRequires: rdma-core-devel BuildRequires: ibacm-devel BuildRequires: net-snmp-devel Epoch: 1 %description basic Contains basic tools for fabric management necessary on all compute nodes. %package fastfabric Summary: Management level tools and scripts Requires: eth-tools-basic%{?_isa} >= %{version}-%{release} Requires: cronie Epoch: 1 %description fastfabric Contains tools for managing fabric on a management node. %prep %autosetup -n eth-fast-fabric-%{version_no_tilde} %build cd OpenIb_Host OPA_FEATURE_SET= CLOCAL='%build_cflags' CCLOCAL='%build_cxxflags' LDLOCAL='%build_ldflags' ./ff_build.sh %{_builddir} %install BUILDDIR=%{_builddir} DESTDIR=%{buildroot} LIBDIR=%{_prefix}/lib DSAP_LIBDIR=%{_libdir} ./OpenIb_Host/ff_install.sh %files basic %{_sbindir}/ethcapture %{_prefix}/lib/eth-tools/setup_self_ssh %{_prefix}/lib/eth-tools/usemem %{_prefix}/lib/eth-tools/ethipcalc %{_prefix}/lib/eth-tools/stream %{_mandir}/man1/ethcapture.1* %{_datadir}/eth-tools/samples/mgt_config.xml-sample %dir %{_sysconfdir}/eth-tools/ %config(noreplace) %{_sysconfdir}/eth-tools/mgt_config.xml %files fastfabric %{_sbindir}/* %exclude %{_sbindir}/ethcapture %{_prefix}/lib/eth-tools/* %exclude %{_prefix}/lib/eth-tools/setup_self_ssh %exclude %{_prefix}/lib/eth-tools/usemem %exclude %{_prefix}/lib/eth-tools/ethipcalc %exclude %{_prefix}/lib/eth-tools/stream %{_datadir}/eth-tools/* %exclude %{_datadir}/eth-tools/samples/mgt_config.xml-sample %{_mandir}/man8/eth*.8* %{_usrsrc}/eth/* %{_sysconfdir}/eth-tools/ethmon.si.conf # Replace ethmon.si.conf, as it's a template config file. %config(noreplace) %{_sysconfdir}/eth-tools/ethfastfabric.conf %config(noreplace) %{_sysconfdir}/eth-tools/ethmon.conf %config(noreplace) %{_sysconfdir}/eth-tools/allhosts %config(noreplace) %{_sysconfdir}/eth-tools/chassis %config(noreplace) %{_sysconfdir}/eth-tools/hosts %config(noreplace) %{_sysconfdir}/eth-tools/switches %config(noreplace) /usr/lib/eth-tools/osid_wrapper %changelog * Fri Feb 05 2021 Jijun Wang <jijun.wang> - 11.0.0.0-163 - Cleaned up for upstream * Mon Feb 26 2018 Jijun Wang <jijun.wang> - 10.8.0.0-1 - Added epoch for RHEL address-resolution, basic and fastfabric - Added component information in description for all rpms * Thu Apr 13 2017 Scott Breyer <scott.j.breyer> - 10.5.0.0-1 - Updates for spec file cleanup * Fri Oct 10 2014 Erik E. Kahn <erik.kahn> - 1.0.0-ifs-1 - Initial version
Package approved. You still need to find a sponsor to be in the packager group, see https://fedoraproject.org/wiki/How_to_get_sponsored_into_the_packager_group
Sponsored.
(fedscm-admin): The Pagure repository was created at https://src.fedoraproject.org/rpms/eth-fast-fabric
This package has: Name: eth-tools in the spec file, but the name added to dist-git/this-review/the thing added to koji is: eth-fast-fabric This does not work with our tooling. You need to either adjust the spec file to use Name: eth-fast-fabric or retire that package and re-review this under the name 'eth-tools'
(In reply to Kevin Fenzi from comment #10) > This package has: > > Name: eth-tools > > in the spec file, but the name added to dist-git/this-review/the thing added > to koji is: eth-fast-fabric > > This does not work with our tooling. > > You need to either adjust the spec file to use Name: eth-fast-fabric or > retire that package and re-review this under the name 'eth-tools' Thanks for the quick response. Discussed with my team, we will "retire" eth-fast-fabric and ask re-review under the name eth-tools.
(fedscm-admin): The Pagure repository was created at https://src.fedoraproject.org/rpms/eth-tools
*** Bug 1943670 has been marked as a duplicate of this bug. ***
@Jijun, please also build the package for Fedora 34. thanks
FEDORA-2021-ceac7860ec has been submitted as an update to Fedora 34. https://bodhi.fedoraproject.org/updates/FEDORA-2021-ceac7860ec
(In reply to Honggang LI from comment #14) > @Jijun, please also build the package for Fedora 34. thanks Build the package for f34 (https://koji.fedoraproject.org/koji/taskinfo?taskID=65308893) just before the final freeze. Hopefully it's in f34.
(In reply to Jijun Wang from comment #16) > (In reply to Honggang LI from comment #14) > > @Jijun, please also build the package for Fedora 34. thanks > > Build the package for f34 > (https://koji.fedoraproject.org/koji/taskinfo?taskID=65308893) just before > the final freeze. Hopefully it's in f34. eth-tools for f34 is still under "pending->testing" state. Not sure what I can do on my side. I noticed F34 will have a final go/no-go meeting next week on 04/15.
FEDORA-2021-ceac7860ec has been pushed to the Fedora 34 testing repository. Soon you'll be able to install the update with the following command: `sudo dnf install --enablerepo=updates-testing --advisory=FEDORA-2021-ceac7860ec \*` You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2021-ceac7860ec See also https://fedoraproject.org/wiki/QA:Updates_Testing for more information on how to test updates.
(In reply to Jijun Wang from comment #17) > eth-tools for f34 is still under "pending->testing" state. Not sure what I > can do on my side. I noticed F34 will have a final go/no-go meeting next > week on 04/15. After eth-tools fc34 build passed the test, you will receive a notification. Then you can push it into stable repo via the URL in the notification email.
FEDORA-2021-ceac7860ec has been pushed to the Fedora 34 stable repository. If problem still persists, please make note of it in this bug report.