Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

build: merges are incorrectly handled when calculating a build tag #225

Closed
isaac-io opened this issue Nov 9, 2022 · 0 comments · Fixed by #226
Closed

build: merges are incorrectly handled when calculating a build tag #225

isaac-io opened this issue Nov 9, 2022 · 0 comments · Fixed by #226
Assignees
Labels
bug Something isn't working build Build related
Milestone

Comments

@isaac-io
Copy link
Contributor

isaac-io commented Nov 9, 2022

Due to the use of naked git rev-list, the build tag attempts to calculate a build tag spanning the entire merge commit history from the other parent, resulting in incorrect base calculation for both the base tag and the base branch. Additionally, when long histories are merged, the calculation of the build tag takes a very long time due to the inclusion of all of these commits.

To Reproduce

  1. Checkout the 7.8 merge branch from Merge RocksDB 7.8 into Speedb #219
  2. Cherry pick the build tag calculation
  3. Run ./build_tools/spdb_get_build_tag.py
  4. After waiting a very long time (60+ seconds on a Mac, according to @mrambacher), you'll get the following incomprehensible build tag:
(#merge-7.8+108)-(yuval/comp_precut2-squashed-10655+7)-(#merge-7.8+1)-(yuval/comp_precut2-squashed-10655+7)-(#merge-7.8+1)-(yuval/comp_precut2-squashed-10655+7)-(#merge-7.8+1)-(yuval/comp_precut2-squashed-10655+54)-(#merge-7.8+4)-(yuval/comp_precut2-squashed-10655+25)-(#merge-7.8+1)-(yuval/comp_precut2-squashed-10655+55)-(#merge-7.8+1)-(yuval/comp_precut2-squashed-10655+48)-(#merge-7.8+1)-(yuval/comp_precut2-squashed-10655+3)-(#merge-7.8+4)-(yuval/comp_precut2-squashed-10655+199)-(#merge-7.8+1)-(yuval/comp_precut2-squashed-10655+7)-(#merge-7.8+2)-(yuval/comp_precut2-squashed-10655+4)-(#merge-7.8+2)-(yuval/comp_precut2-squashed-10655+7)-(#merge-7.8+1)-(yuval/comp_precut2-squashed-10655+5)-(#merge-7.8+2)-(yuval/comp_precut2-squashed-10655+7)-(#merge-7.8+69)-(main+1)-(#merge-7.8+3)-(main+1)-(#merge-7.8+7)-(main+1)-(#merge-7.8+1)-(main+3)-(#merge-7.8+3)

Expected behavior

A correct build tag is calculated in a reasonable duration of time, especially in incremental builds.

Additional context

N/A

@isaac-io isaac-io added bug Something isn't working build Build related labels Nov 9, 2022
@isaac-io isaac-io added this to the v2.2.0 milestone Nov 9, 2022
@isaac-io isaac-io self-assigned this Nov 9, 2022
isaac-io added a commit that referenced this issue Nov 9, 2022
)

Merge commits were incorrectly handled due to the named use of `git rev-list`,
which does not give an ancestry path as I expected, but simply a list of
unique commits between the two refs.

Additionally, the check for the base tag was wrong in the face of merge
commits, since a tag could be an ancestor through a merge commit, but not
be the actual base of the branch.

Both of these issues resulted in an incorrect tag being calculated, as well
as a build slowdown due to the iteration over all of the commits from the
other parent of the merge commit, which is unneeded.

While at it, add logic for caching the build tag in order to speed up
builds when the checked-out git tree hasn't changed.
isaac-io added a commit that referenced this issue Nov 12, 2022
)

Merge commits were incorrectly handled due to the named use of `git rev-list`,
which does not give an ancestry path as I expected, but simply a list of
unique commits between the two refs.

Additionally, the check for the base tag was wrong in the face of merge
commits, since a tag could be an ancestor through a merge commit, but not
be the actual base of the branch.

Both of these issues resulted in an incorrect tag being calculated, as well
as a build slowdown due to the iteration over all of the commits from the
other parent of the merge commit, which is unneeded.

While at it, add logic for caching the build tag in order to speed up
builds when the checked-out git tree hasn't changed.
Yuval-Ariel pushed a commit that referenced this issue Nov 15, 2022
)

Merge commits were incorrectly handled due to the named use of `git rev-list`,
which does not give an ancestry path as I expected, but simply a list of
unique commits between the two refs.

Additionally, the check for the base tag was wrong in the face of merge
commits, since a tag could be an ancestor through a merge commit, but not
be the actual base of the branch.

Both of these issues resulted in an incorrect tag being calculated, as well
as a build slowdown due to the iteration over all of the commits from the
other parent of the merge commit, which is unneeded.

While at it, add logic for caching the build tag in order to speed up
builds when the checked-out git tree hasn't changed.
Yuval-Ariel pushed a commit that referenced this issue Nov 25, 2022
)

Merge commits were incorrectly handled due to the named use of `git rev-list`,
which does not give an ancestry path as I expected, but simply a list of
unique commits between the two refs.

