Closed cmacq2 closed 8 years ago
Looks good other than as I mentioned in #28, I'm not seeing a need for creating multiple image files at one time.
Please explain how you see this image registry working. Also, how does the new -I
option work when you have multiple image files?
I'm envisioning having a folder called image-drivers
that contains individual files for each driver. The driver used by a project is specified by setting a variable in the config
file of one of the components. brickstrap searches first $(br_project_dir)/image-drivers
, then $(br_script_path)/image-drivers
for a matching driver. This way you don't have to mess with registering any functions. The file will just be an executable script that creates the image.
Also, how does the new -I option work when you have multiple image files
Drivers are registered with a 'name' that identifies the driver as well as a file type suffix. Names are unique. The disk image file name is generated from the basename of -I
as follows:
<basename>-<driver name>.<file type suffix>
So the necessary identifying information is simply appended to the basename specified with -I
.
E.g., assuming -I test
:
test-default.img
test-my_custom_driver.suffix
Please explain how you see this image registry working.
In a follow up integration PR, we introduce a commandline option (-F
or something) to select which image types to generate. This will mediate between project defaults and end-user choice.
From the configuration developer perspective:
fs-beagle
, fs-rpi
, fs-kvm
).custom-image.sh
file.custom-image.sh
you do something like this:#
# create custom image type:
# $1: disk image file to write.
#
function my_custom_img_driver_func()
{
# ... implement it here...
}
br_register_image_type "my_driver_name" my_custom_img_driver_func "img-file-suffix"
# you can specify multiple functions and register multiple drivers in one custom-image.sh.
# just make sure the names are unique.
config
file you can adjust IMAGE_TYPES
environment variable to include "my_driver_name". Alternatively you can require the user select it with -F my_driver_name
. IMAGE_TYPES
allows the project configuration to override the default image type when no image type has been selected through -F
.From the end-user perspective you can specify the exact set of image types to generate, provided corresponding drivers are available either from brickstrap defaults or from the component selection, using the -F
option.
It's getting late and I'm about to call it quits, so I haven't read your previous comment yet. However, I wanted to let you know that I just pushed a branch with some image drivers so that you can see what I am hoping for in an image driver. Feel free to make comments on it as to how you see it fitting into what you are proposing here.
One thing I would like to do is call a validate
function early in the process to catch as many configuration errors as possible before running through the full build.
It's getting late and I'm about to call it quits, so I haven't read your previous comment yet. However, I wanted to let you know that I just pushed a branch with some image drivers so that you can see what I am hoping for in an image driver. Feel free to make comments on it as to how you see it fitting into what you are proposing here.
The thing I like:
The aspects which I think just don't work right at the moment, and are better addressed in this PR already:
<basename>-bootroot.img
)-f
was specified.case $1 in ... esac
block makes no sense). This also means you can't reuse functions that depend on state, efficiently. In particular your calls to $(br_image_path)
require that the BR_IMAGE_BASE_NAME
and BR_DESTDIR
variables are set. There's currently nothing in brickstrap code to export those from brickstrap itself... Likewise BR_FORCE
is also not available. I don't really want to contemplate the code necessary to export those through e.g. the environment to the image driver.I've updated my PR to provide necessary integration bits as well.
Additionally I added support for validation of custom configuration options (register additional validator functions), and I've also integrated one of your default drivers, so you can see how that should work now. See brickstrap-image-drivers.sh for the example of how I integrated the "redundant rootfs + data" image type.
It seems like if one fails we should just be calling fail and stopping brickstrap altogether.
I think it is preferable to let the create-image step continue as much as it can. During development of a project configuration it may be that you accidentally break more than one image type, so it seems preferable to have a somewhat more comprehensive report (via the error messages) than just failing immediately on the first bad image.
Also, it makes it slightly easier for an end-user to work around a broken image type: even if one image build fails, another might succeed and present a viable interim alternative.
<basename>-<driver name>.<file type suffix>
I would really rather not have the driver name part of the file name if I am creating only one image file.
To keep things consistent with the images I have been publishing, the image name should just be <dest>.img
where <dest>
is the -d
option.
I would really rather not have the driver name part of the file name if I am creating only one image file.
Sure, that can be done. I don't really see/understand why that is necessary, though. Question: would this special case behaviour also extend to not enforcing the corresponding file suffix (i.e. -I
must be passed the full filename)?
To keep things consistent with the images I have been publishing, the image name should just be...
You could also execute brickstrap a bit differently: simply specify -I
. If we agree to special case the single-driver case, then it should be sufficient to specify -I
the way you want it (without suffix)...
How about the case when you want to make multiple images using the same driver with different parameters, for example IMAGE_SIZE? Rather than trying to handle such cases in brickstrap, I propose that we just advocate the following...
Brickstrap only creates one image file when you run brickstrap ... all
. The driver that is used is specified in the config
file in the project. The resulting file name is <dest>.<ext>
where <dest>
is the -d
option and <ext>
is determined by the image driver.
If you want to override the default settings of the driver, then use the corresponding environment variables. e.g. IMAGE_FILE_SIZE=1800M brickstrap ... all
.
If you want to override the driver, use the -l
option. e.g. brickstrap ... -l beaglebone all
If you want to override the basename of the image file, use the -I
option. e.g. brickstrap ... -I my-custom all
(results in my-custom.img
)
If you want to build additional images, then use the create-image
command after the all
command.
brickstrap ... -I my-project-4GB all
IMAGE_FILE_SIZE=1800M brickstrap ... -I my-project-2GB create-image
IMAGE_FILE_SIZE=900M brickstrap ... -I my-project-1GB create-image
brickstrap ... -l beaglebone -I my-project-beaglebone-4GB create-image
For now, you can abuse the create-report
hook if you need to automate creating more than one image.
How about the case when you want to make multiple images using the same driver with different parameters, for example IMAGE_SIZE? Rather than trying to handle such cases in brickstrap, I propose that we just advocate the following...
Uh: you can do so right now using the exact same create-image workflow you suggest already. There is no extra handling or support in brickstrap required for that, and I certainly do not propose we add it (I don't see what this would buy us, see next paragraph).
Besides: how often would you want to do that versus wanting to make different image types? I'd think that between those two cases the scales would tip in favour of different types over sizes -- mainly because you generally either do not statically fix the size of the image or you have a very particular target in mind anyway (so only one specific size).
Being able to generate multiple types of images as part of the same build command isn't all that exotic or out-there. It's kind of how you roll with tools like yocto/open-embedded (it is one of the few features of open-embedded that is actually easy to use for a novice user).
The resulting file name is
. where is the -d option and is determined by the image driver.
Let's keep the inferred default file name for the image as it is in this PR. I think the alternative has a few drawbacks:
-l
option was passed to build the image.I think my main objection is that the proposal doesn't make the brickstrap code significantly less complicated (all it really would amount to is removing two outer loops that invoke two particular functions so you can invoke those two functions with fixed arguments directly). At the same time it also does not make brickstrap any easier to use, having to fiddle with options.
So I don't see what we gain by reverting to generating only a single fixed image, albeit one selected via commandline options.
About yocto specifically: this section of its manual explains how image generation works with yocto/open-embedded.
TL;DR summary is: you set up the variable IMAGE_FSTYPES in a local build config
Uh: you can do so right now using the exact same create-image workflow you suggest already.
The point that I am trying to make is that if this already works, why are we inventing a new method to do the same thing? The complexity is not in the implementation, but rather in trying to explain "if you want to make more than one image, you can do it this way, but it only works in some cases otherwise, you have to do it this other way". Just saying "do it this way" is much simpler.
how often would you want to do that versus wanting to make different image types?
Always. I can't think of a single real-life use case for me personally (distributing images for EV3, RPi1/2 and BeagleBone) that could not be accomplished by using the default image driver and environment variables (that would be set in the config
of the appropriate component directory). In the example drivers I gave earlier, the two cases that I would actually use are the default and the beaglebone. However, the beaglebone is identical other than the two extra dd
s for the bootloader, so it doesn't really make sense to make it a separate driver. I would just make it part of the default driver and put if statements around the beaglebone specific stuff so that it is ignored unless the variables are set. In real life, I wouldn't actually generate various sized images, I was just giving that as a simple example of using a variable in the context of the existing default image driver.
Although the full truth is that the answer is neither. I don't have a single real-life case where I would want to produce more than one image from a given rootfs.
Let's keep the inferred default file name for the image as it is in this PR.
That's fine as long as there is some way I can have full control over the entire basename of the file name (not including the file extension).
The timestamp embedded in the file name is actually kind of useful
By default, (i.e. the -d
option is omitted), the destination directory should have the timestamp in the directory name already. So, by using <dest>
as the image basename, it should have the timestamp in it already.
Offtopic: The reason I keep pushing back so hard about these theoretical use cases is exactly that - they are theoretical. I maintain brickstrap because it meets a real need for me (distributing images for the ev3dev.org project). If others come along with real needs and want to modify brickstrap in a way that can meet their need as well without breaking my needs, then great. However, I am quickly getting tired of spending my time thinking about what someone might want to do instead of working on something that actually meets a real need. @BrendanSimon has a real need for something other than the default image driver, so if we can make some sort of hook for him to latch on to, then great, but I expect him to do most of the work for actually implementing the driver to meet his needs. I would like to keep brickstrap as simple and minimalistic as possible. If you are interested in something more all-encompassing, maybe checkout ELBE that I heard about recently on the emdebian mailing list.
These are great ideas that you have, I just can't justify spending the time it is taking me to keep up at this rate with your innovations and really understand what you are doing. It is unfortunate, but that is the way it is.
Or if you want to reverse the roles where you are the maintainer and I am a contributor that is an option too. :smile:
OK: that is fair enough.
Going forward, it seems that the basic functionality of the PR is agreed on but that image generation should be restricted to a single image.
By default, (i.e. the -d option is omitted), the destination directory should have the timestamp in the directory name already. So, by using
as the image basename, it should have the timestamp in it already.
This is not currently the case: by default it is actually $(pwd)
. I think that is actually preferable in the single image case because it means you can issue multiple create-image
commands without specifying 'redundant' options or risking the default getting in your way by 'changing' due to elapsed time.
I'll readjust the PR to take your feedback into account and rework the IMAGE_TYPES
variable to a DEFAULT_IMAGE_TYPE
(to be used as a default only).
Ok, so how does this version look which reduces the number of images being generated in a single brickstrap back down to one and ensures you have full control over the image base name (sans suffix) through -I
.
By default an image name looks like this: <destdirname>-<timestamp>-<driver>.<suffix>
Using the -I <custom_name>
option it becomes: <custom_name>.<suffix>
I'll update the PR to include the fixes found.
You may want to wait a bit. I still can't get brickstrap to run.
I still can't get brickstrap to run.
At what point does it fail? I'm currently doing a test run and it proceeds happily to download stage with options: -I foo -p /full/path/to/my/project -c . all
Somewhere in brp_parse_cli_options
. Haven't figured it out exactly. My command line is
brickstrap.sh -p ev3dev-jessie -c base -c debian -c ev3 all -f
Found the problem. #35
br_register_image_type
now emits an error message with fail
if arguments are missing or empty.
Thanks for compromising with me.
This commit provides preliminary support for custom partitioning schemes/image file types. For discussion refer to: issue #10: https://github.com/ev3dev/brickstrap/issues/10
This change assumes some artefacts provided by PR #28 are available: https://github.com/ev3dev/brickstrap/pull/28 Specifically: br_image_dir() and br_image_path() functions.
This change does not address integration within brickstrap.sh itself (should be trivial, though).