Cache key calculation fails to take dependency type into account in some cases #711

Open Cynical-Optimist opened 3 years ago

See original issue on GitLab In GitLab by [Gitlab user @ben-brewer-ct] on Oct 12, 2018, 13:33

Summary

When changing the type of a dependency, the cache key for the element doesn't always change.

Steps to reproduce

For any artifact which currently has a dependency which is "type: build": Remove the "type: build" so that it's type is "runtime+build". The cache key will not change.

What is the current bug behavior?

The cache key doesn't change when switching between "build" and "build+runtime" dependency types.

What is the expected correct behavior?

The cache key should change because the behaviour of the build changes.

Relevant logs and/or screenshots

I can't post the logs due to non-disclosure, but I may attempt to create an example if it's required.

Possible fixes

Unconditionally add the dependency type to the cache key calculation.

Other relevant information

In buildstream there are 3 dependency types:

build which happens with "type: build"
runtime which happens with "type: runtime"
build+runtime which happens when there's no type field

It would be easy to believe that "build" and "build+runtime" would cause the element to be identical and as such the cache key wouldn't change, because you'd think that "runtime" depends have no effect at build-time, however this is not the case:

For example imagine a gcc runtime dependency, it will be included at build time by anything which builds gcc, because gcc would need that to run.

Stated clearly:

If A build depends on B and B build depends on C. C is not staged at build time for A.
If A build depends on B and B build+runtime depends on C. C is staged at build time for A because it's needed to run B.

So the difference between "build" and "build+runtime" manifests at build time, and needs to be handled by buildstream.

This error is very likely to be encountered in any situation where you're aiming for a minimal set of dependencies. As it makes sense to start out having everything as a build-depend until you know that it's needed as a build+runtime dependency.

In my case I now have to delete cache elements to force a rebuild to see the correct behaviour.

This bug has directly caused builds that should fail to succeeed on my machine.

BuildStream version affected: /milestone %BuildStream_v1.2.3

In GitLab by [Gitlab user @ben-brewer-ct] on Oct 12, 2018, 13:38

changed the description

In GitLab by [Gitlab user @ben-brewer-ct] on Oct 12, 2018, 13:40

changed the description

In GitLab by [Gitlab user @ben-brewer-ct] on Oct 12, 2018, 13:40

changed the description

In GitLab by [Gitlab user @ben-brewer-ct] on Oct 12, 2018, 13:55

changed the description

In GitLab by [Gitlab user @ben-brewer-ct] on Oct 12, 2018, 13:55

changed the description

In GitLab by [Gitlab user @juergbi] on Oct 15, 2018, 15:43

Do you see this issue with strict or non-strict build mode?

In GitLab by [Gitlab user @ben-brewer-ct] on Oct 15, 2018, 15:44

Whatever is the default.

In GitLab by [Gitlab user @ben-brewer-ct] on Oct 15, 2018, 15:55

Just tested with --strict and the same issue occurs.

In GitLab by [Gitlab user @juergbi] on Oct 15, 2018, 16:08

Interesting, I don't see yet why this doesn't work as expected. The cache keys of the build dependencies and their runtime dependencies are included when calculating the cache key. I.e., in your first case only the cache key of B should affect the cache key of A, but in the second case the cache keys of both B and C should affect the cache key of A. Thus, the cache key of A should change when switching from build to build+runtime.

In GitLab by [Gitlab user @tristanvb] on Oct 15, 2018, 16:23

Thanks for commenting [Gitlab user @juergbi], I forgot to come back to this but had it in the back of my mind.

When changing the type of a dependency, the cache key for the element doesn't always change.

While the above statement is true, it, in itself is not a problem (for instance, if you add a new runtime only dependency which is not required at build time, then the same artifact can logically be reused and does not need to be rebuilt for this new runtime dependency, this is untrue of reverse build dependencies of the same element, and I think this is the case [Gitlab user @ben]-brewer-ct is pointing out has a problem).

I may be wrong, but I believe the dependency cache keys considered while constructing a cache key, includes all of the cache keys for elements in Scope.BUILD (i.e.: all of the cache keys of elements which are going to be staged to build).

Maybe we are not using Element.dependencies() to discover the keys, and are instead employing a different manual algorithm to obtain the keys at cache key calculation time ? I will have to look deeper...

In the meantime, I think a good thing to do would be to create a failing test case which demonstrates this happening.

In GitLab by [Gitlab user @toscalix] on Oct 18, 2018, 11:58

[Gitlab user @valentindavid] and [Gitlab user @ben]-brewer-ct went over this issue during the BuildStream Gathering

In GitLab by [Gitlab user @toscalix] on Oct 18, 2018, 12:14

assigned to [Gitlab user @valentindavid]

In GitLab by [Gitlab user @valentindavid] on Oct 18, 2018, 14:07

[Gitlab user @toscalix] That was not that issue we talked about.

In GitLab by [Gitlab user @ben-brewer-ct] on Oct 18, 2018, 14:10

[Gitlab user @toscalix] I would still say this bug is critical. It's the main reason we haven't yet got runtime dependencies correct on our project and it is definitely causing incorrect builds.

Can you please explain to me why this is not critical, or undo your changes to the tags.

In GitLab by [Gitlab user @valentindavid] on Oct 25, 2018, 16:54

[Gitlab user @ben]-brewer-ct I cannot reproduce the issue you explained. I wrote a unit test that covers your description. But it does not fail.

Either I misunderstood your explanation or your explanation does not correctly describe the issue you are currently seeing.

