Skip to content

Conversation

@k0kubun
Copy link
Member

@k0kubun k0kubun commented Oct 6, 2025

@k0kubun k0kubun merged commit 4aeef6f into ruby:master Oct 6, 2025
44 checks passed
@k0kubun k0kubun deleted the sync-ruby branch October 6, 2025 23:50
@k0kubun
Copy link
Member Author

k0kubun commented Oct 27, 2025

@rhenium I see that today's merge commit b7ee7e0 failed the sync due to 632d6e6 whose conflict seems to be resolved by the merge commit. I want to discuss how to avoid failing the sync workflow going forward.

In ruby/ruby, because a couple of committers did not like merge commits (see also: mame's git hook), we decided not to allow merge commits. Because of that, tool/sync_default_gems.rb cherry-picks commits one by one, also modifying the content to adapt for ruby/ruby.

By nature, if you push a merge commit that resolves a conflict in it, the commit with the conflict (e.g. 632d6e6, which conflicts with 64f4aae) in the merged tree will always result in a conflict when cherry-picking it.

To address the situation, I've thought of the following options:

  1. Stop pushing merge commits to ruby/openssl
    • When commits like 632d6e6 are pushed to master without a merge commit, their conflict will need to be fixed in the commit that introduced the conflict, so the sync will not fail this way.
    • Also consider disabling "Allow merge commits" and maybe enabling "Allow rebase merging" for PRs
  2. Keep pushing merge commits to ruby/openssl, but manually fix conflicts outside merge commits
    • If you modify the commits in the merged branch beforehand, you can make a merge commit without resolving any conflict in the merge commit. Such a merge can be synced successfully despite the use of a merge commit.
  3. Migrate openssl from a default gem to a bundled gem
    • Bundled gems don't need to be synced to ruby/ruby.
  4. Keep failing the sync workflow and manually sync outdated files whenever it fails.
  5. Allow pushing merge commits to ruby/ruby and modify tool/sync_default_gems.rb to use that when needed.

WDYT?

@rhenium
Copy link
Member

rhenium commented Oct 27, 2025

Honestly, I didn't really see this as a major problem. It happens rather infrequently, only when branches are merged upwards, so the manual work required on my side has been minimal. The new sync workflow also generates a notification email now, which is convenient.

Since the main motivation for copying default gems into ruby/ruby was to let committers quickly apply patches without waiting for gem maintainers, there can be local changes in ruby/ruby at times. I think occasional sync failures are somewhat expected, even without tricky merge commits.

  1. Migrate openssl from a default gem to a bundled gem

    • Bundled gems don't need to be synced to ruby/ruby.

I believe this is still a goal for every default gem, which has been gradually progressing over the past ~10 years.

AFAIK the remaining blockers for openssl are RubyGems and net/http (which is also blocked by RubyGems).

@k0kubun
Copy link
Member Author

k0kubun commented Oct 27, 2025

The new sync workflow also generates a notification email now, which is convenient.

Yeah, that's part of the features this was intended to deliver, so I'm glad you like it.

Since the main motivation for copying default gems into ruby/ruby was to let committers quickly apply patches without waiting for gem maintainers, there can be local changes in ruby/ruby at times.

That's right. I expect this to happen at times, and I want the maintainer to notice the discrepancy by the workflow failure.

But it's not a reason to fail sync when there's no ruby/ruby-local changes. Both 64f4aae and 632d6e6 came from ruby/openssl, so ruby/ruby-local changes are not the cause of the failure we're discussing. I'm saying that, when all changes are made in ruby/openssl and there's no ruby/ruby-local patch, their sync should never fail.

the manual work required on my side has been minimal.

The current workflow is that you merge a branch, resolve a conflict in the merge commit, and manually fix failed syncs. If you resolve a conflict in the commit that introduced the conflict by doing (1) or (2), the sync should automatically succeed. So you can remove the "manually fix failed syncs" step from the "minimal" workflow if you do so.

  1. Migrate openssl from a default gem to a bundled gem

I believe this is still a goal for every default gem
AFAIK the remaining blockers for openssl are RubyGems and net/http (which is also blocked by RubyGems).

Yeah, so it's not a short-term solution for ruby/openssl. WDYT about doing (1) until we make it happen? If you want to explicitly leave "Merge branch" commits in git history, how about (2)?

@rhenium
Copy link
Member

rhenium commented Oct 28, 2025

I feel this is best handled by tool/sync_default_gems.rb. Similar to option (5), but I started to wonder if it could cherry-pick a merge commit as a whole instead of flattening it and replaying each individual commit, since such detail isn't important for ruby/ruby. I'll see if I can make some enhancement there.

I'm a big fan of using merge commits to group related changes logically or to avoid making duplicate commits. Option (2) likely causes more issues than it resolves. In particular, git bisect can become harder to use if not every commit in the repositories compiles.

@k0kubun
Copy link
Member Author

k0kubun commented Oct 28, 2025

OK, I respect your preference of merge commits. I leave it for you to decide how to handle them.

FYI, I made a little improvement on the sync workflow ruby/ruby@d82a590, which shows a diff like this on conflicts. I hope it helps you fix sync failures.

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.

2 participants