| Message ID | 1464193022-5437-1-git-send-email-tim.gardner@canonical.com |
|---|---|
| State | New |
| Headers | show |
On Wed, May 25, 2016 at 10:17:02AM -0600, Tim Gardner wrote: > BugLink: http://bugs.launchpad.net/bugs/1585311 > > Signed-off-by: Tim Gardner <tim.gardner@canonical.com> > --- > > v2 - changed the commit subject a little > > debian/rules.d/2-binary-arch.mk | 3 +++ > debian/rules.d/3-binary-indep.mk | 1 + > 2 files changed, 4 insertions(+) > > diff --git a/debian/rules.d/2-binary-arch.mk b/debian/rules.d/2-binary-arch.mk > index 0e7fedb..8ce1fa8 100644 > --- a/debian/rules.d/2-binary-arch.mk > +++ b/debian/rules.d/2-binary-arch.mk > @@ -391,6 +391,7 @@ ifeq ($(do_tools_hyperv),true) > $(LN) ../../$(src_pkg_name)-tools-$(abi_release)/hv_kvp_daemon $(cloudpkgdir)/usr/lib/linux-tools/$(abi_release)-$* > $(LN) ../../$(src_pkg_name)-tools-$(abi_release)/hv_vss_daemon $(cloudpkgdir)/usr/lib/linux-tools/$(abi_release)-$* > $(LN) ../../$(src_pkg_name)-tools-$(abi_release)/hv_fcopy_daemon $(cloudpkgdir)/usr/lib/linux-tools/$(abi_release)-$* > + $(LN) ../../$(src_pkg_name)-tools-$(abi_release)/lsvmbus $(cloudpkgdir)/usr/lib/linux-tools/$(abi_release)-$* > endif > endif > > @@ -669,6 +670,8 @@ ifeq ($(do_tools_hyperv),true) > $(cloudpkgdir)/usr/lib/$(src_pkg_name)-tools-$(abi_release) > install -m755 $(builddirpa)/tools/hv/hv_fcopy_daemon \ > $(cloudpkgdir)/usr/lib/$(src_pkg_name)-tools-$(abi_release) > + install -m755 $(builddirpa)/tools/hv/lsvmbus \ > + $(cloudpkgdir)/usr/lib/$(src_pkg_name)-tools-$(abi_release) > endif > endif > > diff --git a/debian/rules.d/3-binary-indep.mk b/debian/rules.d/3-binary-indep.mk > index 29cc541..32bc90c 100644 > --- a/debian/rules.d/3-binary-indep.mk > +++ b/debian/rules.d/3-binary-indep.mk > @@ -127,6 +127,7 @@ ifeq ($(do_tools_common),true) > install -m755 debian/tools/generic $(cloudsbin)/hv_kvp_daemon > install -m755 debian/tools/generic $(cloudsbin)/hv_vss_daemon > install -m755 debian/tools/generic $(cloudsbin)/hv_fcopy_daemon > + install -m755 debian/tools/generic $(cloudsbin)/lsvmbus > install -m755 debian/cloud-tools/hv_get_dhcp_info $(cloudsbin) > install -m755 debian/cloud-tools/hv_get_dns_info $(cloudsbin) > install -m755 debian/cloud-tools/hv_set_ifconfig $(cloudsbin) > -- There is nothing wrong with the patch per-see. However, this is a python2 thing and we should be seeing if it can be run with python3 at least. Also it should have a manual page if it is going to be installed. /me goes test whether it will work as python3. -apw
How about these two in addition which switch the script up to python3 and add a manual page. I have tested the python3 conversion on an actual instance and it seems to produce identicle output. -apw
I presume this will work on LTS Trusty ?
On Fri, May 27, 2016 at 01:52:20PM +0100, Andy Whitcroft wrote: > How about these two in addition which switch the script up to python3 > and add a manual page. I have tested the python3 conversion on an > actual instance and it seems to produce identicle output. > > -apw While I'm all for modernizing this script, I'd suggest that the patch "tools/hv/lsvmbus -- convert to python3" should be upstreamed with cc: stable (first) instead of applied as UBUNTU SAUCE... 1. It does not fix any actual manifesting problem, yet makes our version of the tools/hv/lsvmbus script diverge from mainline. Why should we SRU this? 2. Nothing else in mainline appears to require python3... Wouldn't these changes work just as well without the "#!/usr/bin/env python3" delta? (So it would continue to work if python3 wasn't installed). 3. I think switching from 'print' command python3 print() function should always also use "from __future__ import print_function" -- regardless of whether or not the script currently "needs" that -- because not doing so introduces a hard-to-debug situation when somebody tries to add e.g.: print(..., end='') to what appears to be a print() function call. -Kamal
On Fri, May 27, 2016 at 01:52:20PM +0100, Andy Whitcroft wrote: > How about these two in addition which switch the script up to python3 > and add a manual page. I have tested the python3 conversion on an > actual instance and it seems to produce identicle output. > > -apw > > -- > kernel-team mailing list > kernel-team@lists.ubuntu.com > https://lists.ubuntu.com/mailman/listinfo/kernel-team I'm fine with the additional changes to the script and with the addition of the man page. Brad
On Fri, May 27, 2016 at 06:46:08AM -0700, Kamal Mostafa wrote: > On Fri, May 27, 2016 at 01:52:20PM +0100, Andy Whitcroft wrote: > > How about these two in addition which switch the script up to python3 > > and add a manual page. I have tested the python3 conversion on an > > actual instance and it seems to produce identicle output. > > > > -apw > > While I'm all for modernizing this script, I'd suggest that the patch > "tools/hv/lsvmbus -- convert to python3" should be upstreamed with cc: > stable (first) instead of applied as UBUNTU SAUCE... > > 1. It does not fix any actual manifesting problem, yet makes our version > of the tools/hv/lsvmbus script diverge from mainline. Why should we SRU > this? > > 2. Nothing else in mainline appears to require python3... Wouldn't > these changes work just as well without the "#!/usr/bin/env python3" > delta? (So it would continue to work if python3 wasn't installed). I believe we do not install python2 by default, especially in Yakkety. So either we need to convert to python3 or we need to depend on python2 on the binary package. The main issue here is that the default python version is a distro series related thing, not an upstream related thing. > 3. I think switching from 'print' command python3 print() function > should always also use "from __future__ import print_function" -- > regardless of whether or not the script currently "needs" that -- > because not doing so introduces a hard-to-debug situation when somebody > tries to add e.g.: print(..., end='') to what appears to be a print() > function call. That may well be true. -apw
On Fri, May 27, 2016 at 11:46:19PM +0100, Andy Whitcroft wrote: > I believe we do not install python2 by default, especially in Yakkety. > So either we need to convert to python3 or we need to depend on python2 > on the binary package. The main issue here is that the default python > version is a distro series related thing, not an upstream related thing. I have clarified the position on python2/3 use on #ubuntu-devel. We are actually activly meant to avoid adding any new python2 dependancies in both Xenial and Yakkety (Xenial was the first series we attempted to eradicate python2). Based on that position we should either convert this script to python3 or reject the whole series exposing it. > > 3. I think switching from 'print' command python3 print() function > > should always also use "from __future__ import print_function" -- > > regardless of whether or not the script currently "needs" that -- > > because not doing so introduces a hard-to-debug situation when somebody > > tries to add e.g.: print(..., end='') to what appears to be a print() > > function call. Also in discussions with the people doing the conversion from python2 to 3 it does not seem to add value to include the future statement in a script only intended for use in python3. This is of use in libraries which get imported in either, but this is not such a library. That said as it is a noop there I would not object to having it there if that makes you happier. -apw
After further discussion with Andy, I ACK the python3-converted patch as is. -Kamal
Applied (added BugLink). -Kamal
On Fri, Jun 03, 2016 at 07:11:52AM -0700, Kamal Mostafa wrote: > Applied (added BugLink). > > -Kamal
diff --git a/debian/rules.d/2-binary-arch.mk b/debian/rules.d/2-binary-arch.mk index 0e7fedb..8ce1fa8 100644 --- a/debian/rules.d/2-binary-arch.mk +++ b/debian/rules.d/2-binary-arch.mk @@ -391,6 +391,7 @@ ifeq ($(do_tools_hyperv),true) $(LN) ../../$(src_pkg_name)-tools-$(abi_release)/hv_kvp_daemon $(cloudpkgdir)/usr/lib/linux-tools/$(abi_release)-$* $(LN) ../../$(src_pkg_name)-tools-$(abi_release)/hv_vss_daemon $(cloudpkgdir)/usr/lib/linux-tools/$(abi_release)-$* $(LN) ../../$(src_pkg_name)-tools-$(abi_release)/hv_fcopy_daemon $(cloudpkgdir)/usr/lib/linux-tools/$(abi_release)-$* + $(LN) ../../$(src_pkg_name)-tools-$(abi_release)/lsvmbus $(cloudpkgdir)/usr/lib/linux-tools/$(abi_release)-$* endif endif @@ -669,6 +670,8 @@ ifeq ($(do_tools_hyperv),true) $(cloudpkgdir)/usr/lib/$(src_pkg_name)-tools-$(abi_release) install -m755 $(builddirpa)/tools/hv/hv_fcopy_daemon \ $(cloudpkgdir)/usr/lib/$(src_pkg_name)-tools-$(abi_release) + install -m755 $(builddirpa)/tools/hv/lsvmbus \ + $(cloudpkgdir)/usr/lib/$(src_pkg_name)-tools-$(abi_release) endif endif diff --git a/debian/rules.d/3-binary-indep.mk b/debian/rules.d/3-binary-indep.mk index 29cc541..32bc90c 100644 --- a/debian/rules.d/3-binary-indep.mk +++ b/debian/rules.d/3-binary-indep.mk @@ -127,6 +127,7 @@ ifeq ($(do_tools_common),true) install -m755 debian/tools/generic $(cloudsbin)/hv_kvp_daemon install -m755 debian/tools/generic $(cloudsbin)/hv_vss_daemon install -m755 debian/tools/generic $(cloudsbin)/hv_fcopy_daemon + install -m755 debian/tools/generic $(cloudsbin)/lsvmbus install -m755 debian/cloud-tools/hv_get_dhcp_info $(cloudsbin) install -m755 debian/cloud-tools/hv_get_dns_info $(cloudsbin) install -m755 debian/cloud-tools/hv_set_ifconfig $(cloudsbin)
BugLink: http://bugs.launchpad.net/bugs/1585311 Signed-off-by: Tim Gardner <tim.gardner@canonical.com> --- v2 - changed the commit subject a little debian/rules.d/2-binary-arch.mk | 3 +++ debian/rules.d/3-binary-indep.mk | 1 + 2 files changed, 4 insertions(+)