Tests are broken on CentOS

It is also worth mentioning that these failures happen in the same way on Red Hat EL 7.5 as well, which is the upstream distro of CentOS.

As mentioned in #buildstream it looks like most of the failures are coming from git, leading to an assumption that it could be down to the host git config/version and the recent changes made to the git plugin.

added Bug label

Related MR that demonstrates this failure: !1053 (closed).

Once we fix the issue, this MR would ideally start passing.

The git version is 1.8.3.1

docker run --rm -it buildstream/testsuite-centos:7.6-917af42e-c3153dea git --version
git version 1.8.3.1

Regardless of whether or not it ought to have an effect, that version of git is over five years old, so there'll be a LOT to trawl through if we were to try and determine changes in behaviour from the git side. As such, we probably need to break down some of the failing tests more carefully.

OOI were the tests working before the changes to the git plugin (how we interact with tags it seems) landed?

I hadn't have tested in ~1 month, but I had a time were everything was working yes

For what it's worth, using the centos dockerfile I've been able to see the same tests failures. Sadly I've not had enough time to poke at the git tag / describe changes that have caused this in buildtstream as of yet

It's outputting that there are 3 commits after the tag when the test expects it to say there are 2.

The test produces a tree like:

  I
  |
  F1 ← refs/tags/tag1
 /  \
F3  F2 ← refs/heads/branch2, refs/tags/tag2
 \  /
  M ← master, HEAD

Frankly 1 would be my interpretation of what the answer should be, but the algorithm is defined as the commits reachable from HEAD minus the commits reachable from tag2, so the two commits it's saying are after tag2 are F3 and M.

(git log has the --ancestry-path option for limiting it to the path between the listed commits, which if describe had a similar option would limit the "commits after" to 1.)

In the version of git running in CentOS, the produced tree looks like:

  F1 ← refs/tags/tag1
 /
F3  F2 ← refs/tags/tag2
 \  /
  M ← master, HEAD

Since the link between F2 and F1 is missing it doesn't know that F1 is reachable from F2, so it is included in the count.

At a guess I'd say that improvements to the shallow commit traversal algorithm since the version in CentOS mean that it doesn't matter that F2 shouldn't have been marked as a shallow commit.

Adding an extra step to check all the shallow commits to see if all of their parents are in the included tree and not write them as shallow if they are.

This still has the edge case of shallow commits who only have some of their parents included, which is presumably why the algorithm was changed since that version to only omit parents in shallow commits if that parent is missing.

(as an aside, the current algorithm for walking the tree can include commits that are not part of the required history since it is not using --ancestry-path).

719ade06 will fix that problem, however there are other problems with the old version of git that make the tests continue to fail.

https://gitlab.com/BuildStream/buildstream/blob/719ade06cdcf5af43e95b0cc06b091b14bc9c1b3/tests/sources/git.py#L872 will throw an exception because the old version of git won't have --first-parent. This could be handled by parameterising the test so it's not run if your version of git is too old. This isn't ideal, but not a problem if other platforms are proving the history is there.

https://gitlab.com/BuildStream/buildstream/blob/719ade06cdcf5af43e95b0cc06b091b14bc9c1b3/tests/sources/git.py#L876 however demonstrates that it is not, since the git source plugin will treat the absence of --first-parent as a non-fatal error, it won't notice that it should include tag1, so the test fails.

Ironically this has also proven that the current implementation is also including commit history that it does not need to, since the commit that the missing tag would point to is still present, so the number of commits remains the same.

Given the lack of --first-parent isn't treated as a fatal error, and instead produces a different cache key I'm assuming this behaviour is intentional, so I'm going to amend the test suite to check whether --first-parent is missing and deal with tag1 not being there.

Given the lack of --first-parent isn't treated as a fatal error, and instead produces a different cache key I'm assuming this behaviour is intentional, so I'm going to amend the test suite to check whether --first-parent is missing and deal with tag1 not being there.

I'm not sure we should have a different key there. I might be misunderstanding or this might be an oversight but it might be good to check the intent with the author of this part :)

mentioned in merge request !1066 (closed)

@valentindavid we could do with your input, since it determines whether to accept the change, or require a newer version of git, and not support that version of CentOS.

The reason we ignore errors originally is because there might be no tag using first parents. So the silent error here was not intended for that purpose.

However, this code happens only during tracking. So reproduction is not an issue here. If you track with the wrong git version, you still build what is explicitly saved in the element or in project.refs. What we build is exactly what is in the element and project.refs.

So yes. We can just ignore those tests when git version detected to be too old.

Other solutions:

Fail track when git version is too old.
Warn when tracking and git version is too old.
Disable git describe feature when git version is too old.
Traverse manually the graph to find the tags.

Minimum version for --first-parent is 1.8.4 from August 23rd, 2013. (actually since rc0). https://github.com/git/git/commit/e00dd1e9485c50f202cc97dfae19d510e108b565

It was not very obvious to understand. The behavior of older and newer git should be the same during build.

Coming back to this graph:

  F1 ← refs/tags/tag1
 /
F3  F2 ← refs/tags/tag2
 \  /
  M ← master, HEAD

F1 is included because git rev-list --boundary lists F1 as a boundary. For the moment we include all boundary commits.

This has nothing to do with the fact there is tag on it. This is in the output of git rev-list --boundary tag2..master.

@richardmaw-codethink is right, using --ancestry-path seems to fix issue. The good news is that it works with git rev-list.

mentioned in commit 69c11a32

mentioned in merge request !1069 (merged)

mentioned in commit f379b131

@richardmaw-codethink I have fixed the issue with too many commits included in !1069 (merged). I have also fixed another bug I came across while testing. We do not track properly when the HEAD is tagged. Could you review it?

@valentindavid I have reviewed.

There's other issues causing tests to break on CentOS, so I'm going to work through them and list them as I go before putting together an MR.

mentioned in commit e4788dde

mentioned in commit 0eac4008

mentioned in merge request !1085 (merged)

I submitted !1085 (merged) to make the test suite pass, and included !1053 (closed) to test whether it works generally or just worked in my own container.

closed via merge request !1085 (merged)

mentioned in commit 33782865

Tests are broken on CentOS

Summary

Steps to reproduce

What is the current bug behavior?

What is the expected correct behavior?

Child items 0

Activity