Add the ability to specify custom build steps when adding modules

[jakub-nt]

No description provided.

[larsewi]

Please explain how this should work.

Do you need it for cfbs convert? If so, please explain why.

Is there a ticket? If so could you link it to the PR and in the last commit?

[jakub-nt]

It should work by using a provided steps list as the added module's "steps" field in cfbs.json.

Yes, it's for cfbs convert, for adding the local module. The usual build steps are not what is desired.

[olehermanse]

Looks good, just some smaller things to fix.

[olehermanse]

I don't think it's necessary to switch from explicit_build_steps to use_default_build_steps. Just use explicit_build_steps all the way down where needed.

[jakub-nt]

The actual build step values would be unused then, there would only be a check whether they're None or not. That would read weird. Dunno.

[olehermanse]

This leaves the module object in a "half done" state, I think one of these would be preferable:

1. Just add the defaults, and you can overwrite module["steps"] in the calling code.
2. Or, send explicit_build_steps all the way down to this function.

[jakub-nt]

This leaves the module object in a "half done" state

I don't see how that would be the case after and not be the case before. The argument to this function is a Module object, which at the time of entering this function will already have some build steps from when it was constructed (and that construction will now use explicit_build_steps, too, so all the required steps should be there already).

[olehermanse]

Okay, are these methods the ones which prompt user about bundle and policy files? If so, we should add a comment about that here.

[jakub-nt]

Yes, those are the methods. I still don't see what you would ideally like to see here, so feel free to suggest.

[olehermanse]

It looks like _handle_local_module() is not really needed for our case? Maybe a better approach is to clarify it a bit (give it a better name / docstring), and avoid calling it in this case?

[jakub-nt]

Yeah, I found this function a bit strange/unclear (e.g. how it appears in a general _add_without_dependencies function) at first. The current code is setting build steps in a few separate places, maybe there could be improvements, I avoided changing the structure for now, since optional params in existing places could do the job.

[olehermanse]

This docstring note could be replaced by an assert.

[jakub-nt]

I would say the note is for the users of the function, for them to learn what the function parameter does. This is why it's in the docstring. An assert is only readable inside of the function.

[olehermanse]

You could have both. An assert is much more important IMO, and less likely to go stale.

[olehermanse]

Same here;

if explicit_build_steps is not None:
    assert len(to_add) == 1, "explicit_build_steps is only for adding a single module"
    assert is_local(to_add[0]), "explicit_build_steps is only for adding a local module"

[jakub-nt]

This is not the same. Are you saying the ability to provide explicit build steps should be limited to adding single local modules only? It seems like it could be useful for adding multiple local modules at once, or for providing custom build steps for modules added from an index or URL.

[olehermanse]

I know it's not exactly the same, but I would add restrictions to try to catch programmer mistakes, rather than notes that look like they could be missed or go stale in the future.

In this case, I think we could place very strict restritctions, yes, only one module, no dependencies, has to be local.

AFAIK that's what we need (for now). The pre-existing add code is a bit messy (not your fault), so it would be nice to have asserts which protect against the unexpected.

If in the future we want to expand it to multiple modules or non-local modules, that is easily doable - remove one assert and ensure it works correctly.