build_providers: add a build image setup facility #2192
Conversation
snapcraft/file_utils.py
Outdated
| return path | ||
| elif shutil.which(command_name): | ||
| return shutil.which(command_name) |
There was a problem hiding this comment.
I like the elif here, but not the side effect of the fact that we have to run which twice. I suggest using an if instead so we only need to run it once.
There was a problem hiding this comment.
assignment expressions will come soon :-P
There was a problem hiding this comment.
did is fixed in #2193
snapcraft/file_utils.py
Outdated
| "A tool snapcraft depends on could not be found: {command!r}.\n" | ||
| "Ensure the tool is installed and available, and try again." | ||
| ) | ||
| raise RequiredCommandNotFound(fmt=fmt, command=command_name) |
There was a problem hiding this comment.
Can we refactor this error so the message doesn't depend so much on the caller? Is there a reason it isn't setup so we can just pass the missing command to it and use a standard message?
There was a problem hiding this comment.
I will create a new one and add a TODO, your suggestion was my original idea, but the changes delve too deep into deprecated logic.
There was a problem hiding this comment.
this is mitigated in #2193
| fmt = ( | ||
| "An issue was encountered when trying to retrieve the build image for {base!r}.\n" | ||
| "The server responded with HTTP status code {status_code!r}.\n" | ||
| "Contact the creator of {base!r} for further assistance." |
There was a problem hiding this comment.
We can make this more succinct and get rid of a newline:
"Failed to retrieve build image for {base!r}:"
"The server responded with HTTP status code {status_code!r}.\n"
Contact the creator of {base!r} for assistance."
|
|
||
| fmt = ( | ||
| "An issue occurred when setting up the build image for this project.\n" | ||
| "The command exited with exit code {exit_code!r}." |
There was a problem hiding this comment.
This one could be shortened a bit as well, getting rid of the newline.
"Failed to set up the build image for this project:"
"The command exited with exit code {exit_code!r}."
| fmt = ( | ||
| "Cannot find a suitable build image to use to create snaps for the " | ||
| "base {base!r} and architecture {snap_arch!r}.\n" | ||
| "Contact the creator of {base!r} for further assistance." |
There was a problem hiding this comment.
Suggested rewording:
"Cannot find suitable build image for base {base!r} and architecture {snap_arch!r}.\n"
"Contact the creator of {base!r} for assistance."
| base="core16", | ||
| snap_arch="amd64", | ||
| url="https://cloud-images.ubuntu.com/releases/16.04/release-20180703/ubuntu-16.04-server-cloudimg-amd64-disk1.img", # noqa: E501 | ||
| checksum="79549e87ddfc61b1cc8626a67ccc025cd7111d1af93ec28ea46ba6de70819f8c", # noqa: E501 |
There was a problem hiding this comment.
I thought we decided to not checksum the images for now?
There was a problem hiding this comment.
Hm, yes, the notes. I am going on the fact that it is easier to remove this requirement in the future. Also, the fact that we are using this as a backing store means we need to have different entities for each project (related to pruning). I also had download issues :-)
Eventually we could keep this list online and marshal it from some json living on snapcore.
|
|
||
|
|
||
| def setup(*, base: str, snap_arch: str, size: str, image_path: str) -> None: | ||
| """Setup an instance for base and snap_arch on sized size image_path.""" |
There was a problem hiding this comment.
I'm not parsing this description: "... on sized size image_path" ? Also, mind adding sphinx docs?
There was a problem hiding this comment.
s/on/of/ :man_shrugging:
| # qemu-img parameters: | ||
| # -q: quiet. | ||
| # -f: first image format. | ||
| # -b: backing file. |
| base=self.base, status_code=request.status_code | ||
| ) | ||
| # First create the prefix as tempfile.TemporaryDirectory does not do that for you | ||
| os.makedirs(self._image_cache.file_cache, exist_ok=True) |
| @@ -0,0 +1,137 @@ | |||
| # -*- Mode:Python; indent-tabs-mode:nil; tab-width:4 -*- | |||
There was a problem hiding this comment.
Can you elaborate a little regarding the placement of this file? We have build_providers/_qemu and build_providers/_multipass, and now a build_providers/images.py that is very qemu-specific? What is the plan for multipass support with this interface?
There was a problem hiding this comment.
We will use these to launch the mutlipass images too.
snapcraft/internal/cache/_file.py
Outdated
| @@ -26,12 +26,12 @@ | |||
| class FileCache(SnapcraftCache): | |||
| """Generic file cache.""" | |||
|
|
|||
| def __init__(self): | |||
| def __init__(self, *, taxonomy: str = "files") -> None: | |||
There was a problem hiding this comment.
Github decided to hide my comment, so I'll try again:
I don't know what to think when I see "taxonomy" as an argument. It's a field of study. From how it's used, perhaps "namespace" would make more sense?
There was a problem hiding this comment.
I was having troubles thinking of a word. Your suggestion is so obvious now!
Change get_tool_path to always return an absolute path and fail if the required command cannot be found. The unit tests have been adapted to not be path dependent and tests for retrieving environment dependent paths have been added. LP: #1785320 Signed-off-by: Sergio Schvezov <sergio.schvezov@canonical.com>
Download the correct build image to build a base for the corresponding architecture. Use a cached download if available and setup the original build disk image as the backing store for the newly setup image. LP: #1782779 Signed-off-by: Sergio Schvezov <sergio.schvezov@canonical.com>
Codecov Report
@@ Coverage Diff @@
## master #2192 +/- ##
=========================================
Coverage ? 91.18%
=========================================
Files ? 202
Lines ? 12987
Branches ? 1921
=========================================
Hits ? 11842
Misses ? 777
Partials ? 368
Continue to review full report at Codecov.
|
Signed-off-by: Sergio Schvezov <sergio.schvezov@canonical.com>
Download the correct build image to build a base for the corresponding architecture.
Use a cached download if available and setup the original build disk image as the
backing store for the newly setup image.
LP: #1782779
Signed-off-by: Sergio Schvezov sergio.schvezov@canonical.com
./runtests.sh static?./runtests.sh unit?You test the UI by doing