Merge ~jslarraz/review-tools:refactor-add-containers into review-tools:master
- Git
- lp:~jslarraz/review-tools
- refactor-add-containers
- Merge into master
Status: | Rejected |
---|---|
Rejected by: | Jorge Sancho Larraz |
Proposed branch: | ~jslarraz/review-tools:refactor-add-containers |
Merge into: | review-tools:master |
Diff against target: |
1609 lines (+1134/-252) 11 files modified
reviewtools/common.py (+1/-159) reviewtools/containers/__init__.py (+0/-0) reviewtools/containers/base_container.py (+407/-0) reviewtools/containers/snap_container.py (+116/-0) reviewtools/containers/squashfs_container.py (+195/-0) reviewtools/sr_common.py (+26/-15) reviewtools/sr_functional.py (+0/-1) reviewtools/sr_lint.py (+11/-11) reviewtools/sr_tests.py (+44/-66) reviewtools/tests/containers/test_base_container.py (+169/-0) reviewtools/tests/containers/test_squashfs_container.py (+165/-0) |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
MyApps reviewers | Pending | ||
Review via email:
|
Commit message
reviewtools refactor: add containers
Description of the change
While implementing component support for reviewtools, I found that reviewtools does not support very well this case, as only one element can be unpacked at once (either the snap or the component) because of how it works (mainly the use of global variables to define unpacked directories)
To improve this situation we agreed to refactor reviewtools with the purpose to make it easier to understand, more flexible, and easier to develop new test cases. In this line, this PR add three helper classes and the minimal changes to make checks to work using these helpers. Updated documentation about helper classes can be temporary found [https:/
- 9083b00... by Jorge Sancho Larraz
-
Run black on container files
- fffd80e... by Jorge Sancho Larraz
-
Remove Review class instead of commenting to simplify review
- 9fe6510... by Jorge Sancho Larraz
-
Move constant definitions back to common.py to make pr easier to review
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Jorge Sancho Larraz (jslarraz) wrote : | # |
Unmerged commits
- 9fe6510... by Jorge Sancho Larraz
-
Move constant definitions back to common.py to make pr easier to review
-
test:0 (build) coverage:0 (build) 1 → 2 of 2 results First • Previous • Next • Last - fffd80e... by Jorge Sancho Larraz
-
Remove Review class instead of commenting to simplify review
- 9083b00... by Jorge Sancho Larraz
-
Run black on container files
- 8246e58... by Jorge Sancho Larraz
-
Update review classes to use new containers
- 2d9987a... by Jorge Sancho Larraz
-
Add BaseContainer SquashfsContainer SnapContainer and relative tests
Preview Diff
1 | diff --git a/reviewtools/common.py b/reviewtools/common.py |
2 | index f46149b..9ed2fd9 100644 |
3 | --- a/reviewtools/common.py |
4 | +++ b/reviewtools/common.py |
5 | @@ -23,7 +23,6 @@ import glob |
6 | import inspect |
7 | import json |
8 | import logging |
9 | -import magic |
10 | import os |
11 | from pkg_resources import resource_filename |
12 | import re |
13 | @@ -351,163 +350,6 @@ class ReviewBase(object): |
14 | self.review_type = name |
15 | |
16 | |
17 | -class Review(ReviewBase): |
18 | - """Common review class""" |
19 | - |
20 | - magic_binary_file_descriptions = [ |
21 | - "application/x-executable; charset=binary", |
22 | - "application/x-sharedlib; charset=binary", |
23 | - "application/x-object; charset=binary", |
24 | - # 18.04 and higher doesn't show the charset |
25 | - "application/x-executable", |
26 | - "application/x-sharedlib", |
27 | - "application/x-object", |
28 | - "application/x-pie-executable", |
29 | - ] |
30 | - |
31 | - def __init__(self, fn, review_type, overrides=None): |
32 | - ReviewBase.__init__(self, review_type, overrides) |
33 | - |
34 | - self.pkg_filename = fn |
35 | - self._check_package_exists() |
36 | - |
37 | - global MKDTEMP_DIR |
38 | - if ( |
39 | - MKDTEMP_DIR is None |
40 | - and "SNAP_USER_COMMON" in os.environ |
41 | - and os.path.exists(os.environ["SNAP_USER_COMMON"]) |
42 | - ): |
43 | - MKDTEMP_DIR = os.environ["SNAP_USER_COMMON"] |
44 | - |
45 | - global UNPACK_DIR |
46 | - if UNPACK_DIR is None: |
47 | - UNPACK_DIR = unpack_pkg(fn) |
48 | - self.unpack_dir = UNPACK_DIR |
49 | - |
50 | - # unpack_pkg() now only supports snap v2, so just hardcode these |
51 | - self.is_snap2 = True |
52 | - self.pkgfmt = {"type": "snap", "version": "16.04"} |
53 | - |
54 | - global RAW_UNPACK_DIR |
55 | - if RAW_UNPACK_DIR is None: |
56 | - RAW_UNPACK_DIR = raw_unpack_pkg(fn) |
57 | - self.raw_unpack_dir = RAW_UNPACK_DIR |
58 | - |
59 | - # Get a list of all unpacked files |
60 | - self.pkg_files = [] |
61 | - # self._list_all_files() sets self.pkg_files so we can mock it |
62 | - self._list_all_files() |
63 | - |
64 | - # Setup what is needed to get a list of all unpacked compiled binaries |
65 | - self.pkg_bin_files = [] |
66 | - # self._list_all_compiled_binaries() sets self.pkg_files so we can |
67 | - # mock it |
68 | - self._list_all_compiled_binaries() |
69 | - |
70 | - def _check_innerpath_executable(self, fn): |
71 | - """Check that the provided path exists and is executable""" |
72 | - return os.access(fn, os.X_OK) |
73 | - |
74 | - def _extract_statinfo(self, fn): |
75 | - """Extract statinfo from file""" |
76 | - try: |
77 | - st = os.stat(fn) |
78 | - except Exception: |
79 | - return None |
80 | - return st |
81 | - |
82 | - def _extract_file(self, fn): |
83 | - """Extract file""" |
84 | - if not fn.startswith("/"): |
85 | - error("_extract_file() expects absolute path") |
86 | - rel = os.path.relpath(fn, self.unpack_dir) |
87 | - |
88 | - if not os.path.isfile(fn): |
89 | - error("Could not find '%s'" % rel) |
90 | - return open_file_read(fn) |
91 | - |
92 | - def _path_join(self, dirname, rest): |
93 | - return os.path.join(dirname, rest) |
94 | - |
95 | - def _get_sha512sum(self, fn): |
96 | - """Get sha512sum of file""" |
97 | - (rc, out) = cmd(["sha512sum", fn]) |
98 | - if rc != 0: |
99 | - return None |
100 | - return out.split()[0] |
101 | - |
102 | - def _pkgfmt_type(self): |
103 | - """Return the package format type""" |
104 | - if "type" not in self.pkgfmt: |
105 | - return "" |
106 | - return self.pkgfmt["type"] |
107 | - |
108 | - def _pkgfmt_version(self): |
109 | - """Return the package format version""" |
110 | - if "version" not in self.pkgfmt: |
111 | - return "" |
112 | - return self.pkgfmt["version"] |
113 | - |
114 | - def _check_package_exists(self): |
115 | - """Check that the provided package exists""" |
116 | - if not os.path.exists(self.pkg_filename): |
117 | - error("Could not find '%s'" % self.pkg_filename) |
118 | - |
119 | - def _list_all_files(self): |
120 | - """List all files included in this package.""" |
121 | - global PKG_FILES |
122 | - if PKG_FILES is None: |
123 | - PKG_FILES = [] |
124 | - for root, dirnames, filenames in os.walk(self.unpack_dir): |
125 | - for f in filenames: |
126 | - PKG_FILES.append(os.path.join(root, f)) |
127 | - |
128 | - self.pkg_files = PKG_FILES |
129 | - |
130 | - def _check_if_message_catalog(self, fn): |
131 | - """Check if file is a message catalog (.mo file).""" |
132 | - if fn.endswith(".mo"): |
133 | - return True |
134 | - return False |
135 | - |
136 | - def _list_all_compiled_binaries(self): |
137 | - """List all compiled binaries in this package.""" |
138 | - global PKG_BIN_FILES |
139 | - if PKG_BIN_FILES is None: |
140 | - self.mime = magic.open(magic.MAGIC_MIME) |
141 | - self.mime.load() |
142 | - PKG_BIN_FILES = [] |
143 | - for i in self.pkg_files: |
144 | - try: |
145 | - res = self.mime.file(i) |
146 | - except Exception: # pragma: nocover |
147 | - # workaround for zesty python3-magic |
148 | - debug("could not detemine mime type of '%s'" % i) |
149 | - continue |
150 | - |
151 | - if ( |
152 | - res in self.magic_binary_file_descriptions |
153 | - and not self._check_if_message_catalog(i) |
154 | - and i not in PKG_BIN_FILES |
155 | - ): |
156 | - PKG_BIN_FILES.append(i) |
157 | - |
158 | - self.pkg_bin_files = PKG_BIN_FILES |
159 | - |
160 | - def _verify_pkgversion(self, v): |
161 | - """Verify package name""" |
162 | - if not isinstance(v, (str, int, float)): |
163 | - return False |
164 | - re_valid_version = re.compile( |
165 | - r"^((\d+):)?" # epoch |
166 | - "([A-Za-z0-9.+:~-]+?)" # upstream |
167 | - "(-([A-Za-z0-9+.~]+))?$" |
168 | - ) # debian |
169 | - if re_valid_version.match(str(v)): |
170 | - return True |
171 | - return False |
172 | - |
173 | - |
174 | # |
175 | # Utility functions |
176 | # |
177 | @@ -519,7 +361,7 @@ def error(out, exit_code=1, do_exit=True, output_type=None): |
178 | global RESULT_TYPES |
179 | |
180 | if output_type is not None: |
181 | - Review.set_report_type(None, output_type) |
182 | + ReviewBase.set_report_type(None, output_type) |
183 | |
184 | try: |
185 | if REPORT_OUTPUT == "json": |
186 | diff --git a/reviewtools/containers/__init__.py b/reviewtools/containers/__init__.py |
187 | new file mode 100644 |
188 | index 0000000..e69de29 |
189 | --- /dev/null |
190 | +++ b/reviewtools/containers/__init__.py |
191 | diff --git a/reviewtools/containers/base_container.py b/reviewtools/containers/base_container.py |
192 | new file mode 100644 |
193 | index 0000000..d3081e2 |
194 | --- /dev/null |
195 | +++ b/reviewtools/containers/base_container.py |
196 | @@ -0,0 +1,407 @@ |
197 | +import os |
198 | +import magic |
199 | +import shutil |
200 | +from typing import Union |
201 | +from tempfile import mkdtemp |
202 | +from reviewtools.common import MKDTEMP_PREFIX, MKDTEMP_DIR |
203 | + |
204 | + |
205 | +class ContainerException(Exception): |
206 | + """This class represents exceptions""" |
207 | + |
208 | + def __init__(self, value): |
209 | + self.value = value |
210 | + |
211 | + def __str__(self): |
212 | + return repr(self.value) |
213 | + |
214 | + |
215 | +class BaseContainer: |
216 | + """ |
217 | + A container is a file that encapsulates a file system. This BaseContainer |
218 | + class models the basic functionality common to all containers, and it is |
219 | + expected to be extended by subclasses handling functionality specific |
220 | + to the container format, such as Squashfs, tar or deb |
221 | + """ |
222 | + |
223 | + magic_binary_file_descriptions = [ |
224 | + "application/x-executable; charset=binary", |
225 | + "application/x-sharedlib; charset=binary", |
226 | + "application/x-object; charset=binary", |
227 | + # 18.04 and higher doesn't show the charset |
228 | + "application/x-executable", |
229 | + "application/x-sharedlib", |
230 | + "application/x-object", |
231 | + "application/x-pie-executable", |
232 | + ] |
233 | + |
234 | + max_packed_size = None |
235 | + min_packed_size = None |
236 | + max_unpacked_size = None |
237 | + min_unpacked_size = None |
238 | + _filename = None |
239 | + _unpack_dir = None |
240 | + _files = None |
241 | + _bin_files = None |
242 | + |
243 | + def __init__(self, filename_or_unpackdir): |
244 | + """ |
245 | + Note: |
246 | + This object will create a temporary copy of the provided container file |
247 | + or unpacked directory. Thus, creating a new instance of the container |
248 | + will be needed to reflect any changes made externally to the original |
249 | + container file or unpacked directory. |
250 | + Automatically created temporary files will be automatically cleaned up |
251 | + on container object deletion. |
252 | + |
253 | + Args: |
254 | + filename_or_unpackdir (str): In the most common way of operation, this |
255 | + parameter is the relative or absolute path to the container file. |
256 | + However, it may also be the relative or absolute path to a directory |
257 | + with the unpacked content of the container file |
258 | + |
259 | + Raises: |
260 | + ContainerException: Calling this method will raise an exception |
261 | + if the provided path does not exist or is not accessible. |
262 | + """ |
263 | + self.tmp_dir = mkdtemp(prefix=MKDTEMP_PREFIX, dir=MKDTEMP_DIR) |
264 | + if os.path.isfile(filename_or_unpackdir): |
265 | + if os.path.basename(filename_or_unpackdir).count(".") == 0: |
266 | + raise ContainerException("container filename must include an extension") |
267 | + self._filename = "%s/%s" % ( |
268 | + self.tmp_dir, |
269 | + os.path.basename(filename_or_unpackdir), |
270 | + ) |
271 | + shutil.copyfile(filename_or_unpackdir, self._filename) |
272 | + self.check_container_size(self.min_packed_size, self.max_packed_size) |
273 | + elif os.path.isdir(filename_or_unpackdir): |
274 | + self._unpack_dir = "%s/%s" % ( |
275 | + self.tmp_dir, |
276 | + os.path.basename(filename_or_unpackdir), |
277 | + ) |
278 | + shutil.copytree(filename_or_unpackdir, self._unpack_dir, dirs_exist_ok=True) |
279 | + self.check_unpacked_size(self.min_unpacked_size, self.max_unpacked_size) |
280 | + else: |
281 | + raise ContainerException( |
282 | + "directory %s does not exists or has incorrect permissions" |
283 | + % filename_or_unpackdir |
284 | + ) |
285 | + |
286 | + def __del__(self): |
287 | + """ |
288 | + Deletes the temporary copy of the files used by this class. This method |
289 | + is automatically called on object deletion |
290 | + """ |
291 | + try: |
292 | + shutil.rmtree(self.tmp_dir) |
293 | + except: |
294 | + pass |
295 | + |
296 | + @property |
297 | + def filename(self) -> str: |
298 | + """ |
299 | + Absolute path to container file temporary location. If it is not defined |
300 | + (i.e. BaseContainer was initialized with an unpacked directory), then pack() |
301 | + will be called and the path of the newly created file will be returned. |
302 | + |
303 | + Note: |
304 | + If under any specific condition you need to avoid calling pack() you |
305 | + should use the private _filename attribute instead of this property. It |
306 | + probably will be only needed by BaseContainer internals and its |
307 | + subclasses. |
308 | + """ |
309 | + if self._filename is None: |
310 | + self._filename = self.pack() |
311 | + return self._filename |
312 | + |
313 | + @property |
314 | + def unpack_dir(self) -> str: |
315 | + """ |
316 | + Absolute path to unpack directory temporary location. If it is not defined |
317 | + (i.e. BaseContainer was initialized with a container fle), then unpack() |
318 | + will be called and the path of the newly created directory will be returned. |
319 | + |
320 | + Note: |
321 | + If under any specific condition you need to avoid calling unpack() you |
322 | + should use the private _unpack_dir attribute instead of this property. It |
323 | + probably will be only needed by BaseContainer internals and its |
324 | + subclasses. |
325 | + """ |
326 | + if self._unpack_dir is None: |
327 | + self._unpack_dir = self.unpack() |
328 | + return self._unpack_dir |
329 | + |
330 | + @property |
331 | + def files(self) -> list: |
332 | + """ |
333 | + List of files present in the container. List elements will be the absolute |
334 | + path of the file taking unpack_dir as root directory. If it is not defined |
335 | + (i.e. the list has not been populated yet), then get_file_list() will be |
336 | + called and the newly created list will be returned. |
337 | + |
338 | + Note: |
339 | + If under any specific condition you need to avoid calling get_files() |
340 | + you should use the private _files attribute instead of this property. |
341 | + It probably will be only needed by BaseContainer internals and its |
342 | + subclasses. |
343 | + """ |
344 | + if self._files is None: |
345 | + self._files = self.get_files_list() |
346 | + return self._files |
347 | + |
348 | + @property |
349 | + def bin_files(self) -> list: |
350 | + """ |
351 | + List of compiled binaries present in the container. List elements will be |
352 | + the absolute path of the file taking unpack_dir as root directory. If it is |
353 | + not defined (i.e. the list has not been populated yet), then |
354 | + get_compiled_binaries_list() will be called and the newly created list will |
355 | + be returned. |
356 | + |
357 | + Note: |
358 | + If under any specific condition you need to avoid calling |
359 | + get_compiled_binaries_list() you should use the private _bin_files |
360 | + attribute instead of this property. It probably will be only needed by |
361 | + BaseContainer internals and its subclasses. |
362 | + """ |
363 | + if self._bin_files is None: |
364 | + self._bin_files = self.get_compiled_binaries_list() |
365 | + return self._bin_files |
366 | + |
367 | + def check_container_format(self): |
368 | + """ |
369 | + Checks that the container file has the expected format. |
370 | + |
371 | + Raises: |
372 | + NotImplementedError: Calling this method will raise an exception as |
373 | + format is specific to the container type. This method needs to be |
374 | + implemented by the specific subclasses. |
375 | + """ |
376 | + raise NotImplementedError("Must be implemented by subclasses") |
377 | + |
378 | + def get_container_size(self) -> int: |
379 | + """ |
380 | + Uses os.path.getsize() to return the size of the container file. If |
381 | + container file is not defined, it will be created by calling pack(). |
382 | + |
383 | + Retruns: |
384 | + int: Size in bytes of the container file |
385 | + """ |
386 | + return os.path.getsize(self.filename) |
387 | + |
388 | + def check_container_size( |
389 | + self, minSize: Union[int, None] = None, maxSize: Union[int, None] = None |
390 | + ): |
391 | + """ |
392 | + Checks that the container size is within the specified range. If |
393 | + container file is not defined, it will be created by calling pack(). |
394 | + |
395 | + Args: |
396 | + minSize (int): Minimum container file size. If it is not specified |
397 | + (i.e. it is None) this check will be skiped. |
398 | + maxSize (int): Maximum container file size. If it is not specified |
399 | + (i.e. it is None) this check will be skiped. |
400 | + |
401 | + Raises: |
402 | + ContainerException: If container size is less than minSize or greater |
403 | + than maxSize, a ContainerException will be raised. |
404 | + """ |
405 | + size = self.get_container_size() |
406 | + if minSize and (size < minSize): |
407 | + raise ContainerException( |
408 | + "container file is too small (%dM < %dM)" |
409 | + % (size / 1024 / 1024, minSize / 1024 / 1024) |
410 | + ) |
411 | + elif maxSize and (size > maxSize): |
412 | + raise ContainerException( |
413 | + "container file is too large (%dM > %dM)" |
414 | + % (size / 1024 / 1024, maxSize / 1024 / 1024) |
415 | + ) |
416 | + |
417 | + def get_unpacked_size(self) -> int: |
418 | + """ |
419 | + Calculates the total size of the unpack dir by adding individual sizes of |
420 | + files listed by os.walk. If unpack dir is not defined, it will be created |
421 | + by calling unpack(). |
422 | + |
423 | + Returns: |
424 | + int: Size in bytes of the container file |
425 | + """ |
426 | + size = 0 |
427 | + for root, dirnames, filenames in os.walk(self.unpack_dir): |
428 | + for f in filenames: |
429 | + if os.path.exists(os.path.join(root, f)): |
430 | + size += os.path.getsize(os.path.join(root, f)) |
431 | + return size |
432 | + |
433 | + def check_unpacked_size( |
434 | + self, minSize: Union[int, None] = None, maxSize: Union[int, None] = None |
435 | + ): |
436 | + """ |
437 | + Checks that the unpacked size is within the specified range. If |
438 | + unpack die is not defined, it will be created by calling unpack(). |
439 | + |
440 | + Args: |
441 | + minSize (int): Minimum unpacked size. If it is not specified |
442 | + (i.e. it is None) this check will be skiped. |
443 | + maxSize (int): Maximum unpacked size. If it is not specified |
444 | + (i.e. it is None) this check will be skiped. |
445 | + |
446 | + Raises: |
447 | + ContainerException: If unpacked size is less than minSize or greater |
448 | + than maxSize, a ContainerException will be raised. |
449 | + """ |
450 | + size = self.get_unpacked_size() |
451 | + if minSize and (size < minSize): |
452 | + raise ContainerException( |
453 | + "unpacked directory is too small (%dM < %dM)" |
454 | + % (size / 1024 / 1024, minSize / 1024 / 1024) |
455 | + ) |
456 | + elif maxSize and (size > maxSize): |
457 | + raise ContainerException( |
458 | + "unpacked directory is too large (%dM > %dM)" |
459 | + % (size / 1024 / 1024, maxSize / 1024 / 1024) |
460 | + ) |
461 | + |
462 | + def calculate_unpacked_size(self) -> int: |
463 | + """ |
464 | + Estimates the space that the unpacked directory will use without unpacking. |
465 | + |
466 | + Returns: |
467 | + int: Estimated size in bytes of the unpacked directory |
468 | + |
469 | + Raises: |
470 | + NotImplementedError: Calling this method will raise an exception as the |
471 | + estimation method is specific to the container type. This method |
472 | + needs to be implemented by the specific subclasses. |
473 | + """ |
474 | + raise NotImplementedError("Must be implemented by subclasses") |
475 | + |
476 | + def check_disk_space(self): |
477 | + """ |
478 | + Checks that the estimated size of less than the 90% of available space in |
479 | + disk. |
480 | + |
481 | + Raises: |
482 | + ContainerException: If estimated unpacked size exceed the 90% of the |
483 | + available space, an exception will be raised. |
484 | + """ |
485 | + size = self.calculate_unpacked_size() |
486 | + st = os.statvfs(self.tmp_dir) |
487 | + avail = st.f_bsize * st.f_bavail * 0.9 # 90% of available space |
488 | + if size > avail * 0.9: |
489 | + raise ContainerException( |
490 | + "insufficient available disk space (%dM > %dM)" |
491 | + % (size / 1024 / 1024, avail / 1024 / 1024) |
492 | + ) |
493 | + |
494 | + def unpack(self) -> str: |
495 | + """ |
496 | + Extracts the content of the container file and returns the absolute path to |
497 | + the newly unpacked directory. |
498 | + |
499 | + Returns: |
500 | + str: absolute path to the newly created unpack dir |
501 | + |
502 | + Raises: |
503 | + NotImplementedError: Calling this method will raise an exception as the |
504 | + estimation method is specific to the container type. This method |
505 | + needs to be implemented by the specific subclasses. |
506 | + """ |
507 | + raise NotImplementedError("Must be implemented by subclasses") |
508 | + |
509 | + def pack(self) -> str: |
510 | + """ |
511 | + Creates a container file from the unpacked directory specified by unpack_dir. |
512 | + |
513 | + Returns: |
514 | + str: absolute path to the newly created container file |
515 | + |
516 | + Raises: |
517 | + NotImplementedError: Calling this method will raise an exception as the |
518 | + estimation method is specific to the container type. This method |
519 | + needs to be implemented by the specific subclasses. |
520 | + """ |
521 | + raise NotImplementedError("Must be implemented by subclasses") |
522 | + |
523 | + def get_file(self, fn: str) -> Union[str, None]: |
524 | + """ |
525 | + Reads and return the binary content of the specified file. |
526 | + |
527 | + Args: |
528 | + fn (str): file relative path to container root (i.e. unpack_dir) |
529 | + |
530 | + Returns: |
531 | + str: binary content of the specified file or None if the file is not |
532 | + present in the container. The output will need to be decoded() to |
533 | + use it as a string |
534 | + |
535 | + Raises: |
536 | + NotImplementedError: Calling this method will raise an exception as the |
537 | + estimation method is specific to the container type. This method |
538 | + needs to be implemented by the specific subclasses. |
539 | + """ |
540 | + if not os.path.exists(os.path.join(self.unpack_dir, fn)): |
541 | + return None |
542 | + |
543 | + try: |
544 | + with open(os.path.join(self.unpack_dir, fn), "rb") as fd: |
545 | + return fd.read() |
546 | + except: |
547 | + raise ContainerException( |
548 | + "Somthing went wrong while reading file %s from the container" % fn |
549 | + ) |
550 | + |
551 | + def get_files_list(self, rel: bool = True) -> list: |
552 | + """ |
553 | + Populates the list of files present in the container (self.files) and |
554 | + returns its value. If unpack dir is not defined, it will be created |
555 | + by calling unpack(). |
556 | + |
557 | + Args: |
558 | + rel (bool): if ``True``, file paths are relative to unpack_dir directory, |
559 | + otherwise absolute paths are used. Defaults to ``False`` |
560 | + |
561 | + Returns: |
562 | + list(str): list of files present in the container |
563 | + """ |
564 | + files = [] |
565 | + for root, dirnames, filenames in os.walk(self.unpack_dir): |
566 | + for f in filenames: |
567 | + file = os.path.join(root, f) |
568 | + file = os.path.relpath(file, self.unpack_dir) if rel else file |
569 | + files.append(file) |
570 | + return files |
571 | + |
572 | + def get_compiled_binaries_list(self, rel: bool = True) -> list: |
573 | + """ |
574 | + Populates the list of compiled binaries present in the container and returns |
575 | + its value. If files list is not defined, it will be populated by calling |
576 | + get_files_list() |
577 | + |
578 | + Args: |
579 | + rel (bool): if ``True``, file paths are relative to unpack_dir directory, |
580 | + otherwise absolute paths are used. Defaults to ``False`` |
581 | + |
582 | + Returns: |
583 | + list(str): list of files present in the container |
584 | + """ |
585 | + mime = magic.open(magic.MAGIC_MIME) |
586 | + mime.load() |
587 | + bin_files = [] |
588 | + for file in self.get_files_list(rel=False): |
589 | + try: |
590 | + res = mime.file(file) |
591 | + except Exception: # pragma: nocover |
592 | + # workaround for zesty python3-magic |
593 | + continue |
594 | + |
595 | + file = os.path.relpath(file, self.unpack_dir) if rel else file |
596 | + if ( |
597 | + res in self.magic_binary_file_descriptions |
598 | + and not file.endswith(".mo") |
599 | + and file not in bin_files |
600 | + ): |
601 | + bin_files.append(file) |
602 | + |
603 | + return bin_files |
604 | diff --git a/reviewtools/containers/snap_container.py b/reviewtools/containers/snap_container.py |
605 | new file mode 100644 |
606 | index 0000000..8ac271a |
607 | --- /dev/null |
608 | +++ b/reviewtools/containers/snap_container.py |
609 | @@ -0,0 +1,116 @@ |
610 | +import ruamel.yaml |
611 | +from typing import Union |
612 | + |
613 | +from reviewtools.containers.base_container import ContainerException |
614 | +from reviewtools.containers.squashfs_container import SquashfsContainer |
615 | + |
616 | + |
617 | +class SnapContainer(SquashfsContainer): |
618 | + max_unpacked_size = 25 * 1024 * 1024 * 1024 |
619 | + max_packed_size = 5 * 1024 * 1024 * 1024 |
620 | + |
621 | + _snap_yaml = None |
622 | + _manifest_yaml = None |
623 | + _dpkg = None |
624 | + |
625 | + @property |
626 | + def snap_yaml(self) -> object: |
627 | + """ |
628 | + Content of "/meta/snap.yaml" file loaded as python object If it has not been |
629 | + previously loaded, then get_snap_yaml() will be called and the loaded object |
630 | + will be returned. |
631 | + |
632 | + Note: |
633 | + If under any specific condition you need to avoid calling |
634 | + get_snap_yaml(), you should use the private _snap_yaml attribute instead |
635 | + of this property. It probably will be only needed by SnapContainer |
636 | + internals and its subclasses. |
637 | + """ |
638 | + if self._snap_yaml is None: |
639 | + self._snap_yaml = self.get_snap_yaml() |
640 | + return self._snap_yaml |
641 | + |
642 | + @property |
643 | + def manifest_yaml(self) -> object: |
644 | + """ |
645 | + Content of "/snap/manifest.yaml" file loaded as python object If it has not |
646 | + been previously loaded, then get_manifest_yaml() will be called and the |
647 | + loaded object will be returned. |
648 | + |
649 | + Note: |
650 | + If under any specific condition you need to avoid calling |
651 | + get_manifest_yaml(), you should use the private _manifest_yaml attribute |
652 | + instead of this property. It probably will be only needed by |
653 | + SnapContainer internals and its subclasses. |
654 | + """ |
655 | + if self._manifest_yaml is None: |
656 | + self._manifest_yaml = self.get_manifest_yaml() |
657 | + return self._manifest_yaml |
658 | + |
659 | + @property |
660 | + def dpkg_entries(self) -> list: |
661 | + """ |
662 | + List packages as lines from "/usr/share/snappy/dpkg.list" or |
663 | + "/snap/dpkg.list" in this precedence order. If it has not been previously |
664 | + loaded, then get_dpkg_entries() will be called and the loaded list will be |
665 | + returned. |
666 | + |
667 | + Note: |
668 | + If under any specific condition you need to avoid calling |
669 | + get_dpkg_entries() you should use the private _dpkg_entries attribute |
670 | + instead of this property. It probably will be only needed by |
671 | + SnapContainer internals and its subclasses. |
672 | + """ |
673 | + if self._dpkg is None: |
674 | + self._dpkg = self.get_dpkg_entries() |
675 | + return self._dpkg |
676 | + |
677 | + def __init__(self, filename_or_unpackdir): |
678 | + super().__init__(filename_or_unpackdir) |
679 | + |
680 | + # # Check size limits if they are defined |
681 | + # if self.min_packed_size or self.max_packed_size: |
682 | + # self.check_container_size(self.min_packed_size, self.max_packed_size) |
683 | + # if self.min_unpacked_size or self.max_unpacked_size: |
684 | + # self.check_unpacked_size(self.min_unpacked_size, self.max_unpacked_size) |
685 | + |
686 | + def get_yaml(self, fn: str) -> Union[str, None]: |
687 | + # Load file |
688 | + file = self.get_file(fn) |
689 | + if file is None: |
690 | + raise ContainerException("file %s not found" % fn) |
691 | + |
692 | + # Prevent duplicated keys |
693 | + yaml = ruamel.yaml.YAML(typ="safe") |
694 | + yaml.allow_duplicate_keys = False |
695 | + try: |
696 | + yaml_object = yaml.load(file.decode()) |
697 | + except ruamel.yaml.constructor.DuplicateKeyError as e: |
698 | + raise ContainerException(e.problem) |
699 | + |
700 | + return yaml_object |
701 | + |
702 | + def get_snap_yaml(self): |
703 | + try: |
704 | + self._snap_yaml = self.get_yaml("/meta/snap.yaml") |
705 | + except ContainerException as e: |
706 | + raise ContainerException("could not read /meta/snap.yaml: %s" % e) |
707 | + |
708 | + return self.snap_yaml |
709 | + |
710 | + def get_manifest_yaml(self): |
711 | + try: |
712 | + self._manifest_yaml = self.get_yaml("/snap/manifest.yaml") |
713 | + except ContainerException as e: |
714 | + raise ContainerException("could not read /snap/manifest.yaml: %s" % e) |
715 | + |
716 | + return self._manifest_yaml |
717 | + |
718 | + def get_dpkg_entries(self): |
719 | + dpkg_entries = [] |
720 | + dpkg = self.get_file("/usr/share/snappy/dpkg.list") |
721 | + if dpkg is None: |
722 | + dpkg = self.get_file("/snap/dpkg.list") |
723 | + if dpkg is not None: |
724 | + dpkg_entries = dpkg.decode().split("\n") |
725 | + return dpkg_entries |
726 | diff --git a/reviewtools/containers/squashfs_container.py b/reviewtools/containers/squashfs_container.py |
727 | new file mode 100644 |
728 | index 0000000..b98c1a3 |
729 | --- /dev/null |
730 | +++ b/reviewtools/containers/squashfs_container.py |
731 | @@ -0,0 +1,195 @@ |
732 | +import os |
733 | +import shutil |
734 | + |
735 | +from reviewtools.common import cmd, cmdIgnoreErrorStrings, UNSQUASHFS_IGNORED_ERRORS |
736 | +from reviewtools.containers.base_container import BaseContainer, ContainerException |
737 | + |
738 | + |
739 | +class SquashfsContainer(BaseContainer): |
740 | + """ |
741 | + A container is a file that encapsulates a file system. This class extends |
742 | + BaseContainer to handle functionality that is specific to Squashfs containers. |
743 | + """ |
744 | + |
745 | + def __init__(self, filename_or_unpackdir): |
746 | + """ |
747 | + If SquashfsContainer is initialized with a container file, it will also |
748 | + verify that the provided file is actually a squashfs, in addition to the |
749 | + checks performed by the BaseContainer. |
750 | + |
751 | + Note: |
752 | + This object will create a temporary copy of the provided container file |
753 | + or unpacked directory. Thus, creating a new instance of the container |
754 | + will be needed to reflect any changes made externally to the original |
755 | + container file or unpacked directory. |
756 | + Automatically created temporary files will be automatically cleaned up |
757 | + on container object deletion. |
758 | + |
759 | + Args: |
760 | + filename_or_unpackdir (str): In the most common way of operation, this |
761 | + parameter is the relative or absolute path to the container file. |
762 | + However, it may also be the relative or absolute path to a directory |
763 | + with the unpacked content of the container file |
764 | + |
765 | + Raises: |
766 | + ContainerException: Calling this method will raise an exception |
767 | + if the provided path does not exist or is not accessible. |
768 | + """ |
769 | + super().__init__(filename_or_unpackdir) |
770 | + |
771 | + # Check is squash file |
772 | + if self._filename is not None: |
773 | + self.check_container_format() |
774 | + |
775 | + def check_container_format(self): |
776 | + """ |
777 | + Checks that the container file has the expected format. |
778 | + |
779 | + Raises: |
780 | + ContainerException: This method will raise an exception if the |
781 | + container file is not a squashfs container, or if the container file |
782 | + is not defined (i.e. SquashfsContainer was initialized with an unpacked |
783 | + directory) and pack() was not explicitly called by the user. |
784 | + """ |
785 | + if self._filename is None: |
786 | + raise ContainerException("container file does not exist") |
787 | + |
788 | + with open(self.filename, "rb") as f: |
789 | + header = f.read(10) |
790 | + if not header.startswith(b"hsqs"): |
791 | + raise ContainerException("Unsupported package format (not squashfs)") |
792 | + |
793 | + def calculate_unpacked_size(self) -> int: |
794 | + """ |
795 | + Estimates the space that the unpacked directory will use by adding the |
796 | + individual files size reported by "unsquashfs -lln" command. It may not |
797 | + be completely accurate, as this command does not consider that each folder |
798 | + requires at least one complete block |
799 | + |
800 | + Returns: |
801 | + int: Estimated size in bytes of the unpacked directory |
802 | + """ |
803 | + if self._filename is None: |
804 | + raise ContainerException("container file does not exist") |
805 | + |
806 | + (rc, out) = cmd(["unsquashfs", "-lln", self.filename]) |
807 | + if rc != 0: |
808 | + raise ContainerException( |
809 | + "unsquashfs -lln '%s' failed: %s" % (self.filename, out) |
810 | + ) |
811 | + |
812 | + size = 0 |
813 | + for line in out.splitlines(): |
814 | + if not line.startswith("-"): # skip non-regular files |
815 | + continue |
816 | + try: |
817 | + size += int(line.split()[2]) |
818 | + except ValueError: # skip non-numbers |
819 | + continue |
820 | + |
821 | + return size |
822 | + |
823 | + def unpack(self, force: bool = False) -> str: |
824 | + """ |
825 | + Extracts the content of the container file and returns the absolute path to |
826 | + the newly unpacked directory. |
827 | + |
828 | + Args: |
829 | + force (bool, optional): If ``True``, the unpack_dir will be overwritten |
830 | + if it already exists. Defaults to ``False`` |
831 | + |
832 | + Returns: |
833 | + str: absolute path to the newly created unpack dir |
834 | + |
835 | + Raises: |
836 | + ContainerException: This method will raise an ContainerException if the |
837 | + container file is not defined (i.e. SquashfsContainer was |
838 | + initialized with an unpacked directory) and pack() was not |
839 | + explicitly called by the user, or if the unpack_dir already exists |
840 | + and this method was called without the force argument set to True |
841 | + RuntimeError: This method will raise a RuntimeError under the execution |
842 | + of the "unsquashfs" command returns with an error code other than 0. |
843 | + """ |
844 | + if self._filename is None: |
845 | + raise ContainerException("container file does not exist") |
846 | + |
847 | + if self._unpack_dir is not None and not force: |
848 | + raise ContainerException( |
849 | + "unpack dir already exists and unpack function was called without force" |
850 | + ) |
851 | + |
852 | + # Check that disk has enough space to extract the container |
853 | + self.check_disk_space() |
854 | + |
855 | + # Unpack the container |
856 | + stage_dir = os.path.join(self.tmp_dir, "stage") |
857 | + (rc, out) = cmdIgnoreErrorStrings( |
858 | + [ |
859 | + "unsquashfs", |
860 | + "-no-progress", |
861 | + "-ignore-errors", |
862 | + "-quiet", |
863 | + "-f", |
864 | + "-d", |
865 | + stage_dir, |
866 | + self.filename, |
867 | + ], |
868 | + UNSQUASHFS_IGNORED_ERRORS, |
869 | + ) |
870 | + if rc != 0: |
871 | + if os.path.isdir(stage_dir): |
872 | + shutil.rmtree(stage_dir) |
873 | + raise RuntimeError("unpacking failed with '%d':\n%s" % (rc, out)) |
874 | + |
875 | + # Update on success |
876 | + if self._unpack_dir is not None and os.path.exists(self._unpack_dir): |
877 | + shutil.rmtree(self._unpack_dir) |
878 | + self._unpack_dir = ".".join(self.filename.split(".")[:-1]) |
879 | + shutil.move(stage_dir, self._unpack_dir) |
880 | + return self._unpack_dir |
881 | + |
882 | + def pack(self, force: bool = False, extension: str = "container") -> str: |
883 | + """ |
884 | + Creates a container file from the unpacked directory specified by unpack_dir |
885 | + |
886 | + Args: |
887 | + force (bool, optional): If ``True``, the container file will be |
888 | + overwritten if it already exists. Defaults to ``False`` |
889 | + extension (str, optional): extension used in the newly created container |
890 | + file. Defaults to ``"container"`` |
891 | + |
892 | + Returns: |
893 | + str: absolute path to the newly created container file |
894 | + |
895 | + Raises: |
896 | + ContainerException: This method will raise an ContainerException if the |
897 | + unpack_dir is not defined (i.e. SquashfsContainer was initialized |
898 | + with a container file) and unpack() was not explicitly called by the |
899 | + user, or if the container file already exists and this method was |
900 | + called without the force argument set to True |
901 | + RuntimeError: This method will raise a RuntimeError under the execution |
902 | + of the "mksquashfs" command returns with an error code other than 0. |
903 | + """ |
904 | + if self._unpack_dir is None: |
905 | + raise ContainerException("unpack dir does not exist") |
906 | + |
907 | + if self._filename is not None and not force: |
908 | + raise ContainerException( |
909 | + "Container file already exists and pack function was called without force" |
910 | + ) |
911 | + |
912 | + # Pack the directory |
913 | + # TODO: make use of MKSQUASHFS_OPTS before updating sr_security::check_squashfs_resquash to use this method |
914 | + stage_file = os.path.join(self.tmp_dir, "stage") |
915 | + (rc, out) = cmd(["mksquashfs", self.unpack_dir, stage_file]) |
916 | + if rc != 0: |
917 | + if os.path.exists: |
918 | + os.unlink(stage_file) |
919 | + raise RuntimeError("packing failed with '%d':\n%s" % (rc, out)) |
920 | + |
921 | + # Update on success |
922 | + if self._filename is not None and os.path.exists(self._filename): |
923 | + os.unlink(self._filename) |
924 | + self._filename = ".".join([self.unpack_dir, extension]) |
925 | + shutil.move(stage_file, self._filename) |
926 | + return self._filename |
927 | diff --git a/reviewtools/sr_common.py b/reviewtools/sr_common.py |
928 | index 201a0dc..a7c6e4e 100644 |
929 | --- a/reviewtools/sr_common.py |
930 | +++ b/reviewtools/sr_common.py |
931 | @@ -21,7 +21,7 @@ import yaml |
932 | |
933 | |
934 | from reviewtools.common import ( |
935 | - Review, |
936 | + ReviewBase, |
937 | ReviewException, |
938 | error, |
939 | open_file_read, |
940 | @@ -31,6 +31,8 @@ from reviewtools.common import ( |
941 | verify_type, |
942 | ) |
943 | from reviewtools.overrides import interfaces_attribs_addons |
944 | +from reviewtools.containers.snap_container import SnapContainer |
945 | +from reviewtools.containers.base_container import ContainerException |
946 | |
947 | |
948 | # |
949 | @@ -40,7 +42,7 @@ class SnapReviewException(ReviewException): |
950 | """This class represents SnapReview exceptions""" |
951 | |
952 | |
953 | -class SnapReview(Review): |
954 | +class SnapReview(ReviewBase): |
955 | """This class represents snap reviews""" |
956 | |
957 | snappy_required = ["name", "version"] |
958 | @@ -414,34 +416,43 @@ class SnapReview(Review): |
959 | def __init__(self, fn, review_type, overrides=None): |
960 | if review_type is None: # for using utility functions |
961 | return |
962 | - Review.__init__(self, fn, review_type, overrides=overrides) |
963 | + ReviewBase.__init__(self, review_type, overrides=overrides) |
964 | |
965 | - # Anything importing this is assumed to be a snap v2 check |
966 | - if not self.is_snap2: |
967 | - return |
968 | + try: |
969 | + self.snap = SnapContainer(fn) |
970 | + except ContainerException as e: |
971 | + error(e.value) |
972 | + self.pkg_filename = self.snap.filename |
973 | + self.unpack_dir = self.snap.unpack_dir |
974 | + |
975 | + # unpack_pkg() now only supports snap v2, so just hardcode these |
976 | + self.pkgfmt = {"type": "snap", "version": "16.04"} |
977 | + |
978 | + # Get a list of all unpacked files |
979 | + self.pkg_files = self.snap.get_files_list(rel=False) |
980 | + |
981 | + # Setup what is needed to get a list of all unpacked compiled binaries |
982 | + self.pkg_bin_files = self.snap.get_compiled_binaries_list(rel=False) |
983 | |
984 | - snap_yaml = self._extract_snap_yaml() |
985 | + snap_yaml = self.snap.get_file("meta/snap.yaml") |
986 | try: |
987 | - self.snap_yaml = yaml.safe_load(snap_yaml) |
988 | + self.snap_yaml = yaml.safe_load(snap_yaml.decode()) |
989 | except Exception: # pragma: nocover |
990 | error("Could not load snap.yaml. Is it properly formatted?") |
991 | - snap_yaml.close() |
992 | |
993 | # read yaml again for duplicated keys check (py-yaml does not |
994 | # do that) |
995 | - snap_yaml = self._extract_snap_yaml() |
996 | + snap_yaml = self.snap.get_file("meta/snap.yaml") |
997 | try: |
998 | - self._verify_no_duplicated_yaml_keys(snap_yaml.read()) |
999 | + self._verify_no_duplicated_yaml_keys(snap_yaml.decode()) |
1000 | except Exception as e: |
1001 | error("Found duplicated yaml keys in snap.yaml: %s" % e) |
1002 | - snap_yaml.close() |
1003 | |
1004 | self.snap_manifest_yaml = {} |
1005 | - manifest_yaml = self._extract_snap_manifest_yaml() |
1006 | + manifest_yaml = self.snap.get_file("snap/manifest.yaml") |
1007 | if manifest_yaml is not None: |
1008 | try: |
1009 | - self.snap_manifest_yaml = yaml.safe_load(manifest_yaml) |
1010 | - manifest_yaml.close() |
1011 | + self.snap_manifest_yaml = yaml.safe_load(manifest_yaml.decode()) |
1012 | if self.snap_manifest_yaml is None: |
1013 | self.snap_manifest_yaml = {} |
1014 | except Exception: # pragma: nocover |
1015 | diff --git a/reviewtools/sr_functional.py b/reviewtools/sr_functional.py |
1016 | index bbdc56b..6823b8a 100644 |
1017 | --- a/reviewtools/sr_functional.py |
1018 | +++ b/reviewtools/sr_functional.py |
1019 | @@ -36,7 +36,6 @@ class SnapReviewFunctional(SnapReview): |
1020 | |
1021 | def __init__(self, fn, overrides=None): |
1022 | SnapReview.__init__(self, fn, "functional-snap-v2", overrides=overrides) |
1023 | - self._list_all_compiled_binaries() |
1024 | |
1025 | # State files only for base snaps, if have -lln output and |
1026 | # --state-output is specified |
1027 | diff --git a/reviewtools/sr_lint.py b/reviewtools/sr_lint.py |
1028 | index c94340a..fc0c7ef 100644 |
1029 | --- a/reviewtools/sr_lint.py |
1030 | +++ b/reviewtools/sr_lint.py |
1031 | @@ -20,6 +20,7 @@ from reviewtools.common import ( |
1032 | find_external_symlinks, |
1033 | STORE_PKGNAME_SNAPV2_MAXLEN, |
1034 | STORE_PKGNAME_SNAPV2_MINLEN, |
1035 | + error |
1036 | ) |
1037 | from reviewtools.overrides import ( |
1038 | redflagged_snap_types_overrides, |
1039 | @@ -55,8 +56,6 @@ class SnapReviewLint(SnapReview): |
1040 | ] |
1041 | self.iffy_files = [r"^\..+\.swp$"] # vim |
1042 | |
1043 | - self._list_all_compiled_binaries() |
1044 | - |
1045 | self.redflagged_snap_types = ["base", "kernel", "gadget", "os", "snapd"] |
1046 | |
1047 | self.interface_plug_requires_desktop_file = ["unity7", "x11", "unity8"] |
1048 | @@ -273,7 +272,7 @@ class SnapReviewLint(SnapReview): |
1049 | t = "info" |
1050 | n = self._get_check_name("hook_executable", app=hook) |
1051 | s = "OK" |
1052 | - if not self._check_innerpath_executable(fn): |
1053 | + if not os.access(fn, os.X_OK): |
1054 | t = "error" |
1055 | s = "%s is not executable" % rel |
1056 | self._add_result(t, n, s) |
1057 | @@ -349,7 +348,7 @@ class SnapReviewLint(SnapReview): |
1058 | t = "info" |
1059 | n = self._get_check_name("icon_exists") |
1060 | s = "OK" |
1061 | - fn = self._path_join(self._get_unpack_dir(), self.snap_yaml["icon"]) |
1062 | + fn = os.path.join(self._get_unpack_dir(), self.snap_yaml["icon"]) |
1063 | if fn not in self.pkg_files: |
1064 | t = "error" |
1065 | s = "icon entry '%s' does not exist" % self.snap_yaml["icon"] |
1066 | @@ -566,7 +565,7 @@ class SnapReviewLint(SnapReview): |
1067 | ): |
1068 | val = val.lstrip("/") |
1069 | |
1070 | - fn = self._path_join(self._get_unpack_dir(), os.path.normpath(val)) |
1071 | + fn = os.path.join(self._get_unpack_dir(), os.path.normpath(val)) |
1072 | if fn not in self.pkg_files: |
1073 | t = "error" |
1074 | s = "%s does not exist" % (self.snap_yaml["apps"][app][key]) |
1075 | @@ -637,7 +636,7 @@ class SnapReviewLint(SnapReview): |
1076 | ): |
1077 | val = val.lstrip("/") |
1078 | |
1079 | - fn = self._path_join(self._get_unpack_dir(), os.path.normpath(val)) |
1080 | + fn = os.path.join(self._get_unpack_dir(), os.path.normpath(val)) |
1081 | if fn not in self.pkg_files: |
1082 | nonexistent.append(val) |
1083 | |
1084 | @@ -2022,9 +2021,10 @@ class SnapReviewLint(SnapReview): |
1085 | else: |
1086 | appnames.append("%s.%s" % (self.snap_yaml["name"], app)) |
1087 | |
1088 | - fh = self._extract_file(fn) |
1089 | - lines = fh.readlines() |
1090 | - fh.close() |
1091 | + file = self.snap.get_file(fn) |
1092 | + if file is None: |
1093 | + error("Could not find '%s'" % fn) |
1094 | + lines = file.decode().split("\n") |
1095 | |
1096 | # For now, just check Exec=, Icon= and bools since: |
1097 | # - snapd strips out anything it doesn't understand |
1098 | @@ -2168,7 +2168,7 @@ class SnapReviewLint(SnapReview): |
1099 | for f in self.pkg_files: |
1100 | fn = os.path.relpath(f, self._get_unpack_dir()) |
1101 | if fn.startswith("meta/gui/") and fn.endswith(".desktop"): |
1102 | - self._verify_desktop_file(f) |
1103 | + self._verify_desktop_file(fn) |
1104 | has_desktop_files = True |
1105 | break |
1106 | |
1107 | @@ -3221,7 +3221,7 @@ class SnapReviewLint(SnapReview): |
1108 | count = 0 |
1109 | for fn in icons: |
1110 | rel = os.path.relpath(fn, self._get_unpack_dir()) |
1111 | - if fn != shlex.quote(fn): |
1112 | + if rel != shlex.quote(rel): |
1113 | bad_names.append(rel) |
1114 | continue |
1115 | elif not os.path.isfile(fn): |
1116 | diff --git a/reviewtools/sr_tests.py b/reviewtools/sr_tests.py |
1117 | index b23fbe6..ca43c5b 100644 |
1118 | --- a/reviewtools/sr_tests.py |
1119 | +++ b/reviewtools/sr_tests.py |
1120 | @@ -15,7 +15,6 @@ |
1121 | # along with this program. If not, see <http://www.gnu.org/licenses/>. |
1122 | |
1123 | import copy |
1124 | -import io |
1125 | import os |
1126 | import yaml |
1127 | |
1128 | @@ -42,27 +41,7 @@ def _mock_func(self): |
1129 | return |
1130 | |
1131 | |
1132 | -def _extract_snap_yaml(self): |
1133 | - """Pretend we read the snap.yaml file""" |
1134 | - return io.StringIO(TEST_SNAP_YAML) |
1135 | - |
1136 | - |
1137 | -def _extract_snap_manifest_yaml(self): |
1138 | - """Pretend we read the snap/manifest.yaml file""" |
1139 | - return io.StringIO(TEST_SNAP_MANIFEST_YAML) |
1140 | - |
1141 | - |
1142 | -def _path_join(self, d, fn): |
1143 | - """Pretend we have a tempdir""" |
1144 | - return os.path.join("/fake", fn) |
1145 | - |
1146 | - |
1147 | -def _pkgfmt_type(self): |
1148 | - """Pretend we found the pkgfmt type""" |
1149 | - return TEST_PKGFMT_TYPE |
1150 | - |
1151 | - |
1152 | -def __get_unpack_dir(self): |
1153 | +def _get_unpack_dir(self): |
1154 | """Pretend we found the unpack dir""" |
1155 | return TEST_UNPACK_DIR |
1156 | |
1157 | @@ -77,53 +56,28 @@ def _cmd_nm(self, args): |
1158 | return TEST_CMD_NM |
1159 | |
1160 | |
1161 | -def create_patches(): |
1162 | - # http://docs.python.org/3.4/library/unittest.mock-examples.html |
1163 | - # Mock patching. Don't use decorators but instead patch in setUp() of the |
1164 | - # child. |
1165 | - patches = [] |
1166 | - patches.append(patch("reviewtools.common.Review._check_package_exists", _mock_func)) |
1167 | - patches.append( |
1168 | - patch("reviewtools.sr_common.SnapReview._extract_snap_yaml", _extract_snap_yaml) |
1169 | - ) |
1170 | - patches.append( |
1171 | - patch( |
1172 | - "reviewtools.sr_common.SnapReview._extract_snap_manifest_yaml", |
1173 | - _extract_snap_manifest_yaml, |
1174 | - ) |
1175 | - ) |
1176 | - patches.append(patch("reviewtools.sr_common.SnapReview._path_join", _path_join)) |
1177 | - patches.append(patch("reviewtools.common.unpack_pkg", _mock_func)) |
1178 | - patches.append(patch("reviewtools.common.raw_unpack_pkg", _mock_func)) |
1179 | - patches.append( |
1180 | - patch("reviewtools.sr_common.SnapReview._list_all_files", _mock_func) |
1181 | - ) |
1182 | - patches.append( |
1183 | - patch( |
1184 | - "reviewtools.sr_common.SnapReview._list_all_compiled_binaries", _mock_func |
1185 | - ) |
1186 | - ) |
1187 | +class SnapContainer(): |
1188 | + def __init__(self, fn): |
1189 | + self.filename = fn |
1190 | + self.unpack_dir = TEST_UNPACK_DIR |
1191 | + self.files = [] |
1192 | + self.bin_files = [] |
1193 | |
1194 | - patches.append(patch("reviewtools.common.Review._list_all_files", _mock_func)) |
1195 | - patches.append( |
1196 | - patch("reviewtools.common.Review._list_all_compiled_binaries", _mock_func) |
1197 | - ) |
1198 | + def unpack(self): |
1199 | + return |
1200 | |
1201 | - # sr_common |
1202 | - patches.append( |
1203 | - patch("reviewtools.sr_common.SnapReview._get_unpack_dir", __get_unpack_dir) |
1204 | - ) |
1205 | - patches.append(patch("reviewtools.sr_common.SnapReview._pkgfmt_type", _pkgfmt_type)) |
1206 | - patches.append( |
1207 | - patch("reviewtools.sr_common.SnapReview._unsquashfs_lln", _unsquashfs_lln) |
1208 | - ) |
1209 | + def get_files_list(self, rel=True): |
1210 | + return [] |
1211 | |
1212 | - # sr_functional |
1213 | - patches.append( |
1214 | - patch("reviewtools.sr_functional.SnapReviewFunctional._cmd_nm", _cmd_nm) |
1215 | - ) |
1216 | + def get_compiled_binaries_list(self, rel=True): |
1217 | + return [] |
1218 | |
1219 | - return patches |
1220 | + def get_file(self, fn): |
1221 | + if fn == "meta/snap.yaml": |
1222 | + return TEST_SNAP_YAML.encode() |
1223 | + elif fn == "snap/manifest.yaml": |
1224 | + return TEST_SNAP_MANIFEST_YAML.encode() |
1225 | + return "".encode() |
1226 | |
1227 | |
1228 | class TestSnapReview(TestCase): |
1229 | @@ -221,9 +175,33 @@ class TestSnapReview(TestCase): |
1230 | global TEST_CMD_NM |
1231 | TEST_CMD_NM = (rc, s) |
1232 | |
1233 | + def create_patches(self): |
1234 | + # http://docs.python.org/3.4/library/unittest.mock-examples.html |
1235 | + # Mock patching. Don't use decorators but instead patch in setUp() of the |
1236 | + # child. |
1237 | + patches = [] |
1238 | + patches.append(patch("reviewtools.sr_common.SnapContainer", SnapContainer)) |
1239 | + patches.append(patch("reviewtools.common.unpack_pkg", _mock_func)) |
1240 | + |
1241 | + # sr_common |
1242 | + patches.append( |
1243 | + patch("reviewtools.sr_common.SnapReview._get_unpack_dir", _get_unpack_dir) |
1244 | + ) |
1245 | + # patches.append(patch("reviewtools.sr_common.SnapReview._pkgfmt_type", _pkgfmt_type)) |
1246 | + patches.append( |
1247 | + patch("reviewtools.sr_common.SnapReview._unsquashfs_lln", _unsquashfs_lln) |
1248 | + ) |
1249 | + |
1250 | + # sr_functional |
1251 | + patches.append( |
1252 | + patch("reviewtools.sr_functional.SnapReviewFunctional._cmd_nm", _cmd_nm) |
1253 | + ) |
1254 | + |
1255 | + return patches |
1256 | + |
1257 | def setUp(self): |
1258 | """Make sure our patches are applied everywhere""" |
1259 | - patches = create_patches() |
1260 | + patches = self.create_patches() |
1261 | for p in patches: |
1262 | p.start() |
1263 | self.addCleanup(p.stop) |
1264 | diff --git a/reviewtools/tests/containers/test_base_container.py b/reviewtools/tests/containers/test_base_container.py |
1265 | new file mode 100644 |
1266 | index 0000000..d7c6a81 |
1267 | --- /dev/null |
1268 | +++ b/reviewtools/tests/containers/test_base_container.py |
1269 | @@ -0,0 +1,169 @@ |
1270 | +import os |
1271 | +import shutil |
1272 | +import unittest |
1273 | +from tempfile import mkstemp, mkdtemp |
1274 | +from reviewtools.containers.base_container import BaseContainer, ContainerException |
1275 | + |
1276 | + |
1277 | +class TestBaseContainer(unittest.TestCase): |
1278 | + """Tests for reviewtools.containers.base_container.BaseContainer""" |
1279 | + |
1280 | + test_files = ["meta/snap.yaml", "meta/hooks/install"] |
1281 | + test_binaries = ["usr/bin/bash"] |
1282 | + |
1283 | + @classmethod |
1284 | + def setUpClass(cls): |
1285 | + cls._create_unpackdir(cls) |
1286 | + cls._create_container(cls) |
1287 | + |
1288 | + @classmethod |
1289 | + def tearDownClass(cls): |
1290 | + if os.path.exists(cls._container_filename): |
1291 | + os.unlink(cls._container_filename) |
1292 | + if os.path.exists(cls._unpack_dir): |
1293 | + shutil.rmtree(cls._unpack_dir) |
1294 | + |
1295 | + def _create_unpackdir(self): |
1296 | + self._unpack_dir = mkdtemp() |
1297 | + self._unpacked_size = 0 |
1298 | + for file in self.test_files + self.test_binaries: |
1299 | + unpacked_file = os.path.join(self._unpack_dir, file) |
1300 | + os.makedirs(os.path.dirname(unpacked_file), exist_ok=True) |
1301 | + if file in self.test_files: |
1302 | + with open(unpacked_file, "w+b") as fd: |
1303 | + fd.write(b"Test") |
1304 | + else: |
1305 | + shutil.copyfile(os.path.join("/", file), unpacked_file) |
1306 | + self._unpacked_size += os.path.getsize(unpacked_file) |
1307 | + |
1308 | + def _create_container(self): |
1309 | + self._container_size = 10000 |
1310 | + fd, self._container_filename = mkstemp(suffix=".test") |
1311 | + with os.fdopen(fd, "w") as f: |
1312 | + f.truncate(self._container_size) |
1313 | + |
1314 | + # Test class initialization |
1315 | + def test_initialization_container__happy(self): |
1316 | + try: |
1317 | + BaseContainer(self._container_filename) |
1318 | + except: |
1319 | + self.fail( |
1320 | + "An unexpected error occurred during BaseContainer initialization with container file" |
1321 | + ) |
1322 | + |
1323 | + def test_initialization_unpackdir__happy(self): |
1324 | + try: |
1325 | + BaseContainer(self._unpack_dir) |
1326 | + except: |
1327 | + self.fail( |
1328 | + "An unexpected error occurred during BaseContainer initialization with unpacked directory" |
1329 | + ) |
1330 | + |
1331 | + def test_initialization__missing_file(self): |
1332 | + with self.assertRaises(ContainerException) as context: |
1333 | + BaseContainer("/non/existing/path") |
1334 | + self.assertTrue( |
1335 | + "does not exists or has incorrect permissions" |
1336 | + in context.exception.value |
1337 | + ) |
1338 | + |
1339 | + # Test check container size |
1340 | + def test_check_container_size__happy(self): |
1341 | + try: |
1342 | + BaseContainer(self._container_filename).check_container_size( |
1343 | + minSize=self._container_size - 1, maxSize=self._container_size + 1 |
1344 | + ) |
1345 | + except: |
1346 | + self.fail("An unexpected error occurred while checking file size") |
1347 | + |
1348 | + def test_check_container_size__too_big(self): |
1349 | + with self.assertRaises(ContainerException) as context: |
1350 | + BaseContainer(self._container_filename).check_container_size( |
1351 | + maxSize=self._container_size - 1 |
1352 | + ) |
1353 | + self.assertTrue("container file is too large " in context.exception.value) |
1354 | + |
1355 | + def test_check_container_size__too_small(self): |
1356 | + with self.assertRaises(ContainerException) as context: |
1357 | + BaseContainer(self._container_filename).check_container_size( |
1358 | + minSize=self._container_size + 1 |
1359 | + ) |
1360 | + self.assertTrue("container file is too small " in context.exception.value) |
1361 | + |
1362 | + # Test check unpacked size |
1363 | + def test_check_unpacked_size__happy(self): |
1364 | + try: |
1365 | + BaseContainer(self._unpack_dir).check_unpacked_size( |
1366 | + minSize=self._unpacked_size - 1, maxSize=self._unpacked_size + 1 |
1367 | + ) |
1368 | + except: |
1369 | + self.fail("An unexpected error occurred while checking file size") |
1370 | + |
1371 | + def test_check_unpacked_size__too_big(self): |
1372 | + with self.assertRaises(ContainerException) as context: |
1373 | + BaseContainer(self._unpack_dir).check_unpacked_size( |
1374 | + maxSize=self._unpacked_size - 1 |
1375 | + ) |
1376 | + self.assertTrue("container file is too large " in context.exception.value) |
1377 | + |
1378 | + def test_check_unpacked_size__too_small(self): |
1379 | + with self.assertRaises(ContainerException) as context: |
1380 | + BaseContainer(self._unpack_dir).check_unpacked_size( |
1381 | + minSize=self._unpacked_size + 1 |
1382 | + ) |
1383 | + self.assertTrue("container file is too small " in context.exception.value) |
1384 | + |
1385 | + # Test get file |
1386 | + def test_get_file__happy(self): |
1387 | + try: |
1388 | + container = BaseContainer(self._unpack_dir) |
1389 | + snap_yaml = container.get_file("meta/snap.yaml") |
1390 | + self.assertEqual(snap_yaml.decode(), "Test") |
1391 | + except: |
1392 | + self.fail("An unexpected error occurred while getting meta/snap.yaml") |
1393 | + |
1394 | + def test_get_file__missing_file(self): |
1395 | + try: |
1396 | + container = BaseContainer(self._unpack_dir) |
1397 | + file = container.get_file("non/existent/file") |
1398 | + self.assertEqual(file, None) |
1399 | + except: |
1400 | + self.fail("An unexpected error occurred while getting non/existent/file") |
1401 | + |
1402 | + # Test get files list |
1403 | + def test_get_files_list__happy(self): |
1404 | + try: |
1405 | + container = BaseContainer(self._unpack_dir) |
1406 | + self.assertCountEqual( |
1407 | + container.get_files_list(), self.test_files + self.test_binaries |
1408 | + ) |
1409 | + except: |
1410 | + self.fail("An unexpected error occurred while getting files list") |
1411 | + |
1412 | + def test_files__happy(self): |
1413 | + try: |
1414 | + container = BaseContainer(self._unpack_dir) |
1415 | + self.assertCountEqual(container.files, self.test_files + self.test_binaries) |
1416 | + except: |
1417 | + self.fail("An unexpected error occurred while getting files list") |
1418 | + |
1419 | + # Test get binaries list |
1420 | + def test_get_compiled_binaries_list__happy(self): |
1421 | + try: |
1422 | + container = BaseContainer(self._unpack_dir) |
1423 | + self.assertCountEqual( |
1424 | + container.get_compiled_binaries_list(), self.test_binaries |
1425 | + ) |
1426 | + except: |
1427 | + self.fail( |
1428 | + "An unexpected error occurred while getting compiled binaries list" |
1429 | + ) |
1430 | + |
1431 | + def test_bin_files__happy(self): |
1432 | + try: |
1433 | + container = BaseContainer(self._unpack_dir) |
1434 | + self.assertCountEqual(container.bin_files, self.test_binaries) |
1435 | + except: |
1436 | + self.fail( |
1437 | + "An unexpected error occurred while getting compiled binaries list" |
1438 | + ) |
1439 | diff --git a/reviewtools/tests/containers/test_squashfs_container.py b/reviewtools/tests/containers/test_squashfs_container.py |
1440 | new file mode 100644 |
1441 | index 0000000..6bac90f |
1442 | --- /dev/null |
1443 | +++ b/reviewtools/tests/containers/test_squashfs_container.py |
1444 | @@ -0,0 +1,165 @@ |
1445 | +import os |
1446 | +import hashlib |
1447 | +from tempfile import mkstemp, mkdtemp, TemporaryFile |
1448 | +from reviewtools.containers.base_container import ContainerException |
1449 | +from reviewtools.containers.squashfs_container import SquashfsContainer |
1450 | +from reviewtools.tests.containers.test_base_container import TestBaseContainer |
1451 | +from subprocess import run |
1452 | + |
1453 | + |
1454 | +class TestSquashfsContainer(TestBaseContainer): |
1455 | + """Tests for reviewtools.containers""" |
1456 | + |
1457 | + def _create_container(self): |
1458 | + fd, self._container_filename = mkstemp(suffix=".test") |
1459 | + os.unlink(self._container_filename) |
1460 | + # Ensure reproducible build |
1461 | + os.environ["SOURCE_DATE_EPOCH"] = "0" |
1462 | + run(["mksquashfs", self._unpack_dir, self._container_filename]) |
1463 | + os.unsetenv("SOURCE_DATE_EPOCH") |
1464 | + self._container_size = os.path.getsize(self._container_filename) |
1465 | + |
1466 | + # Test initialization |
1467 | + def test_check_initialization__happy(self): |
1468 | + try: |
1469 | + SquashfsContainer(self._container_filename) |
1470 | + except: |
1471 | + self.fail( |
1472 | + "An unexpected error occurred during SquashfsContainer initialization with container file" |
1473 | + ) |
1474 | + |
1475 | + def test_check_initialization__invalid_format(self): |
1476 | + with self.assertRaises(ContainerException) as context: |
1477 | + fd, plain_file = mkstemp() |
1478 | + SquashfsContainer(plain_file) |
1479 | + self.assertEqual( |
1480 | + "unsupported package format (not squashfs)", context.exception.value |
1481 | + ) |
1482 | + os.unlink(plain_file) |
1483 | + |
1484 | + # Test deletion |
1485 | + def test_check_cleanup(self): |
1486 | + try: |
1487 | + squashfsContainer = SquashfsContainer(self._container_filename) |
1488 | + filename = squashfsContainer.filename |
1489 | + del squashfsContainer |
1490 | + self.assertFalse(os.path.exists(filename)) |
1491 | + except: |
1492 | + self.fail("An unexpected error occurred during deletion test") |
1493 | + |
1494 | + # Test check format |
1495 | + def test_check_container_format__missing_container(self): |
1496 | + with self.assertRaises(ContainerException) as context: |
1497 | + SquashfsContainer(self._unpack_dir).check_container_format() |
1498 | + self.assertEqual("container file does not exist", context.exception.value) |
1499 | + |
1500 | + # Test check format |
1501 | + def test_calculate_unpacked_size__happy(self): |
1502 | + try: |
1503 | + # unpacked size calculated is a bit smaller than actual size as it does not consider that |
1504 | + # folders require at least one complete block |
1505 | + squashfsContainer = SquashfsContainer(self._container_filename) |
1506 | + size = squashfsContainer.calculate_unpacked_size() |
1507 | + self.assertGreaterEqual(size, self._unpacked_size) |
1508 | + self.assertLess(size, 1.05 * self._unpacked_size) |
1509 | + except: |
1510 | + self.fail("An unexpected error occurred during unpacked size calculation") |
1511 | + |
1512 | + def test_calculate_unpacked_size__missing_container(self): |
1513 | + with self.assertRaises(ContainerException) as context: |
1514 | + SquashfsContainer(self._unpack_dir).calculate_unpacked_size() |
1515 | + self.assertEqual("container file does not exist", context.exception.value) |
1516 | + |
1517 | + # Test unpack |
1518 | + def test_unpack__happy(self): |
1519 | + try: |
1520 | + squashfsContainer = SquashfsContainer(self._container_filename) |
1521 | + squashfsContainer.unpack() |
1522 | + except: |
1523 | + self.fail("An unexpected error occurred during unpack") |
1524 | + |
1525 | + def test_unpack__force(self): |
1526 | + try: |
1527 | + squashfsContainer = SquashfsContainer(self._container_filename) |
1528 | + squashfsContainer.unpack() |
1529 | + squashfsContainer.unpack(force=True) |
1530 | + except: |
1531 | + self.fail("An unexpected error occurred during unpack") |
1532 | + |
1533 | + # Test check format |
1534 | + def test_unpack__missing_container(self): |
1535 | + with self.assertRaises(ContainerException) as context: |
1536 | + SquashfsContainer(self._unpack_dir).unpack() |
1537 | + self.assertEqual("container file does not exist", context.exception.value) |
1538 | + |
1539 | + def test_unpack__no_force(self): |
1540 | + with self.assertRaises(ContainerException) as context: |
1541 | + squashfsContainer = SquashfsContainer(self._container_filename) |
1542 | + squashfsContainer.unpack() |
1543 | + squashfsContainer.unpack() |
1544 | + self.assertEqual( |
1545 | + "unpack dir already exists and unpack function was called without force", |
1546 | + context.exception.value, |
1547 | + ) |
1548 | + |
1549 | + # Test unpack |
1550 | + def test_pack__happy(self): |
1551 | + try: |
1552 | + squashfsContainer = SquashfsContainer(self._unpack_dir) |
1553 | + squashfsContainer.pack() |
1554 | + except: |
1555 | + self.fail("An unexpected error occurred during pack") |
1556 | + |
1557 | + def test_pack__force(self): |
1558 | + try: |
1559 | + squashfsContainer = SquashfsContainer(self._unpack_dir) |
1560 | + squashfsContainer.pack() |
1561 | + squashfsContainer.pack(force=True) |
1562 | + except: |
1563 | + self.fail("An unexpected error occurred during pack") |
1564 | + |
1565 | + # Test check format |
1566 | + def test_pack__missing_container(self): |
1567 | + with self.assertRaises(ContainerException) as context: |
1568 | + SquashfsContainer(self._container_filename).pack() |
1569 | + self.assertEqual("unpack dir does not exist", context.exception.value) |
1570 | + |
1571 | + def test_pack__no_force(self): |
1572 | + with self.assertRaises(ContainerException) as context: |
1573 | + squashfsContainer = SquashfsContainer(self._container_filename) |
1574 | + squashfsContainer.pack() |
1575 | + squashfsContainer.pack() |
1576 | + self.assertEqual( |
1577 | + "pack dir already exists and unpack function was called without force", |
1578 | + context.exception.value, |
1579 | + ) |
1580 | + |
1581 | + def test_repack(self): |
1582 | + with open(self._container_filename, "rb") as fd: |
1583 | + init_hash = hashlib.sha256(fd.read()).hexdigest() |
1584 | + squashfsContainer = SquashfsContainer(self._container_filename) |
1585 | + |
1586 | + # # Test unpack dir |
1587 | + squashfsContainer.unpack() |
1588 | + original_hash = "" |
1589 | + for path, dirs, files in os.walk(self._unpack_dir): |
1590 | + for file in files: |
1591 | + with open("%s/%s" % (path, file), "rb") as fd: |
1592 | + original_hash += hashlib.sha256(fd.read()).hexdigest() |
1593 | + original_hash = hashlib.sha256(original_hash.encode()).hexdigest() |
1594 | + |
1595 | + unpacked_hash = "" |
1596 | + for path, dirs, files in os.walk(squashfsContainer.unpack_dir): |
1597 | + for file in files: |
1598 | + with open("%s/%s" % (path, file), "rb") as fd: |
1599 | + unpacked_hash += hashlib.sha256(fd.read()).hexdigest() |
1600 | + unpacked_hash = hashlib.sha256(unpacked_hash.encode()).hexdigest() |
1601 | + self.assertEqual(original_hash, unpacked_hash) |
1602 | + |
1603 | + # Test packed file |
1604 | + os.environ["SOURCE_DATE_EPOCH"] = "0" |
1605 | + squashfsContainer.pack(force=True) |
1606 | + os.unsetenv("SOURCE_DATE_EPOCH") |
1607 | + with open(squashfsContainer.filename, "rb") as fd: |
1608 | + late_hash = hashlib.sha256(fd.read()).hexdigest() |
1609 | + self.assertEqual(init_hash, late_hash) |
PR simplified a bit and inline comments added to make the review easier. I think it should be possible to split this PR in several if it help to make it through