Merge lp:~andrea.corbellini/software-properties/fix-621977 into lp:software-properties

Proposed by Andrea Corbellini on 2012-11-18
Status: Needs review
Proposed branch: lp:~andrea.corbellini/software-properties/fix-621977
Merge into: lp:software-properties
Diff against target: 307 lines (+44/-40)
2 files modified
add-apt-repository (+13/-10)
softwareproperties/SoftwareProperties.py (+31/-30)
To merge this branch: bzr merge lp:~andrea.corbellini/software-properties/fix-621977
Reviewer Review Type Date Requested Status
Andrea Corbellini (community) Resubmit on 2012-12-14
Barry Warsaw (community) 2012-11-18 Needs Fixing on 2012-12-11
Robert Roth (community) Approve on 2012-11-21
Review via email: mp+134815@code.launchpad.net

Description of the change

This branch fixes bug #621977 in the following way:

1. deb-src lines are added, but commented out, so that Software Properties still lets you enable them later;

2. a new -s, --enable-source command line option is added so that you can add uncommented deb-src line with a single command.

To post a comment you must log in.
Robert Roth (evfool) wrote :

The patch looks fine with one minor detail: lots of unchanged lines are added to the diff (probably because tab/spaces usage - whitespace differences). It would be nice if you could set up your editor to use spaces instead of tabs (I think software-properties uses spaces instead of tabs) to avoid harder-to-review diffs, because the reviewer has to filter the real changes.

Other than that: nice solution to comment the source line by default, but leave an option to enable the source with the same command.

review: Approve

> The patch looks fine with one minor detail: lots of unchanged lines are added
> to the diff (probably because tab/spaces usage - whitespace differences). It
> would be nice if you could set up your editor to use spaces instead of tabs (I
> think software-properties uses spaces instead of tabs) to avoid harder-to-
> review diffs, because the reviewer has to filter the real changes.

Actually I'm using spaces for indentation. The problem with the diff is caused by the fact that my editor automatically removes trailing whitespace characters. Anyhow, sorry about that :-)

> Other than that: nice solution to comment the source line by default, but
> leave an option to enable the source with the same command.

Thanks!

Barry Warsaw (barry) wrote :

The diff looks fine, and while the whitespace cleanups are distracting, I personally appreciate all that trailing whitespace not hurting my eyes :)

I'll go ahead and sponsor this into Raring. Thanks for your contribution to Ubuntu!

review: Approve
Barry Warsaw (barry) wrote :

Whoops, this causes some test failures. I think they're shallow -- let's see.

Barry Warsaw (barry) wrote :

1 failure, 1 error:

======================================================================
ERROR: test_add_remove_source_by_line (__main__.TestDBus)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "test_dbus.py", line 248, in test_add_remove_source_by_line
    self.iface.RemoveSource(s.replace("deb", "deb-src"))
  File "/usr/lib/python2.7/dist-packages/dbus/proxies.py", line 145, in __call__
    **keywords)
  File "/usr/lib/python2.7/dist-packages/dbus/connection.py", line 651, in call_blocking
    message, timeout)
DBusException: org.freedesktop.DBus.Python.AttributeError: Traceback (most recent call last):
  File "/usr/lib/python2.7/dist-packages/dbus/service.py", line 707, in _message_cb
    retval = candidate_method(self, *args, **keywords)
  File "../softwareproperties/dbus/SoftwarePropertiesDBus.py", line 278, in RemoveSource
    self.remove_source(_to_unicode(source))
  File "../softwareproperties/SoftwareProperties.py", line 741, in remove_source
    if source.file != apt_pkg.config.find_file("Dir::Etc::sourcelist"):
AttributeError: 'NoneType' object has no attribute 'file'

======================================================================
FAIL: test_enable_enable_disable_source_code_sources (__main__.TestDBus)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "test_dbus.py", line 127, in test_enable_enable_disable_source_code_sources
    self.assertFalse("deb-src" in sourceslist)
AssertionError: True is not false

So I think before this is merged you have to decide how to handle this. Marking this Needs Fixing for now.

review: Needs Fixing
review: Resubmit

Unmerged revisions

822. By Andrea Corbellini on 2012-11-18