Here is the patch for the unit test test_711.patch.

Either point to me what is missing on the unit test, or please provide us with a simple example that can reproduce the issue.

In GitLab by [Gitlab user @toscalix] on Oct 29, 2018, 09:06

assigned to [Gitlab user @ben-brewer-ct]

In GitLab by [Gitlab user @toscalix] on Oct 29, 2018, 09:06

unassigned [Gitlab user @valentindavid]

In GitLab by [Gitlab user @ben-brewer-ct] on Oct 30, 2018, 16:59

I can reproduce this bug trivially on my machine by removing the "kind: build" field from a dependency and doing a re-build.

I am happy to show [Gitlab user @valentindavid] the issue in person if he wishes.

In GitLab by [Gitlab user @ben-brewer-ct] on Oct 30, 2018, 16:59

assigned to [Gitlab user @valentindavid] and unassigned [Gitlab user @ben-brewer-ct]

In GitLab by [Gitlab user @toscalix] on Nov 1, 2018, 09:37

assigned to [Gitlab user @tlater]

In GitLab by [Gitlab user @toscalix] on Nov 1, 2018, 09:38

[Gitlab user @tlater] can you take a look at the bug with [Gitlab user @ben]-brewer-ct when you are at the office?

In GitLab by [Gitlab user @devcurmudgeon] on Nov 20, 2018, 12:26

As I understand it this bug shows bst failing to handling runtime vs build time dependencies accurately. That's fundamental functionality, so this has to be critical, not just important IMO

In GitLab by [Gitlab user @ben-brewer-ct] on Nov 20, 2018, 15:52

I'm trying to replicate this issue with latest master on the private codebase I was working on (which [Gitlab user @tlater] also has access to), however BuildStream master won't build this project and reports the following bug:

[00:00:02][820e7a94][build:bootstrap-junction.bst:gnu-config.bst] BUG     Build

    An unhandled exception occured:

    Traceback (most recent call last):
      File "/home/flatmush/buildstream/buildstream/_scheduler/jobs/job.py", line 417, in _child_action
        result = self.child_process()  # pylint: disable=assignment-from-no-return
      File "/home/flatmush/buildstream/buildstream/_scheduler/jobs/elementjob.py", line 94, in child_process
        return self._action_cb(self._element)
      File "/home/flatmush/buildstream/buildstream/_scheduler/queues/buildqueue.py", line 71, in process
        return element._assemble()
      File "/home/flatmush/buildstream/buildstream/element.py", line 1561, in _assemble
        self.stage(sandbox)
      File "/home/flatmush/buildstream/buildstream/buildelement.py", line 223, in stage
        self.stage_sources(sandbox, self.get_variable('build-root'))
      File "/home/flatmush/buildstream/buildstream/element.py", line 796, in stage_sources
        self._stage_sources_in_sandbox(sandbox, directory)
      File "/home/flatmush/buildstream/buildstream/element.py", line 1368, in _stage_sources_in_sandbox
        self._stage_sources_at(host_vdirectory, mount_workspaces=mount_workspaces)
      File "/home/flatmush/buildstream/buildstream/element.py", line 1408, in _stage_sources_at
        source._stage(temp_staging_directory)
      File "/home/flatmush/buildstream/buildstream/source.py", line 672, in _stage
        self.stage(staging_directory)
      File "/home/flatmush/buildstream/buildstream/plugins/sources/git.py", line 495, in stage
        self.mirror.stage(directory, track=(self.tracking if not self.tracked else None))
    AttributeError: 'GitTagSource' object has no attribute 'tracked'

In GitLab by [Gitlab user @tlater] on Nov 20, 2018, 16:08

[Gitlab user @ben]-brewer-ct that's pretty odd, I have not come across that error yet. My last clone is at most two weeks ago, so it's a recent regression (if it's not deprecated functionality).

In GitLab by [Gitlab user @juergbi] on Nov 20, 2018, 16:09

I think this is a bug in the git_tag plugin in bst-external. My branch there might fix this: https://gitlab.com/BuildStream/bst-external/tree/juerg/git

Side note, I think we shouldn't do such unsupportable monkey patching in bst-external.

In GitLab by [Gitlab user @valentindavid] on Nov 20, 2018, 17:05

[Gitlab user @juergbi] I have created MR BuildStream/bst-external!61 that does not do monkey patching and instead modifies the 1.2 version of the git plugin. That should be safer.

In GitLab by [Gitlab user @tlater] on Nov 21, 2018, 11:55

I have acquired a copy of the project that reproduces this behavior from [Gitlab user @ben-brewer-ct], I'm trying to debug using that.

So far it's proven difficult mostly because the project in question is not trivial to build - it's fairly large, and various elements' sources fail to fetch/track.

I'm hoping to make some actual progress on building this today, though, everything builds pretty far down the pipeline at this point :)

In GitLab by [Gitlab user @tlater] on Nov 28, 2018, 17:45

I have spent a lot more time getting that project to build than I'd have liked to, sorry for making so little progress here :|

I will have to cut down on BuildStream time even further for a few weeks due to university obligations - I'll unassign myself for the time being, but will revisit if it is still open when I get back.

If anyone else wants to have a look in the mean time, feel free to ping me for more info. I've made little progress on a reproduction though, and can't share the project, so will probably be of limited use.

In GitLab by [Gitlab user @tlater] on Nov 28, 2018, 17:46

unassigned [Gitlab user @tlater]

buildstream-migration / bst-staging