Re: Proposal: Run configure-commands on-demand for workspaced elements

[Tristan Van Berkom <tristan vanberkom codethink co uk>]

Thanks for the write up Chandan !

On Mon, 2017-11-13 at 19:18 +0000, Chandan Singh wrote:
> Overview
> ========
>
> While I was working on adding support for incremental builds [1], we realized
> that incremental builds might not actually work for several projects if the
> `configure-commands` of the build elements run each time [2]. This is because
> running `configure-commands` might re-generate files such as `config.h` with
> new timestamps, which will cause everything else to rebuild as well. Thereby,
> eliminating any benefit that incremental builds might have provided.
>
> Proposed Solution
> =================
>
> One way we can work around this problem is by not running configure-commands
> by default for workspaced elements after the first time they are built. If
> user wishes to run the configure-commands again, that can be done on-demand.
> To add this functionality, we will need to store some state about whether the
> configure-commands have been run yet.
>
> Each time user tries to build an element that has any workspaces open,
> BuildStream would use the local state to figure out if the configure-commands
> have been run in past for this element. If yes, then the configure-commands
> are excluded from the build pipeline. Otherwise, configure-commands are run
> as usual but at the end of the build, we store the fact that these commands
> have been run in local state.
>
> Changes to .bst/workspaces.yml
> -------------------------------
>
> Since we already have a file that stores information about currently open
> workspaces - `.bst/workspaces.yml` - it would make sense to use the same file
> to the state of configure-commands being run. The current structure of the file
> looks something like this:
>
>     path/to/my/element.bst:
>       0: /path/to/workspace
>
> I was thinking to add a new `needs-configure` option to this file. With that
> option, the config file will look something like this:
>
>     path/to/my/element.bst:
>       sources:
>         0: /path/to/workspace
>       needs-configure: false
>
> Anytime a new workspace is opened for an element, we can set needs-configure
> option for that element to true and each time a build finishes successfully,
> we mark needs-configure to false for that element. If an element does not has
> any workspaces open, then needs-configure is always assumed to be true and
> configure-commands are run each time.
>
> We have two options about where to store the information about
> configure-commands being run - we can either store that information as a
> property of the element or as a property of the workspace. In either case,
> the structure of this file would change and whenever the new version of
> BuildStream (with these changes) rolls out, it will break people's existing
> workspaces, if they have any.
>
> I was wondering what should be our strategy about this kind of change? Should
> we have a temporary workaround in place that can convert old format of the file
> to the new format, should we just print a helpful error message and expect
> users to fix it themselves, or is something that's off-limits and we should
> think of other ways of storing this information without breaking current
> format?

I think maybe a build count might be more abstract and cover the same
effect, i.e. initially opening a workspace has a build count of 0 (who
knows, one day we might want to consider this metadata for other
reasons than configure commands, better to implement some recorded
workspace state that is abstract from it's initial purpose).

For the concern of backwards compatibility, one need only consider the
absence of the attribute in the same way as a build count of 0, simple.

> Changes to bst command(s)
> -------------------------
>
> Since the configure-commands will not run by default for workspaced elements,
> it will make sense to provide some way of allowing users to re-run those
> commands. At present, I have two options in mind, both of which have their own
> pros and cons:
>
> Option 1 - `bst build --reconfigure`
>
> This option would either throw an error or do nothing (another thing to decide)
> for elements that do not have any workspaces open. For elements that do have
> a workspace open, it is going to set the needs-configure option to true. Since
> the re-running of configure-commands can be treated as part of the build, it
> might be more intuitive for users.
>
> Option 2 - `bst workspace reset --soft`
>
> With this approach, the user would have to soft reset the workspace as a
> preparatory step, which would set the needs-configure option in the
> `.bst/workspaces.yml` and the subsequent build will run the configure-commands
> again.
>
> I'm personally leaning towards option 1 because it also has the advantage that
> it is one-step process in case someone wants to re-run the configure-commands
> but I'm very interested to hear how others feel about this.

I lean towards option 2 because it stays confined to workspace commands
and doesnt creep in too much in the overall api surface.

BUT, I would say that both of these should be discarded for the initial
proposal, this is already possible by closing and reopening the
workspace on an existing directory - it is desirable to keep the change
as small and atomic as possible, and consider other UX improvements
separately.


Finally, the most important part of this proposal is how we communicate
this state to Elements. Remember that BuildElement is not a special
core thing, it's there for convenience, this is the most sensitive part
of the change we need to be considering.

> From what I read in the proposal so far:

  "we can set needs-configure option for that element to true and each
   time a build finishes successfully, we mark needs-configure to false
   for that element"

Well, maybe it's this, again "configure commands" is a thing that is
specific to BuildElement, not abstract enough in my opinion.

Maybe if we communicated to the element "the amount of times it has
been built in the same sandbox" (and only *maybe* is that sandbox a
workspace... we dont really need to tell the Element api about
workspaces), then elements such as BuildElements can use that
information to deduce that setup operations when prepare the state of
their Sources need not be run a second time.

In the case of a BuildElement, it would avoid running the configure
commands redundantly.


Cheers,
    -Tristan