Do not enable deb-src lines by default.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'add-apt-repository'
2--- add-apt-repository 2012-11-09 12:21:58 +0000
3+++ add-apt-repository 2012-11-18 19:27:20 +0000
4@@ -33,7 +33,7 @@
5 if len(user_inst.ppas) > 0:
6 # Translators: %(entity)s is either "team" or "user"
7 print(_("The %(entity)s named '%(user)s' has no PPA named '%(ppa)s'") % {
8- 'entity' : entity_name,
9+ 'entity' : entity_name,
10 'user' : user,
11 'ppa' : ppa_name})
12 print(_("Please choose from the following available PPAs:"))
13@@ -44,7 +44,7 @@
14 else:
15 # Translators: %(entity)s is either "team" or "user"
16 print(_("The %(entity)s named '%(user)s' does not have any PPA") % {
17- 'entity' : entity_name,
18+ 'entity' : entity_name,
19 'user' : user})
20 except KeyError:
21 pass
22@@ -64,13 +64,13 @@
23 gettext.textdomain("software-properties")
24 usage = """Usage: %prog <sourceline>
25
26-%prog is a script for adding apt sources.list entries.
27-It can be used to add any repository and also provides a shorthand
28+%prog is a script for adding apt sources.list entries.
29+It can be used to add any repository and also provides a shorthand
30 syntax for adding a Launchpad PPA (Personal Package Archive)
31 repository.
32
33 <sourceline> - The apt repository source line to add. This is one of:
34- a complete apt line in quotes,
35+ a complete apt line in quotes,
36 a repo url and areas in quotes (areas defaults to 'main')
37 a PPA shortcut.
38 a distro component
39@@ -79,7 +79,7 @@
40 apt-add-repository 'deb http://myserver/path/to/repo stable myrepo'
41 apt-add-repository 'http://myserver/path/to/repo myrepo'
42 apt-add-repository 'https://packages.medibuntu.org free non-free'
43- apt-add-repository http://extras.ubuntu.com/ubuntu
44+ apt-add-repository http://extras.ubuntu.com/ubuntu
45 apt-add-repository ppa:user/repository
46 apt-add-repository multiverse
47
48@@ -87,7 +87,7 @@
49 sources.list
50 """
51 parser = OptionParser(usage)
52- # FIXME: provide a --sources-list-file= option that
53+ # FIXME: provide a --sources-list-file= option that
54 # puts the line into a specific file in sources.list.d
55 parser.add_option ("-m", "--massive-debug", action="store_true",
56 dest="massive_debug", default=False,
57@@ -98,6 +98,9 @@
58 parser.add_option("-k", "--keyserver",
59 dest="keyserver", default=DEFAULT_KEYSERVER,
60 help=_("URL of keyserver. Default: %default"))
61+ parser.add_option("-s", "--enable-source", action="store_true",
62+ dest="enable_source", default=False,
63+ help=_("Allow downloading of the source packages from the repository"))
64 parser.add_option("-y", "--yes", action="store_true",
65 dest="assume_yes", default=False,
66 help=_("Assume yes to all queries"))
67@@ -129,7 +132,7 @@
68 if user.startswith("~"):
69 print(_("Did you mean 'ppa:%s/%s' ?") %(user[1:], ppa_name))
70 sys.exit(1) # Exit because the user cannot be correct
71- # If the PPA does not exist, then try to find if the user/team
72+ # If the PPA does not exist, then try to find if the user/team
73 # exists. If it exists, list down the PPAs
74 _maybe_suggest_ppa_name_based_on_user(user)
75 sys.exit(1)
76@@ -142,7 +145,7 @@
77 if "private" in ppa_info and ppa_info["private"]:
78 print(_("Adding private PPAs is not supported currently"))
79 sys.exit(1)
80-
81+
82 if options.remove:
83 print(_("You are about to remove the following PPA from your system:"))
84 else:
85@@ -198,7 +201,7 @@
86 print(_("Error: '%s' doesn't exist in a sourcelist file") % debsrc_line)
87
88 else:
89- if not sp.add_source_from_line(line):
90+ if not sp.add_source_from_line(line, options.enable_source):
91 print(_("Error: '%s' invalid") % line)
92 sys.exit(1)
93 sp.sourceslist.save()
94
95=== modified file 'softwareproperties/SoftwareProperties.py'
96--- softwareproperties/SoftwareProperties.py 2012-10-01 09:24:55 +0000
97+++ softwareproperties/SoftwareProperties.py 2012-11-18 19:27:20 +0000
98@@ -78,10 +78,10 @@
99 RELEASE_UPGRADES_LTS : 'lts',
100 RELEASE_UPGRADES_NEVER : 'never',
101 }
102-
103+
104 def __init__(self, datadir=None, options=None, rootdir="/"):
105- """ Provides the core functionality to configure the used software
106- repositories, the corresponding authentication keys and
107+ """ Provides the core functionality to configure the used software
108+ repositories, the corresponding authentication keys and
109 update automation """
110 self.popconfile = rootdir+"/etc/popularity-contest.conf"
111
112@@ -97,7 +97,7 @@
113
114 self.sourceslist = SourcesList()
115 self.distro = aptsources.distro.get_distro()
116-
117+
118 self.seen_server = []
119 self.modified_sourceslist = False
120
121@@ -136,8 +136,8 @@
122 self.write_config()
123
124 def get_update_automation_level(self):
125- """ Parse the apt cron configuration. Try to fit a predefined use case
126- and return it. Special case: if the user made a custom
127+ """ Parse the apt cron configuration. Try to fit a predefined use case
128+ and return it. Special case: if the user made a custom
129 configurtation, that we cannot represent it will return None """
130 if apt_pkg.config.find_i(softwareproperties.CONF_MAP["autoupdate"]) > 0:
131 # Autodownload
132@@ -159,9 +159,9 @@
133 return None
134
135 def set_update_automation_level(self, state):
136- """ Set the apt periodic configurtation to the selected
137- update automation level. To synchronize the cache update and the
138- actual upgrading function, the upgrade function, e.g. unattended,
139+ """ Set the apt periodic configurtation to the selected
140+ update automation level. To synchronize the cache update and the
141+ actual upgrading function, the upgrade function, e.g. unattended,
142 will run every day, if enabled. """
143 if state == softwareproperties.UPDATE_INST_SEC:
144 apt_pkg.config.set(softwareproperties.CONF_MAP["unattended"], str(1))
145@@ -234,8 +234,8 @@
146 return True
147
148 def get_popcon_participation(self):
149- """ Will return True if the user wants to participate in the popularity
150- contest. Otherwise it will return False. Special case: if no
151+ """ Will return True if the user wants to participate in the popularity
152+ contest. Otherwise it will return False. Special case: if no
153 popcon is installed it will return False """
154 if os.path.exists(self.popconfile):
155 lines = open(self.popconfile).read().split("\n")
156@@ -275,10 +275,10 @@
157 open(self.popconfile, "w").writelines(lines)
158
159 def get_source_code_state(self):
160- """Return True if all distro componets are also available as
161+ """Return True if all distro componets are also available as
162 source code. Otherwise return Flase. Special case: If the
163 configuration cannot be represented return None"""
164-
165+
166 if len(self.distro.source_code_sources) < 1:
167 # we don't have any source code sources, so
168 # uncheck the button
169@@ -327,7 +327,7 @@
170 print("\n")
171
172 def massive_debug_output(self):
173- """Print the complete sources.list"""
174+ """Print the complete sources.list"""
175 print("START SOURCES.LIST:")
176 for source in self.sourceslist:
177 print(source.str())
178@@ -342,12 +342,12 @@
179
180 def enable_component(self, comp):
181 """Enable a component of the distro"""
182- self.distro.enable_component(comp)
183+ self.distro.enable_component(comp)
184 self.set_modified_sourceslist()
185
186 def disable_component(self, comp):
187 """Disable a component of the distro"""
188- self.distro.disable_component(comp)
189+ self.distro.disable_component(comp)
190 self.set_modified_sourceslist()
191
192 def _find_template_from_string(self, name):
193@@ -391,7 +391,7 @@
194 for source in self.distro.source_code_sources:
195 self.sourceslist.remove(source)
196 self.set_modified_sourceslist()
197-
198+
199 def enable_source_code_sources(self):
200 """Enable source code source for all distro sources"""
201 sources = []
202@@ -426,7 +426,7 @@
203 for source in self.sourceslist.list:
204 source_bkp = SourceEntry(line=source.line,file=source.file)
205 self.sourceslist_backup.append(source_bkp)
206-
207+
208 def _find_source_from_string(self, line):
209 # ensure that we have a current list, it might have been changed underneath
210 # us
211@@ -456,7 +456,7 @@
212 return False
213
214 def revert(self):
215- """Revert all settings to the state when software-properties
216+ """Revert all settings to the state when software-properties
217 was launched"""
218 #FIXME: GPG keys are still missing
219 self.restore_apt_conf()
220@@ -557,7 +557,7 @@
221 def get_cdrom_sources(self):
222 """Return the list of CDROM based distro sources"""
223 return self.distro.cdrom_sources
224-
225+
226 def get_comp_download_state(self, comp):
227 """Return a tuple: the first value describes if a component is enabled
228 in the Internet repositories. The second value describes if the
229@@ -567,7 +567,7 @@
230
231 def get_comp_child_state(self, template):
232 """Return a tuple: the first value describes if a component is enabled
233- in one of the child source that matcth the given template.
234+ in one of the child source that matcth the given template.
235 The second value describes if the first value is inconsistent."""
236 comps = []
237 for child in self.distro.child_sources:
238@@ -579,17 +579,17 @@
239 return (True, False)
240 elif len(comps) > 0 and\
241 len(self.distro.enabled_comps ^ set(comps)) != 0:
242- # A matching child source does exist but doesn't include all
243+ # A matching child source does exist but doesn't include all
244 # enabled distro components
245 return(False, True)
246 else:
247 # There is no corresponding child source at all
248 return (False, False)
249-
250+
251 def reload_sourceslist(self):
252 self.sourceslist.refresh()
253 self.sourceslist_visible=[]
254- self.distro.get_sources(self.sourceslist)
255+ self.distro.get_sources(self.sourceslist)
256
257 def write_config(self):
258 """Write the current apt configuration to file"""
259@@ -634,10 +634,10 @@
260 # and append the updated keys
261 for i in cnf.list():
262 f.write("APT::Periodic::%s \"%s\";\n" % (i, cnf.find_i(i)))
263- f.close()
264+ f.close()
265
266 def save_sourceslist(self):
267- """Backup the existing sources.list files and write the current
268+ """Backup the existing sources.list files and write the current
269 configuration"""
270 self.sourceslist.backup(".save")
271 self.sourceslist.save()
272@@ -647,7 +647,7 @@
273 helper that checks if a given line is in the source list
274 return the channel name or None if not found
275 """
276- srcentry = SourceEntry(srcline)
277+ srcentry = SourceEntry(srcline)
278 if os.path.exists(self.CHANNEL_PATH):
279 for f in glob.glob("%s/*.list" % self.CHANNEL_PATH):
280 for line in open(f):
281@@ -696,14 +696,15 @@
282 line = "deb %s %s %s" % ( repo, self.distro.codename, areas )
283 return line
284
285- def add_source_from_line(self, line):
286+ def add_source_from_line(self, line, enable_debsrc=False):
287 """
288 Add a source with the given apt line and auto-add
289 signing key if we have it in the whitelist
290 """
291 (deb_line, file) = expand_ppa_line(line.strip(), self.distro.codename)
292 deb_line = self.expand_http_line(deb_line)
293- debsrc_line = 'deb-src' + deb_line[3:]
294+ debsrc_entry_type = 'deb-src' if enable_debsrc else '#deb-src'
295+ debsrc_line = debsrc_entry_type + deb_line[3:]
296 new_deb_entry = SourceEntry(deb_line, file)
297 new_debsrc_entry = SourceEntry(debsrc_line, file)
298 if new_deb_entry.invalid or new_debsrc_entry.invalid:
299@@ -716,7 +717,7 @@
300 comment=new_deb_entry.comment,
301 file=new_deb_entry.file,
302 architectures=new_deb_entry.architectures)
303- self.sourceslist.add(new_debsrc_entry.type,
304+ self.sourceslist.add(debsrc_entry_type,
305 new_debsrc_entry.uri,
306 new_debsrc_entry.dist,
307 new_debsrc_entry.comps,

Subscribers

People subscribed via source and target branches

to status/vote changes: