Merge lp:~mvo/launchpad/support-timeframe-information into lp:launchpad/db-devel

Proposed by Michael Vogt
Status: Merged
Approved by: Michael Nelson
Approved revision: no longer in the source branch.
Merged at revision: not available
Proposed branch: lp:~mvo/launchpad/support-timeframe-information
Merge into: lp:launchpad/db-devel
Diff against target: 245 lines (+161/-24)
2 files modified
cronscripts/publishing/cron.germinate (+1/-1)
cronscripts/publishing/maintenance-check.py (+160/-23)
To merge this branch: bzr merge lp:~mvo/launchpad/support-timeframe-information
Reviewer Review Type Date Requested Status
Michael Nelson (community) code Approve
Michael Vogt (community) Needs Resubmitting
William Grant code Approve
Review via email: mp+22118@code.launchpad.net

Commit message

Adds support for an additional "overwrite" file that can be used to tweak the support information generated during germination.

Description of the change

Please merge this updated support-timeframe-information branch. It adds support for a additional "overwrite" file that can be used to tweak the support information.

To post a comment you must log in.
Revision history for this message
Michael Vogt (mvo) wrote :

If it can be run on dogfood first to ensure it works properly in the LP environment, that would be most appreciated.

Revision history for this message
Aaron Bentley (abentley) wrote :

The description does not give me enough context to do a review.

Revision history for this message
Michael Nelson (michael.nelson) wrote :
Download full text (9.8 KiB)

Hi Michael!

Thanks for getting in there and improving Launchpad code!

As Aaron suggested, it would be really helpful to include more info with your merge proposal. A template we often try to use is:

https://dev.launchpad.net/QuickCoverLetterTemplate

or if you want lots of details ;) :

https://dev.launchpad.net/DetailedCoverLetterTemplate

Especially important for branches like this without tests is a QA plan for how we can verify on dogfood that this does exactly what you intend. Tests would be wonderful, but I know that it's fair to ask you to add them now (If you want an example of how other cron scripts are tested, see lib/lp/archivepublisher/tests/test_generate_ppa_htaccess.py)

OK, I've got a bunch of small changes below, but more important for me is the summary of what this branch does (after reading it I've now understood most of it, but it would still be helpful to clarify) and steps for us to QA this on dogfood.

As I don't have lots of experience with germination etc. (I'm still learning ;) ), and Julian is away, I'll ask William to take a look to and provide feedback.

Thanks!

> === modified file 'cronscripts/publishing/maintenance-check.py'
> --- cronscripts/publishing/maintenance-check.py 2010-01-22 13:57:45 +0000
> +++ cronscripts/publishing/maintenance-check.py 2010-03-25 11:57:28 +0000
> @@ -59,9 +63,57 @@
> # germinate output base directory
> BASE_URL = "http://people.canonical.com/~ubuntu-archive/germinate-output/"
>
> +# hints dir url, hints file is "$distro.hints" by default
> +# (e.g. lucid.hints)
> +#HINTS_DIR_URL = "http://people.canonical.com/~mvo/maintenance-check/%s.hints"

Is there a reason for leaving the above comment eg. in?

> +HINTS_DIR_URL = "http://people.canonical.com/~ubuntu-archive/seeds/platform.%s/SUPPORTED_HINTS"
> +
> +# we need the archive root to parse the Sources file to support
> +# by-source hints
> +#ARCHIVE_ROOT = "file:/srv/launchpad.net/ubuntu-archive/ubuntu"

Same above?

> +ARCHIVE_ROOT = "http://archive.ubuntu.com/ubuntu"
> +
> # support timeframe tag used in the Packages file
> SUPPORT_TAG = "Supported"
>
> +def get_binaries_for_source_pkg(srcname):
> + """
> + get all binary package names for the given source package name
> + """