Additionally, the check for the base tag was wrong in the face of merge
commits, since a tag could be an ancestor through a merge commit, but not
be the actual base of the branch.

Both of these issues resulted in an incorrect tag being calculated, as well
as a build slowdown due to the iteration over all of the commits from the
other parent of the merge commit, which is unneeded.

While at it, add logic for caching the build tag in order to speed up
builds when the checked-out git tree hasn't changed.
Yuval-Ariel pushed a commit that referenced this issue Nov 25, 2022
)

Merge commits were incorrectly handled due to the named use of `git rev-list`,
which does not give an ancestry path as I expected, but simply a list of
unique commits between the two refs.

Additionally, the check for the base tag was wrong in the face of merge
commits, since a tag could be an ancestor through a merge commit, but not
be the actual base of the branch.

Both of these issues resulted in an incorrect tag being calculated, as well
as a build slowdown due to the iteration over all of the commits from the
other parent of the merge commit, which is unneeded.

While at it, add logic for caching the build tag in order to speed up
builds when the checked-out git tree hasn't changed.
Yuval-Ariel pushed a commit that referenced this issue Nov 25, 2022
)

Merge commits were incorrectly handled due to the named use of `git rev-list`,
which does not give an ancestry path as I expected, but simply a list of
unique commits between the two refs.

Additionally, the check for the base tag was wrong in the face of merge
commits, since a tag could be an ancestor through a merge commit, but not
be the actual base of the branch.

Both of these issues resulted in an incorrect tag being calculated, as well
as a build slowdown due to the iteration over all of the commits from the
other parent of the merge commit, which is unneeded.

While at it, add logic for caching the build tag in order to speed up
builds when the checked-out git tree hasn't changed.
Yuval-Ariel pushed a commit that referenced this issue Apr 30, 2023
)

Merge commits were incorrectly handled due to the named use of `git rev-list`,
which does not give an ancestry path as I expected, but simply a list of
unique commits between the two refs.

Additionally, the check for the base tag was wrong in the face of merge
commits, since a tag could be an ancestor through a merge commit, but not
be the actual base of the branch.

Both of these issues resulted in an incorrect tag being calculated, as well
as a build slowdown due to the iteration over all of the commits from the
other parent of the merge commit, which is unneeded.

While at it, add logic for caching the build tag in order to speed up
builds when the checked-out git tree hasn't changed.
Yuval-Ariel pushed a commit that referenced this issue May 4, 2023
)

Merge commits were incorrectly handled due to the named use of `git rev-list`,
which does not give an ancestry path as I expected, but simply a list of
unique commits between the two refs.

Additionally, the check for the base tag was wrong in the face of merge
commits, since a tag could be an ancestor through a merge commit, but not
be the actual base of the branch.

Both of these issues resulted in an incorrect tag being calculated, as well
as a build slowdown due to the iteration over all of the commits from the
other parent of the merge commit, which is unneeded.

While at it, add logic for caching the build tag in order to speed up
builds when the checked-out git tree hasn't changed.
udi-speedb pushed a commit that referenced this issue Nov 13, 2023
)

Merge commits were incorrectly handled due to the named use of `git rev-list`,
which does not give an ancestry path as I expected, but simply a list of
unique commits between the two refs.

Additionally, the check for the base tag was wrong in the face of merge
commits, since a tag could be an ancestor through a merge commit, but not
be the actual base of the branch.

Both of these issues resulted in an incorrect tag being calculated, as well
as a build slowdown due to the iteration over all of the commits from the
other parent of the merge commit, which is unneeded.

While at it, add logic for caching the build tag in order to speed up
builds when the checked-out git tree hasn't changed.
udi-speedb pushed a commit that referenced this issue Nov 15, 2023
)

Merge commits were incorrectly handled due to the named use of `git rev-list`,
which does not give an ancestry path as I expected, but simply a list of
unique commits between the two refs.

Additionally, the check for the base tag was wrong in the face of merge
commits, since a tag could be an ancestor through a merge commit, but not
be the actual base of the branch.

Both of these issues resulted in an incorrect tag being calculated, as well
as a build slowdown due to the iteration over all of the commits from the
other parent of the merge commit, which is unneeded.

While at it, add logic for caching the build tag in order to speed up
builds when the checked-out git tree hasn't changed.
udi-speedb pushed a commit that referenced this issue Dec 3, 2023
)

Merge commits were incorrectly handled due to the named use of `git rev-list`,
which does not give an ancestry path as I expected, but simply a list of
unique commits between the two refs.

Additionally, the check for the base tag was wrong in the face of merge
commits, since a tag could be an ancestor through a merge commit, but not
be the actual base of the branch.

Both of these issues resulted in an incorrect tag being calculated, as well
as a build slowdown due to the iteration over all of the commits from the
other parent of the merge commit, which is unneeded.

While at it, add logic for caching the build tag in order to speed up
builds when the checked-out git tree hasn't changed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working build Build related
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant