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

Update from 2.0.2 to 2.1.7 and reapply patches #17

Merged
merged 4 commits into from
Jun 24, 2024
Merged

Conversation

al13n321
Copy link
Member

@al13n321 al13n321 commented Jun 20, 2024

This is upstream tag 2.1.7 (8d8768a "2.1.7 Release") + the ~3 of our patches that are not obsolete.

@CLAassistant
Copy link

CLAassistant commented Jun 20, 2024

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
2 out of 3 committers have signed the CLA.

✅ al13n321
✅ SuzyWangIBMer
❌ bnaecker
You have signed the CLA already but the status is still pending? Let us recheck it.

@al13n321
Copy link
Member Author

(Idk how to do this properly, ended up just doing:

git co 8d8768ab # tag 2.1.7 from upstream
git reset origin/clickhouse-new
git ci -m "DISCARD ALL PREVIOUS COMMITS and 'git reset' to upstream 2.1.7 (8d8768ab)"
[cherry-pick fixes]
git push

This seems lame because the commit graph doesn't reflect the fact that this originates from upstream 8d8768a.

Maybe the intended way to do this is to create a new branch with each update instead of merging into clickhouse-new? But then there can't be a PR. PR always corresponds to a merge, and merge is not what we need to do here.)

@al13n321 al13n321 marked this pull request as ready for review June 22, 2024 02:45
@al13n321 al13n321 requested a review from rschu1ze June 22, 2024 02:45
@al13n321 al13n321 merged commit ace9555 into clickhouse-new Jun 24, 2024
275 of 276 checks passed
@rschu1ze
Copy link
Member

rschu1ze commented Aug 6, 2024

@al13n321
Copy link
Member Author

al13n321 commented Aug 7, 2024

https://clickhouse.com/docs/en/development/contrib#adding-and-maintaining-third-party-libraries explains a better way to do this.

Do you mean this part?:

(more work but cleaner): create a new branch with clickhouse/ prefix from the upstream commit or tag you like to integrate. Then re-apply all existing patches using new PRs (or squash them into a single PR). This method can be used when the clickhouse/ branch tracks a specific upstream version branch or tag. It is cleaner in the sense that custom patches and upstream changes are better isolated from each other.

Does it mean the same as what this PR did, but instead of merging into clickhouse-new merge into a new branch? Or does it mean: (1) create and push a new branch with an orphan empty commit, (2) create a second new branch branching off from that empty commit, (3) put the upstream and the patches into the second branch, (4) create a PR to merge the second branch into the first?

@rschu1ze
Copy link
Member

rschu1ze commented Aug 8, 2024

Here is how I would do it:

Upstream zlib labels certain commits with a tag: https://github.com/zlib-ng/zlib-ng/tags ... these are the versions that we want to use as a base. Let's use 2.1.7 as an example.

The first step would be to create a branch ClickHouse/2.1.7 in the forked repository:

  • First, make sure to have the upstream and forked repo available as remotes in your local Git, then fetch from both
  • Then create a branch git switch --create ClickHouse/2.1.7 2.1.7

Next, we need to pick old patches into ClickHouse/2.1.7. I'd simply use git cherry-pick for that, and to keep things simple possibly squash commits if they were introduced by an PR that had multiple commits.

Then push ClickHouse/2.1.7 to the forked repository (and reference it by the main ClickHouse repository).

The docs say:

then re-apply all existing patches using new PRs (or squash them into a single PR).

That is admittedly quite complicated and has little benefit. I'll update the docs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants