update fedora spec file and tarball generation script #1770

anandaravuri · 2020-12-04T16:49:25Z

Tom PR changes "Split tarball generation for rpm generation #1753" in incorporated
#1753

The offical fedora rpm depends on the url source tarball.
Split tarball generation out so it is easier to generate.

Generalize the naming of the tarball to follow the convention
of -.tar.gz.

Fix opae.spec to use the new source loction opae-/

Generalize the tarball excludes all hidden files

Signed-off-by: anandaravuri ananda.ravuri@intel.com

The offical fedora rpm depends on the url source tarball. Split tarball generation out so it is easier to generate. Generalize the naming of the tarball to follow the convention of -.tar.gz. Fix opae.spec to use the new source loction opae-/ Generalize the tarball excludes all hidden files Signed-off-by: anandaravuri <ananda.ravuri@intel.com>

r-rojo · 2020-12-07T20:05:26Z

packaging/opae/rpm/create

+           'python3-virtualenv' \
+           'python3-pip' \
+           'systemd' \
+           'systemd-rpm-macros' \
           'rpmdevtools'
 do
    yum list installed "${pkg}" >/dev/null 2>&1


use dnf instead of yum

- Fix code review comments Signed-off-by: anandaravuri <ananda.ravuri@intel.com>

Signed-off-by: anandaravuri <ananda.ravuri@intel.com>

packaging/opae/rpm/create

Signed-off-by: anandaravuri <ananda.ravuri@intel.com>

r-rojo · 2020-12-10T18:24:53Z

opae.spec

 License:        BSD
 ExclusiveArch:  x86_64

 Group:          Development/Libraries
 Vendor:         Intel Corporation
 Requires:       uuid, json-c, python3
 URL:            https://github.com/OPAE/%{name}-sdk
-Source0:        https://github.com/OPAE/opae-sdk/releases/download/%{version}-1/%{name}.tar.gz
+Source0:        https://github.com/OPAE/opae-sdk/releases/download/%{version}-1/%{name}-%{version}.tar.gz


I think the '1' in %{version}-1 should be replaced by a variable.
Perhaps using %{release} or %{Release} could work since the release is set in line 4.

opae.spec

gwencasey96 · 2020-12-11T19:43:24Z

Hi Ananda, Tim and Rodrigo, If it's alright with you I'm reviewing 1770 in this email. We were able to use the clean script to build successfully on Fedora 32. However we did have a couple of problems when we tried to use the script on Fedora rawhide. ex/ There is failure in the build log tools/extra/fpgadiag/cmakeFiles/eth-group.dir/eth-group.cpp.o" Which looks like a new compiler warning is causing an error. Is it necessary to resolve these problems for this pr or is this something for later ? Please let me know if you have any questions about testing this on rawhide. Thank you Gwen

…

On Thu, Dec 10, 2020 at 5:43 PM Tim Whisonant ***@***.***> wrote: ***@***.**** approved this pull request. — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub <#1770 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ARY5FTJQO22GJ4UNAQ5I663SUFFG5ANCNFSM4UNV2YPQ> .

r-rojo · 2020-12-15T00:08:47Z

packaging/opae/rpm/create

+rm -rf _build
+mkdir _build
+cd _build
+cmake .. -DOPAE_BUILD_LEGACY=ON  -DOPAE_BUILD_TESTS=ON


This command is erroring out because line 88 changes directory to the parent of the codebase root directory (${SDK_DIR}).
A better way would be to not change directories and use source/build paths when calling cmake.

