Merge lp:~andrea.corbellini/software-properties/fix-621977 into lp:software-properties
- fix-621977
- Merge into main
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 |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Andrea Corbellini (community) | Needs Resubmitting | ||
Barry Warsaw (community) | Needs Fixing | ||
Robert Roth (community) | Approve | ||
Review via email: mp+134815@code.launchpad.net |
Commit message
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.
Andrea Corbellini (andrea.corbellini) 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.
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!
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_
-------
Traceback (most recent call last):
File "test_dbus.py", line 248, in test_add_
self.
File "/usr/lib/
**keywords)
File "/usr/lib/
message, timeout)
DBusException: org.freedesktop
File "/usr/lib/
retval = candidate_
File "../softwarepro
self.
File "../softwarepro
if source.file != apt_pkg.
AttributeError: 'NoneType' object has no attribute 'file'
=======
FAIL: test_enable_
-------
Traceback (most recent call last):
File "test_dbus.py", line 127, in test_enable_
self.
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.
Andrea Corbellini (andrea.corbellini) wrote : | # |
I've submitted a newer, cleaner branch for review:
https:/
Unmerged revisions
- 822. By Andrea Corbellini
-
Do not enable deb-src lines by default.
Preview Diff
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, |
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.