| Message ID | 20240930144936.1175589-1-roid@nvidia.com |
|---|---|
| State | Accepted |
| Commit | 7df4dd2aa0e98be481e9210a637a6671a60eca56 |
| Delegated to: | Eelco Chaudron |
| Headers | show |
| Series | [ovs-dev,v3,1/1] debian: Allow passing DEB_BUILD_OPTIONS. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
On 30 Sep 2024, at 16:49, Roi Dayan wrote: > Allow passing different DEB_BUILD_OPTIONS to make debian-deb. > > Signed-off-by: Roi Dayan <roid@nvidia.com> Thanks Roi for keeping up with my comments :) This revision looks good to me. Acked-by: Eelco Chaudron <echaudro@redhat.com> //Eelco > --- > > Notes: > v3 > - Remove unneeded export call. > - Move assignment to an existing DPDK_NETDEV check. > > v2 > - Fix export of DEB_BUILD_OPTIONS in the Makefile > > debian/automake.mk | 9 +++------ > 1 file changed, 3 insertions(+), 6 deletions(-) > > diff --git a/debian/automake.mk b/debian/automake.mk > index 7b2afafae1a2..7607a2cd5b3a 100644 > --- a/debian/automake.mk > +++ b/debian/automake.mk > @@ -98,10 +98,12 @@ if DPDK_NETDEV > update_deb_control = \ > $(AM_V_GEN) sed -e 's/^\# DPDK_NETDEV //' \ > < $(srcdir)/debian/control.in > debian/control > +DEB_BUILD_OPTIONS ?= nocheck parallel=`nproc` > else > update_deb_control = \ > $(AM_V_GEN) grep -v '^\# DPDK_NETDEV' \ > < $(srcdir)/debian/control.in > debian/control > +DEB_BUILD_OPTIONS ?= nocheck parallel=`nproc` nodpdk > endif > > debian/control: $(srcdir)/debian/control.in Makefile > @@ -123,10 +125,5 @@ debian-deb: debian > $(update_deb_copyright) > $(update_deb_control) > $(AM_V_GEN) fakeroot debian/rules clean > -if DPDK_NETDEV > - $(AM_V_GEN) DEB_BUILD_OPTIONS="nocheck parallel=`nproc`" \ > - fakeroot debian/rules binary > -else > - $(AM_V_GEN) DEB_BUILD_OPTIONS="nocheck parallel=`nproc` nodpdk" \ > + $(AM_V_GEN) DEB_BUILD_OPTIONS="${DEB_BUILD_OPTIONS}" \ > fakeroot debian/rules binary > -endif > -- > 2.46.1
On Mon, Sep 30, 2024 at 05:49:36PM +0300, Roi Dayan via dev wrote: > Allow passing different DEB_BUILD_OPTIONS to make debian-deb. > > Signed-off-by: Roi Dayan <roid@nvidia.com> > --- > > Notes: > v3 > - Remove unneeded export call. > - Move assignment to an existing DPDK_NETDEV check. > > v2 > - Fix export of DEB_BUILD_OPTIONS in the Makefile Thanks Roi, Like Eelco, this version looks good to me. Acked-by: Simon Horman <horms@ovn.org>
On 9/30/24 16:49, Roi Dayan via dev wrote: > Allow passing different DEB_BUILD_OPTIONS to make debian-deb. > > Signed-off-by: Roi Dayan <roid@nvidia.com> > --- > > Notes: > v3 > - Remove unneeded export call. > - Move assignment to an existing DPDK_NETDEV check. > > v2 > - Fix export of DEB_BUILD_OPTIONS in the Makefile > > debian/automake.mk | 9 +++------ > 1 file changed, 3 insertions(+), 6 deletions(-) > > diff --git a/debian/automake.mk b/debian/automake.mk > index 7b2afafae1a2..7607a2cd5b3a 100644 > --- a/debian/automake.mk > +++ b/debian/automake.mk > @@ -98,10 +98,12 @@ if DPDK_NETDEV > update_deb_control = \ > $(AM_V_GEN) sed -e 's/^\# DPDK_NETDEV //' \ > < $(srcdir)/debian/control.in > debian/control > +DEB_BUILD_OPTIONS ?= nocheck parallel=`nproc` > else > update_deb_control = \ > $(AM_V_GEN) grep -v '^\# DPDK_NETDEV' \ > < $(srcdir)/debian/control.in > debian/control > +DEB_BUILD_OPTIONS ?= nocheck parallel=`nproc` nodpdk > endif > > debian/control: $(srcdir)/debian/control.in Makefile > @@ -123,10 +125,5 @@ debian-deb: debian > $(update_deb_copyright) > $(update_deb_control) > $(AM_V_GEN) fakeroot debian/rules clean > -if DPDK_NETDEV > - $(AM_V_GEN) DEB_BUILD_OPTIONS="nocheck parallel=`nproc`" \ > - fakeroot debian/rules binary > -else > - $(AM_V_GEN) DEB_BUILD_OPTIONS="nocheck parallel=`nproc` nodpdk" \ > + $(AM_V_GEN) DEB_BUILD_OPTIONS="${DEB_BUILD_OPTIONS}" \ I think some confusion is coming from env variables vs make variables. While make imports all the env variables, the definitions in the file may still be a little confusing, since they are not shell definitions ( ?= is not a shell operator). Can we maybe use plain braces $() instead of curly ones here ${} ? That may probably make the code a little clearer. Best regards, Ilya Maximets.
On 2 Oct 2024, at 13:35, Ilya Maximets wrote: > On 9/30/24 16:49, Roi Dayan via dev wrote: >> Allow passing different DEB_BUILD_OPTIONS to make debian-deb. >> >> Signed-off-by: Roi Dayan <roid@nvidia.com> >> --- >> >> Notes: >> v3 >> - Remove unneeded export call. >> - Move assignment to an existing DPDK_NETDEV check. >> >> v2 >> - Fix export of DEB_BUILD_OPTIONS in the Makefile >> >> debian/automake.mk | 9 +++------ >> 1 file changed, 3 insertions(+), 6 deletions(-) >> >> diff --git a/debian/automake.mk b/debian/automake.mk >> index 7b2afafae1a2..7607a2cd5b3a 100644 >> --- a/debian/automake.mk >> +++ b/debian/automake.mk >> @@ -98,10 +98,12 @@ if DPDK_NETDEV >> update_deb_control = \ >> $(AM_V_GEN) sed -e 's/^\# DPDK_NETDEV //' \ >> < $(srcdir)/debian/control.in > debian/control >> +DEB_BUILD_OPTIONS ?= nocheck parallel=`nproc` >> else >> update_deb_control = \ >> $(AM_V_GEN) grep -v '^\# DPDK_NETDEV' \ >> < $(srcdir)/debian/control.in > debian/control >> +DEB_BUILD_OPTIONS ?= nocheck parallel=`nproc` nodpdk >> endif >> >> debian/control: $(srcdir)/debian/control.in Makefile >> @@ -123,10 +125,5 @@ debian-deb: debian >> $(update_deb_copyright) >> $(update_deb_control) >> $(AM_V_GEN) fakeroot debian/rules clean >> -if DPDK_NETDEV >> - $(AM_V_GEN) DEB_BUILD_OPTIONS="nocheck parallel=`nproc`" \ >> - fakeroot debian/rules binary >> -else >> - $(AM_V_GEN) DEB_BUILD_OPTIONS="nocheck parallel=`nproc` nodpdk" \ >> + $(AM_V_GEN) DEB_BUILD_OPTIONS="${DEB_BUILD_OPTIONS}" \ > > I think some confusion is coming from env variables vs make variables. > While make imports all the env variables, the definitions in the file > may still be a little confusing, since they are not shell definitions > ( ?= is not a shell operator). > > Can we maybe use plain braces $() instead of curly ones here ${} ? > That may probably make the code a little clearer. Did a quick test and the $() approach works also. I can apply this during commit time, Roi please confirm if you are ok with this change. Cheers, Eelco
On 30 Sep 2024, at 16:49, Roi Dayan wrote: > Allow passing different DEB_BUILD_OPTIONS to make debian-deb. > > Signed-off-by: Roi Dayan <roid@nvidia.com> > --- Applied with the $() instead of curly ones ${}. Cheers, Eelco
On 04/10/2024 14:01, Eelco Chaudron wrote: > > > On 30 Sep 2024, at 16:49, Roi Dayan wrote: > >> Allow passing different DEB_BUILD_OPTIONS to make debian-deb. >> >> Signed-off-by: Roi Dayan <roid@nvidia.com> >> --- > > Applied with the $() instead of curly ones ${}. > > Cheers, > > Eelco > thanks
diff --git a/debian/automake.mk b/debian/automake.mk index 7b2afafae1a2..7607a2cd5b3a 100644 --- a/debian/automake.mk +++ b/debian/automake.mk @@ -98,10 +98,12 @@ if DPDK_NETDEV update_deb_control = \ $(AM_V_GEN) sed -e 's/^\# DPDK_NETDEV //' \ < $(srcdir)/debian/control.in > debian/control +DEB_BUILD_OPTIONS ?= nocheck parallel=`nproc` else update_deb_control = \ $(AM_V_GEN) grep -v '^\# DPDK_NETDEV' \ < $(srcdir)/debian/control.in > debian/control +DEB_BUILD_OPTIONS ?= nocheck parallel=`nproc` nodpdk endif debian/control: $(srcdir)/debian/control.in Makefile @@ -123,10 +125,5 @@ debian-deb: debian $(update_deb_copyright) $(update_deb_control) $(AM_V_GEN) fakeroot debian/rules clean -if DPDK_NETDEV - $(AM_V_GEN) DEB_BUILD_OPTIONS="nocheck parallel=`nproc`" \ - fakeroot debian/rules binary -else - $(AM_V_GEN) DEB_BUILD_OPTIONS="nocheck parallel=`nproc` nodpdk" \ + $(AM_V_GEN) DEB_BUILD_OPTIONS="${DEB_BUILD_OPTIONS}" \ fakeroot debian/rules binary -endif
Allow passing different DEB_BUILD_OPTIONS to make debian-deb. Signed-off-by: Roi Dayan <roid@nvidia.com> --- Notes: v3 - Remove unneeded export call. - Move assignment to an existing DPDK_NETDEV check. v2 - Fix export of DEB_BUILD_OPTIONS in the Makefile debian/automake.mk | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-)