Just a few style comments (based on the Launchpad python style guide at https://dev.launchpad.net/PythonStyleGuide). I realise the rest of this file
doesn't follow the style-guide, but it'd be nice to clean it up bit-by-bit
(but not necessary, up to you).

So the above docstring could be formatted along the lines of:
       """ Return all binary names for the given source name."""

If you are really keen, you could add the :param: and :return: types as outlined under "Docstrings" in the above styleguide, but it's not necessary.

> + pkgnames = set()
> + recs = apt_pkg.GetPkgSrcRecords()
> + while recs.Lookup(srcname):
> + for binary in recs.Binaries:
> + pkgnames.add(binary)
> + return pkgnames
> +
> +def expand_src_pkgname(pkgname):
> + """ expand a given pkgname if prefixed with src: to a list of
> + binary package names, if not prefixed, just return the pkgname
> + """

       """ Expand a package...

review: Needs Information (code)
Revision history for this message
William Grant (wgrant) wrote :

The functionality seems good. Apart from the stylistic issues that noodles identified, this looks fine.

review: Approve (code)
Revision history for this message
Michael Vogt (mvo) wrote :

Many thanks for the review. I updated the branch now to cover most of your points.

Revision history for this message
Michael Vogt (mvo) wrote :

The cover letter:

Summary:
Add ability to override seeds to make support information more accurate

Pre-implementation notes
For some package the seeds are not expressive enough, e.g. kvm is something we want to support for 5y, but the multimedia stack that is part of kvm and is not required to run kvm as a headless service (like libsdl) we only want to support for 3y.

Implementation details:
The implmentation adds the ability to override specific source or binary package support information.

Tests:
There are no unitests (yet). One test is running it and verifing that the output changes from:
libsdl1.2debian/amd64 Supported 5y
to 3y

Revision history for this message
Michael Nelson (michael.nelson) wrote :

Thanks for the changes Michael. I spoke with Julian and he's ok without the tests this time, but would like to start always adding tests for this kind of thing. We'll need to run it on Dogfood before landing it.

review: Approve (code)
Revision history for this message
Julian Edwards (julian-edwards) wrote :

Hi

This can go in as it is, provided that it's tested on dogfood first. However, before any more non-urgent changes go in you should write unit tests please. If you need help with that, we'll be glad to assist.

Revision history for this message
Michael Nelson (michael.nelson) wrote :

Tested initially on df with:

https://pastebin.canonical.com/30476/

Michael will update the script so it doesn't depend on a newer version of python-apt.

Revision history for this message
Michael Vogt (mvo) wrote :

I fixed this now to work with the python-apt version in hardy. Please give it another go.

review: Needs Resubmitting
Revision history for this message
Michael Nelson (michael.nelson) wrote :

Great, thanks Michael!

Here's the output, please check if it's as you expected:

{{{
$ ./codelines/current/cronscripts/publishing/maintenance-check.py lucid > ./ubuntu-archive/ubuntu-misc/more-extra.override.lucid.main_after_mvos_change
hints-file: changing x11-common from 5y to 3y
hints-file: changing libsdl1.2debian from 5y to 3y
hints-file: changing libsdl1.2debian-alsa from 5y to 3y
hints-file: changing libpulse0 from 5y to 3y
hints-file: changing libsndfile1 from 5y to 3y
hints-file: changing libogg0 from 5y to 3y
hints-file: changing libvorbis0a from 5y to 3y
hints-file: changing libvorbisenc2 from 5y to 3y
hints-file: changing libhal-storage1 from 5y to 3y
hints-file: changing libhal1 from 5y to 3y
hints-file: changing hal from 5y to 3y
hints-file: changing hal-info from 5y to 3y
}}}

But doing a diff on the before/after seems to just have *all* packages changed to 5y?
https://pastebin.canonical.com/30498/

review: Needs Information
Revision history for this message
Michael Nelson (michael.nelson) wrote :

OK, after chatting with mvo, I've run the script but *without* the server-ship change of r9154.

{{{
$ ./codelines/current/cronscripts/publishing/maintenance-check.py lucid > ./ubuntu-archive/ubuntu-misc/more-extra.override.lucid.main_after_mvos_change_but_without_server_ship
hints-file: changing x11-common from 5y to 3y
}}}

and:
{{{
$ diff more-extra.override.lucid.main_before_mvos_change more-extra.override.lucid.main_after_mvos_change_but_without_server_ship > /home/michaeln/more-extra.override.lucid.main.mvo.diff.3
}}}

which gives
{{{
$ less more-extra.override.lucid.main.mvo.diff.3
903a904,906
> crypto-modules-2.6.32-20-generic-pae-di/i386 Supported 5y
> crypto-modules-2.6.32-20-generic-pae-di/amd64 Supported 5y
> crypto-modules-2.6.32-20-generic-pae-di/armel Supported 18m
3867a3871,3873
> kernel-image-2.6.32-20-generic-pae-di/i386 Supported 5y
> kernel-image-2.6.32-20-generic-pae-di/amd64 Supported 5y
> kernel-image-2.6.32-20-generic-pae-di/armel Supported 18m
16272a16279,16281
> nic-shared-modules-2.6.32-20-generic-pae-di/i386 Supported 5y
> nic-shared-modules-2.6.32-20-generic-pae-di/amd64 Supported 5y
> nic-shared-modules-2.6.32-20-generic-pae-di/armel Supported 18m
17667a17677,17679
> ppp-modules-2.6.32-20-generic-pae-di/i386 Supported 5y
> ppp-modules-2.6.32-20-generic-pae-di/amd64 Supported 5y
> ppp-modules-2.6.32-20-generic-pae-di/armel Supported 18m
19182a19195,19197
> serial-modules-2.6.32-20-generic-pae-di/i386 Supported 5y
> serial-modules-2.6.32-20-generic-pae-di/amd64 Supported 5y
> serial-modules-2.6.32-20-generic-pae-di/armel Supported 18m
20773,20774c20788,20789
< x11-common/i386 Supported 5y
< x11-common/amd64 Supported 5y
---
> x11-common/i386 Supported 3y
> x11-common/amd64 Supported 3y
}}}

Are those additional entries expected? If not I can re-run script without your changes to confirm.

Revision history for this message
Michael Nelson (michael.nelson) wrote :

Approving after the above tests on dogfood.

review: Approve (code)
Revision history for this message
Michael Nelson (michael.nelson) wrote :

I meant to mention, although it's unrelated to your branch, when running the maintenance check for karmic the following happens on dogfood:
https://pastebin.canonical.com/30562/

To find out why, I added the following:
{{{
=== modified file 'cronscripts/publishing/maintenance-check.py'
--- cronscripts/publishing/maintenance-check.py 2010-01-22 13:57:45 +0000
+++ cronscripts/publishing/maintenance-check.py 2010-04-14 10:10:45 +0000
@@ -68,6 +68,8 @@
     get structure file for named distro and distro version
     (e.g. kubuntu, lucid)
     """
+ url = "%s/%s.%s/structure" % (BASE_URL, name, version)
+ print url
     f = urllib2.urlopen("%s/%s.%s/structure" % (BASE_URL, name, version))
     structure = f.readlines()
     f.close()
}}}