CMAKE_BUILD_DIR="${SDK_DIR}/_build" rm -rf ${CMAKE_BUILD_DIR} mkdir ${CMAKE_BUILD_DIR} cmake -S ${SDK_DIR} -B ${CMAKE_BUILD_DIR

r-rojo · 2020-12-15T00:14:50Z

packaging/opae/rpm/create


 cd "${SCRIPT_DIR}"
 cp ./opae.spec "${RPMBUILD_DIR}/SPECS"

 ${SDK_DIR}/packaging/changelog.py rpm \
    --git-dir="${SDK_DIR}/.git" \
    --project="opae" \
-    --project-version="${PROJECT_VERSION}" \
+    --project-version="${version}" \
    >> "${RPMBUILD_DIR}/SPECS/opae.spec"


This appends the latest changelog commits to the the end of the opae.spec copy which violates the requirement that the changelog be in reverse descending order.
We either need to insert the changelog commits in opae.spec or use the output of the this script to insert it in the appropriate location. I would recommend we do the former (committing it in opae.spec) instead of calling it in create. Doing it this way also allows for the creation of the RPM(s) depend only on the tarball and the .spec file itself.
It would looks something like the following (note, I had to manually set the release in this text as this changelog script only uses the version):

* Mon Dec 14 2020 The OPAE Dev Team <opae@lists.01.org> - 2.0.0-2 - fix code review comments - fix coding style - add create tarball script - update fedora spec file and tarball generation script

The idea was that there should be no changelog stored in opae.spec (the last thing in the file would be the %changelog line). This way, we don't need to maintain super long changelogs, and we're not shipping changelogs for releases where they don't apply.

it's not fixed because this script is still appending changelog to the opae.spec copy in ${RPMBUILD_DIR}/SPECS.

${RPMBUILD_DIR}/SPECS add change log specific to PR

%changelog-> Release info, highlights of this release

a change log is required in the fedora project so when this spec file is moved there at the least the release builds will need to be tracked

fixed
Remove ${RPMBUILD_DIR}/SPECS from RPM create file.
change log hard coded in opae.spec file

r-rojo · 2020-12-15T00:17:17Z

opae.spec

-         fpgad\
-         fpgad-api\
-         fpgad-vc\
+%cmake .. -DCMAKE_INSTALL_PREFIX=/usr  -DOPAE_PRESERVE_REPOS=ON -DOPAE_BUILD_LEGACY=ON -DOPAE_BUILD_SAMPLES=ON -B $PWD


We need to specify python version as 3.9 as it defaults to 2.7.

-DOPAE_PYTHON_VERSION=3.9

by default rpm creation process uses python version 3.9 , no need to set python version in cmake

This is to tell OPAE build system which Python to look for. This default used to be 2.7 but was changed to 3.6 before it was changed to 3

Could the cmake infra auto detect the python version and bail when it is not supported ? I am thinking of the use case where 3.8 would also work and the user would need to know to upgrade to 3.9 and to use 3.9 in the configure step

By adding -DOPAE_PYTHON_VERSION=3.9 we are essentially telling cmake to bail if the Python that is found is not 3.9.
That is why we should change it from 2.7 to 3.9. We could also omit the version from the call to find_package but I'd rather not and be explicit about the required Python version.

r-rojo · 2020-12-15T00:51:26Z

@gwencasey96,
I tried to reproduce the build errors using rawhide container image (fedroa34) and did not run into the build error you mention with eth_group.cpp. I did run into other issues using the create script and a few more with the opae.spec file which I've addressed those in this PR.
Also, the spec file uses the %make_build macro which results in calling make with multiple jobs and fails for me in my container environment (while compiling a different file hssi.cpp).
Can you provide the details on the failure with eth_group.cpp? If the failure happens but the root cause isn't obvious, try changing the opae.spec file to use make with a single job.

/usr/bin/make -O -j1 V=1 VERBOSE=1

Signed-off-by: anandaravuri <ananda.ravuri@intel.com>

r-rojo · 2020-12-15T04:44:22Z

packaging/opae/rpm/create-tarball.sh

+  --exclude=_build \
+  --exclude=.* \
+  --exclude=*~ \
+  --exclude=doc\sphinx \


Is this supposed to be doc/sphinx?

r-rojo · 2020-12-15T20:08:56Z

opae.spec

-         fpgad\
-         fpgad-api\
-         fpgad-vc\
+%cmake .. -DCMAKE_INSTALL_PREFIX=/usr  -DOPAE_PRESERVE_REPOS=ON -DOPAE_BUILD_LEGACY=ON -DOPAE_BUILD_SAMPLES=ON -B $PWD


This is to tell OPAE build system which Python to look for. This default used to be 2.7 but was changed to 3.6 before it was changed to 3

r-rojo · 2020-12-15T20:09:37Z

opae.spec

-* Thu Sep 17 2020  Ananda Ravuri <ananda.ravuri@intel.com> 2.0.0-1
+* Mon Dec 14 2020 The OPAE Dev Team <opae@lists.01.org> - 2.0.0-2
+- fix code review comments
+- fix  codeing style


s/codeing/coding/

r-rojo · 2020-12-15T20:11:35Z

packaging/opae/rpm/create


 cd "${SCRIPT_DIR}"
 cp ./opae.spec "${RPMBUILD_DIR}/SPECS"

 ${SDK_DIR}/packaging/changelog.py rpm \
    --git-dir="${SDK_DIR}/.git" \
    --project="opae" \
-    --project-version="${PROJECT_VERSION}" \
+    --project-version="${version}" \
    >> "${RPMBUILD_DIR}/SPECS/opae.spec"


it's not fixed because this script is still appending changelog to the opae.spec copy in ${RPMBUILD_DIR}/SPECS.

Signed-off-by: lab <lab@psera2-dell21.ra.intel.com>

r-rojo

For the Python files, we should either do one of the following choices:

remove --record in the setup call and be a little more explicit in the glob pattern for the Python files installed.

%files devel
#...
/usr/lib/python3*/site-packages/opae
/usr/lib/python3*/site-packages/pacsign

concatenate the two files (INSTALLED_OPAE_ADMIN_FILES, INSTALLED_PACSIGN_FILES) into a file that gets referenced in the %files devel section, like the following:

cat $prev/INSTALLED_OPAE_ADMIN_FILES >$prev/DEVEL_PYTHON_INSTALLED_FILES
cat $prev/INSTALLED_PACSIGN_FILES >>$prev/DEVEL_PYTHON_INSTALLED_FILES

%files devel -f DEVEL_PYTHON_INSTALLED_FILES

Also, pacsign has a dependency on python-pkcs11 but there doesn't seem to be an RPM that installs it.
Because of this, the dependency generator in rpmbuild will create an implicit dependency for it.
The best way to install it right now is with pip (pip install python-pkcs11) and beecause we can't express this dependency as an RPM dependency right now, it's best we tell rpmbuild to disable the dependency generator with this %{?python_disable_dependency_generator}
https://docs.fedoraproject.org/en-US/packaging-guidelines/Python/#_dependencies

r-rojo · 2020-12-16T19:27:56Z

opae.spec

@@ -166,15 +142,15 @@ DESTDIR=%{buildroot}  cmake -DCOMPONENT=toolfpgad_api -P cmake_install.cmake
 DESTDIR=%{buildroot}  cmake -DCOMPONENT=toolfpgad_vc -P cmake_install.cmake

 prev=$PWD
-pushd %{_topdir}/BUILD/opae/python/opae.admin/
+pushd %{_topdir}/BUILD/%{name}-%{version}/python/opae.admin/
 %{__python3} setup.py install --single-version-externally-managed  --root=%{buildroot} --record=$prev/INSTALLED_OPAE_ADMIN_FILES


--record is used to record the files installed but the file (INSTALLED_OPAE_ADMIN_FILES) isn't referenced

r-rojo · 2020-12-16T19:28:19Z

opae.spec

 %{__python3} setup.py install --single-version-externally-managed  --root=%{buildroot} --record=$prev/INSTALLED_OPAE_ADMIN_FILES
 popd

-pushd %{_topdir}/BUILD/opae/python/pacsign
+pushd %{_topdir}/BUILD/%{name}-%{version}/python/pacsign
 %{__python3} setup.py install --single-version-externally-managed --root=%{buildroot} --record=$prev/INSTALLED_PACSIGN_FILES


--record is used to record the files installed but the file (INSTALLED_PACSIGN_FILES) isn't referenced.

r-rojo · 2020-12-16T19:28:46Z

opae.spec

@@ -300,4 +281,4 @@ popd
 - Various compiler warning fixes
 - Various memory leak fixes
 - Various Static code scan bug fixes
- Added new FPGA MMIO API to write 512 bits
+- Added new FPGA MMIO API to write 512 bits


missing EOL char

I missed it was still missing this: %{?python_disable_dependency_generator}

r-rojo · 2020-12-17T01:02:40Z

opae.spec

 %{__python3} setup.py install --single-version-externally-managed  --root=%{buildroot} --record=$prev/INSTALLED_OPAE_ADMIN_FILES
 popd

-pushd %{_topdir}/BUILD/opae/python/pacsign
+pushd %{_topdir}/BUILD/%{name}-%{version}/python/pacsign


still missing this macro:

%{?python_disable_dependency_generator}

Signed-off-by: anandaravuri <ananda.ravuri@intel.com>

gwencasey96

The build was successful on Fedora rawhide as of yesterday and successful on Fedora 33 as of today-but you may have one extra rpmbuild directory you do not need with the tarball that create built

Signed-off-by: anandaravuri <ananda.ravuri@intel.com>

tswhison · 2021-01-08T22:46:42Z

opae-libs/plugins/xfpga/common_int.h

@@ -73,12 +73,12 @@
 #define MAP_1G_HUGEPAGE (0x1e << MAP_HUGE_SHIFT) /* 2 ^ 0x1e = 1G */


This file is a part of opae-libs, so it needs to be submitted to that repo.

@anandaravuri, please be sure this file is removed from the pull request and submitted to opae-libs instead.

trixirt · 2021-01-13T19:56:39Z

issues above look ok.
I ran into a what is likely a first time install issue.
rpm -i packaging/opae/rpm/opae-2.0.0-2.fc33.x86_64.rpm
Failed to preset unit: Unit file fpgad.service does not exist.

running rpmlint on opae and opae-devel show known problems.
A new problem
opae-devel.x86_64: E: non-devel-file-in-devel-package /usr/lib64/libfpgad-api.so.2.0.0
likely means this should either move to the opae package or a non system dir like /usr/lib64/opae/

tswhison · 2021-01-13T23:12:19Z

Hi Tom, regarding the placement of libfpgad-api.so.2.0.0, this library is the glue between the fpgad daemon and the plugins that it loads. The remainder of the fpgad files are also packaged in opae-devel. Should we place this file elsewhere when all the other files are in opae-devel? Just trying to understand the best approach.

trixirt · 2021-01-14T19:22:23Z

i think rpmlint is getting hung up with all the *.so going to -devel. I think libfpgad-api.so need to be handled like the other versioned *.so's .. I am thinking of something like this

diff --git a/opae.spec b/opae.spec
index 10bf783c..6a18fb68 100644
--- a/opae.spec
+++ b/opae.spec
@@ -183,7 +183,8 @@ popd
%{_libdir}/libopae-c++-utils.so.2
%{_libdir}/libopae-c++-nlb.so.%{version}
%{_libdir}/libopae-c++-nlb.so.2

+%{_libdir}/libfpgad-api.so.%{version}
+%{_libdir}/libfpgad-api.so.2

%files devel
%dir %{_includedir}/opae
@@ -203,6 +204,7 @@ popd
%{_libdir}/libopae-c++-utils.so
%{_libdir}/libopae-c.so
%{_libdir}/libbitstream.so
+%{_libdir}/libfpgad-api.so
%{_libdir}/opae/libxfpga.so*
%{_libdir}/opae/libmodbmc.so*
%{_bindir}/bist_app*
@@ -247,7 +249,6 @@ popd
%{_bindir}/fpgad
/etc/opae/fpgad.cfg
/etc/sysconfig/fpgad.conf
-%{_libdir}/libfpgad-api.so*
%{_libdir}/opae/libfpgad-vc.so*
/usr/lib/systemd/system/fpgad.service
%{_bindir}/eth_group_mdev.sh

Signed-off-by: anandaravuri <ananda.ravuri@intel.com>

Fixes an rpmlint issue Signed-off-by: Tom Rix <trix@redhat.com>

Though the tarball can be extracted from the source rpm, it is convient to have it already have it with the rpms. This will make posting the tarball in the offical release easier. Signed-off-by: Tom Rix <trix@redhat.com>

This file is part of the jsonscheme internal testsuite, so it is not needed. Resolves an rpmlint issue with devel rpm. Signed-off-by: Tom Rix <trix@redhat.com>

Rpmlint

anandaravuri requested a review from a team as a code owner December 4, 2020 16:49

anandaravuri requested a review from tswhison December 4, 2020 16:49

michelleyho previously approved these changes Dec 4, 2020

View reviewed changes

anandaravuri requested a review from r-rojo December 7, 2020 17:09

r-rojo assigned anandaravuri Dec 7, 2020

r-rojo reviewed Dec 7, 2020

View reviewed changes

anandaravuri requested a review from gwencasey96 December 7, 2020 20:10

r-rojo requested a review from trixirt December 7, 2020 20:42

add create tarball script

0a7122b

- Fix code review comments Signed-off-by: anandaravuri <ananda.ravuri@intel.com>

anandaravuri dismissed michelleyho’s stale review via 0a7122b December 7, 2020 21:35

fix codeing style

e83555a

Signed-off-by: anandaravuri <ananda.ravuri@intel.com>

r-rojo previously approved these changes Dec 8, 2020

View reviewed changes

tswhison reviewed Dec 8, 2020

View reviewed changes

packaging/opae/rpm/create Outdated Show resolved Hide resolved

anandaravuri dismissed r-rojo’s stale review via 1dc1f29 December 9, 2020 00:36

fix code review comments

1dc1f29

Signed-off-by: anandaravuri <ananda.ravuri@intel.com>

r-rojo approved these changes Dec 10, 2020

View reviewed changes

r-rojo reviewed Dec 10, 2020

View reviewed changes

tswhison reviewed Dec 10, 2020

View reviewed changes

opae.spec Show resolved Hide resolved

tswhison previously approved these changes Dec 10, 2020

View reviewed changes

r-rojo suggested changes Dec 15, 2020

View reviewed changes

fix spec file and create rpm review comments

d5048e9

Signed-off-by: anandaravuri <ananda.ravuri@intel.com>

anandaravuri dismissed tswhison’s stale review via d5048e9 December 15, 2020 01:35

r-rojo reviewed Dec 15, 2020

View reviewed changes

Merge branch 'release/2.0.0-2' into aravuri/fix_spec

4dfa7bc

r-rojo reviewed Dec 15, 2020

View reviewed changes

Fix sphinx doc in tarball script

dffb2e7

Signed-off-by: lab <lab@psera2-dell21.ra.intel.com>

r-rojo reviewed Dec 16, 2020

View reviewed changes

fix opae spec review comments

4d5446a

r-rojo previously approved these changes Dec 17, 2020

View reviewed changes

r-rojo reviewed Dec 17, 2020

View reviewed changes

anandaravuri and others added 3 commits January 4, 2021 08:52

fix fedora spec file python dependency generator

bd05b6f

Signed-off-by: anandaravuri <ananda.ravuri@intel.com>

Merge branch 'release/2.0.0-2' into aravuri/fix_spec

a136ad1

Remove change log from create rpm

0ed0a3e

Signed-off-by: anandaravuri <ananda.ravuri@intel.com>

gwencasey96 reviewed Jan 8, 2021

View reviewed changes

remove rpmbuild directory after successful build

e513273

Signed-off-by: anandaravuri <ananda.ravuri@intel.com>

tswhison reviewed Jan 8, 2021

View reviewed changes

fix rpmlint errors in spec file

5a28a66

Signed-off-by: anandaravuri <ananda.ravuri@intel.com>

tswhison previously approved these changes Jan 20, 2021

View reviewed changes

fix rpmbuild python version error

26c1ea2

anandaravuri dismissed tswhison’s stale review via 26c1ea2 January 22, 2021 00:09

anandaravuri and others added 5 commits January 22, 2021 09:49

fix rpmlint python errors

28362c8

Signed-off-by: anandaravuri <ananda.ravuri@intel.com>

Move libfpgad-api.so to devel

53bd695

Fixes an rpmlint issue Signed-off-by: Tom Rix <trix@redhat.com>

Stage the opae tarball with the rpms

e888b13

Though the tarball can be extracted from the source rpm, it is convient to have it already have it with the rpms. This will make posting the tarball in the offical release easier. Signed-off-by: Tom Rix <trix@redhat.com>

Exclude jsonscheme_suite from the rpm

4d4bdb7

This file is part of the jsonscheme internal testsuite, so it is not needed. Resolves an rpmlint issue with devel rpm. Signed-off-by: Tom Rix <trix@redhat.com>

Merge pull request #1817 from trixirt/rpmlint

569cc11

Rpmlint

r-rojo merged commit 83b71b5 into release/2.0.0-2 Jan 26, 2021

r-rojo deleted the aravuri/fix_spec branch January 26, 2021 23:55

		@@ -73,12 +73,12 @@
		#define MAP_1G_HUGEPAGE (0x1e << MAP_HUGE_SHIFT) /* 2 ^ 0x1e = 1G */

update fedora spec file and tarball generation script #1770

update fedora spec file and tarball generation script #1770

Conversation

anandaravuri commented Dec 4, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gwencasey96 commented Dec 11, 2020 via email

Choose a reason for hiding this comment

Choose a reason for hiding this comment

r-rojo Dec 15, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

r-rojo commented Dec 15, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

r-rojo left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gwencasey96 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

trixirt commented Jan 13, 2021 • edited Loading

tswhison commented Jan 13, 2021

trixirt commented Jan 14, 2021

diff --git a/opae.spec b/opae.spec index 10bf783c..6a18fb68 100644 --- a/opae.spec +++ b/opae.spec @@ -183,7 +183,8 @@ popd %{_libdir}/libopae-c++-utils.so.2 %{_libdir}/libopae-c++-nlb.so.%{version} %{_libdir}/libopae-c++-nlb.so.2

r-rojo Dec 15, 2020 •

edited

Loading

r-rojo left a comment •

edited

Loading

trixirt commented Jan 13, 2021 •

edited

Loading

diff --git a/opae.spec b/opae.spec
index 10bf783c..6a18fb68 100644
--- a/opae.spec
+++ b/opae.spec
@@ -183,7 +183,8 @@ popd
%{_libdir}/libopae-c++-utils.so.2
%{_libdir}/libopae-c++-nlb.so.%{version}
%{_libdir}/libopae-c++-nlb.so.2