and the result is:
{{{
http://people.canonical.com/~ubuntu-archive/germinate-output//ubuntu.karmic/structure
http://people.canonical.com/~ubuntu-archive/germinate-output//kubuntu.karmic/structure
http://people.canonical.com/~ubuntu-archive/germinate-output//edubuntu.karmic/structure
http://people.canonical.com/~ubuntu-archive/germinate-output//netbook.karmic/structure
}}}

Revision history for this message
Michael Nelson (michael.nelson) wrote :

JFTR: the above is expected by Michael:

12:28 <mvo> hello
12:28 <mvo> sorry for the slow reply, I was at lunch
12:29 <mvo> it needs to run post release for -updates and friends
12:30 <noodles> Great, as long as it's expected behavior.

Revision history for this message
Michael Nelson (michael.nelson) wrote :

I created bug 562938 for the above.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'cronscripts/publishing/cron.germinate'
--- cronscripts/publishing/cron.germinate 2010-03-02 09:58:34 +0000
+++ cronscripts/publishing/cron.germinate 2010-04-13 12:58:27 +0000
@@ -127,7 +127,7 @@
127echo " done."127echo " done."
128128
129# now generate the Supported extra overrides129# now generate the Supported extra overrides
130$MAINTAINCE_CHECK $suite > "$MISCROOT/more-extra.override.$suite.main.supported"130$MAINTAINCE_CHECK $suite > "$MISCROOT/more-extra.override.$suite.main.supported" 2> maintenance-check.stderr
131if [ $? -eq 0 ]; then131if [ $? -eq 0 ]; then
132 cat "$MISCROOT/more-extra.override.$suite.main.supported" >> "$MISCROOT/more-extra.override.$suite.main.new"132 cat "$MISCROOT/more-extra.override.$suite.main.supported" >> "$MISCROOT/more-extra.override.$suite.main.new"
133fi133fi
134134
=== modified file 'cronscripts/publishing/maintenance-check.py'
--- cronscripts/publishing/maintenance-check.py 2010-04-12 15:13:53 +0000
+++ cronscripts/publishing/maintenance-check.py 2010-04-13 12:58:27 +0000
@@ -6,9 +6,19 @@
6# https://code.edge.launchpad.net/~mvo/ubuntu-maintenance-check/python-port6# https://code.edge.launchpad.net/~mvo/ubuntu-maintenance-check/python-port
7# (where it will vanish once taken here)7# (where it will vanish once taken here)
88
9# this warning filter is only needed on older versions of python-apt,
10# once the machine runs lucid it can be removed
11import warnings
12warnings.filterwarnings("ignore","apt API not stable yet")
13import apt
14warnings.resetwarnings()
15
16import apt_pkg
9import logging17import logging
18import os
10import sys19import sys
11import urllib220import urllib2
21import urlparse
1222
13from optparse import OptionParser23from optparse import OptionParser
1424
@@ -59,24 +69,98 @@
59# germinate output base directory69# germinate output base directory
60BASE_URL = "http://people.canonical.com/~ubuntu-archive/germinate-output/"70BASE_URL = "http://people.canonical.com/~ubuntu-archive/germinate-output/"
6171
72# hints dir url, hints file is "$distro.hints" by default
73# (e.g. lucid.hints)
74HINTS_DIR_URL = "http://people.canonical.com/~ubuntu-archive/seeds/platform.%s/SUPPORTED_HINTS"
75
76# we need the archive root to parse the Sources file to support
77# by-source hints
78ARCHIVE_ROOT = "http://archive.ubuntu.com/ubuntu"
79
62# support timeframe tag used in the Packages file80# support timeframe tag used in the Packages file
63SUPPORT_TAG = "Supported"81SUPPORT_TAG = "Supported"
6482
6583def get_binaries_for_source_pkg(srcname):
66def get_structure(name, version):84 """ Return all binary package names for the given source package name.
67 """ 85
68 get structure file for named distro and distro version 86 :param srcname: The source package name.
69 (e.g. kubuntu, lucid)87 :return: A list of binary package names.
70 """88 """
71 f = urllib2.urlopen("%s/%s.%s/structure" % (BASE_URL, name, version))89 pkgnames = set()
90 recs = apt_pkg.GetPkgSrcRecords()
91 while recs.Lookup(srcname):
92 for binary in recs.Binaries:
93 pkgnames.add(binary)
94 return pkgnames
95
96def expand_src_pkgname(pkgname):
97 """ Expand a package name if it is prefixed with src.
98
99 If the package name is prefixed with src it will be expanded
100 to a list of binary package names. Otherwise the original
101 package name will be returned.
102
103 :param pkgname: The package name (that may include src:prefix).
104 :return: A list of binary package names (the list may be one element long).
105 """
106 if not pkgname.startswith("src:"):
107 return [pkgname]
108 return get_binaries_for_source_pkg(pkgname.split("src:")[1])
109
110def create_and_update_deb_src_source_list(distroseries):
111 """ Create sources.list and update cache.
112
113 This creates a sources.list file with deb-src entries for a given
114 distroseries and apt.Cache.update() to make sure the data is up-to-date.
115 :param distro: The code name of the distribution series (e.g. lucid).
116 :return: None
117 :raises: IOError: When cache update fails.
118 """
119 # apt root dir
120 rootdir="./aptroot.%s" % distroseries
121 sources_list_dir = os.path.join(rootdir, "etc","apt")
122 if not os.path.exists(sources_list_dir):
123 os.makedirs(sources_list_dir)
124 sources_list = open(os.path.join(sources_list_dir, "sources.list"),"w")
125 for pocket in [
126 "%s" % distroseries,
127 "%s-updates" % distroseries,
128 "%s-security" % distroseries]:
129 sources_list.write(
130 "deb-src %s %s main restricted\n" % (
131 ARCHIVE_ROOT, pocket))
132 sources_list.close()
133 # create required dirs/files for apt.Cache(rootdir) to work on older
134 # versions of python-apt. once lucid is used it can be removed
135 for d in ["var/lib/dpkg",
136 "var/cache/apt/archives/partial",
137 "var/lib/apt/lists/partial"]:
138 if not os.path.exists(os.path.join(rootdir,d)):
139 os.makedirs(os.path.join(rootdir,d))
140 if not os.path.exists(os.path.join(rootdir,"var/lib/dpkg/status")):
141 open(os.path.join(rootdir,"var/lib/dpkg/status"),"w")
142 # open cache with our just prepared rootdir
143 cache = apt.Cache(rootdir=rootdir)
144 cache.update(apt.progress.FetchProgress())
145
146def get_structure(distroname, version):
147 """ Get structure file conent for named distro and distro version.
148
149 :param name: Name of the distribution (e.g. kubuntu, ubuntu, xubuntu).
150 :param version: Code name of the distribution version (e.g. lucid).
151 :return: List of strings with the structure file content
152 """
153 f = urllib2.urlopen("%s/%s.%s/structure" % (BASE_URL, distroname, version))
72 structure = f.readlines()154 structure = f.readlines()
73 f.close()155 f.close()
74 return structure156 return structure
75157
76def expand_seeds(structure, seedname):158def expand_seeds(structure, seedname):
77 """ 159 """ Expand seed by its dependencies using the strucure file.
78 expand seed by its dependencies using the strucure file160
79 returns a set() for the seed dependencies (excluding the original seedname)161 :param structure: The content of the STRUCTURE file as string list.
162 :param seedname: The name of the seed as string that needs to be expanded.
163 :return: a set() for the seed dependencies (excluding the original seedname)
80 """164 """
81 seeds = []165 seeds = []
82 for line in structure:166 for line in structure:
@@ -150,6 +234,8 @@
150 parser.add_option("--source-packages", "", default=False,234 parser.add_option("--source-packages", "", default=False,
151 action="store_true", 235 action="store_true",
152 help="show as source pkgs")236 help="show as source pkgs")
237 parser.add_option("--hints-file", "", default=None,
238 help="use diffenrt use hints file location")
153 (options, args) = parser.parse_args()239 (options, args) = parser.parse_args()
154240
155 # init241 # init
@@ -160,6 +246,17 @@
160 sys.exit(1)246 sys.exit(1)
161 else:247 else:
162 distro = "lucid"248 distro = "lucid"
249
250 # make sure our deb-src information is up-to-date
251 create_and_update_deb_src_source_list(distro)
252
253 if options.hints_file:
254 hints_file = options.hints_file
255 (schema, netloc, path, query, fragment) = urlparse.urlsplit(hints_file)
256 if not schema:
257 hints_file = "file:%s" % path
258 else:
259 hints_file = HINTS_DIR_URL % distro
163 260
164 # go over the distros we need to check261 # go over the distros we need to check
165 pkg_support_time = {}262 pkg_support_time = {}
@@ -175,20 +272,60 @@
175 else:272 else:
176 support_timeframe = SUPPORT_TIMEFRAME273 support_timeframe = SUPPORT_TIMEFRAME
177 get_packages_support_time(structure, name, pkg_support_time, support_timeframe)274 get_packages_support_time(structure, name, pkg_support_time, support_timeframe)
275
276 # now check the hints file that is used to overwrite
277 # the default seeds
278 try:
279 for line in urllib2.urlopen(hints_file):
280 line = line.strip()
281 if not line or line.startswith("#"):
282 continue
283 try:
284 (raw_pkgname, support_time) = line.split()
285 for pkgname in expand_src_pkgname(raw_pkgname):
286 if support_time == 'unsupported':
287 try:
288 del pkg_support_time[pkgname]
289 sys.stderr.write("hints-file: marking %s unsupported\n" % pkgname)
290 except KeyError:
291 pass
292 else:
293 if pkg_support_time.get(pkgname) != support_time:
294 sys.stderr.write(
295 "hints-file: changing %s from %s to %s\n" % (
296 pkgname, pkg_support_time.get(pkgname),
297 support_time))
298 pkg_support_time[pkgname] = support_time
299 except:
300 logging.exception("can not parse line '%s'" % line)
301 except urllib2.HTTPError, e:
302 if e.getcode() != 404:
303 raise
304 sys.stderr.write("hints-file: %s gave 404 error\n" % hints_file)
178 305
179 # output suitable for the extra-override file306 # output suitable for the extra-override file
180 for pkgname in sorted(pkg_support_time.keys()):307 for pkgname in sorted(pkg_support_time.keys()):
181 # go over the supported arches, they are divided in 308 # special case, the hints file may contain overrides that
182 # first-class (PRIMARY) and second-class with different309 # are arch-specific (like zsh-doc/armel)
183 # support levels310 if "/" in pkgname:
184 for arch in SUPPORTED_ARCHES:311 print "%s %s %s" % (
185 # full LTS support312 pkgname, SUPPORT_TAG, pkg_support_time[pkgname])
186 if arch in PRIMARY_ARCHES:313 else:
187 print "%s/%s %s %s" % (314 # go over the supported arches, they are divided in
188 pkgname, arch, SUPPORT_TAG, pkg_support_time[pkgname])315 # first-class (PRIMARY) and second-class with different
189 else:316 # support levels
190 # not a LTS supported architecture, gets only regular317 for arch in SUPPORTED_ARCHES:
191 # support_timeframe318 # ensure we do not overwrite arch-specific overwrites
192 print "%s/%s %s %s" % (319 pkgname_and_arch = "%s/%s" % (pkgname, arch)
193 pkgname, arch, SUPPORT_TAG, SUPPORT_TIMEFRAME[0][0])320 if pkgname_and_arch in pkg_support_time:
321 break
322 if arch in PRIMARY_ARCHES:
323 # arch with full LTS support
324 print "%s %s %s" % (
325 pkgname_and_arch, SUPPORT_TAG, pkg_support_time[pkgname])
326 else:
327 # not a LTS supported architecture, gets only regular
328 # support_timeframe
329 print "%s %s %s" % (
330 pkgname_and_arch, SUPPORT_TAG, SUPPORT_TIMEFRAME[0][0])
194 331

Subscribers

People subscribed via source and target branches

to status/vote changes: