Skip to content

Override initialize in bundle rubygems_ext for NameTuple#7239

Merged
martinemde merged 1 commit intomasterfrom
try-name-tuple-override-differently
Dec 11, 2023
Merged

Override initialize in bundle rubygems_ext for NameTuple#7239
martinemde merged 1 commit intomasterfrom
try-name-tuple-override-differently

Conversation

@martinemde
Copy link
Copy Markdown
Contributor

@martinemde martinemde commented Dec 8, 2023

What was the end-user or developer problem that led to this PR?

In PR #7229 we were discussing alternative ways to add this back-ported feature.

What is your fix for the problem, implemented in this PR?

This avoids overriding .new by aliasing initialize to see if it makes any difference on segfaults. Guards tho feature better against rubygems that already have the feature.

Make sure the following tasks are checked

@martinemde martinemde force-pushed the try-name-tuple-override-differently branch from 6412dfb to f63ce68 Compare December 8, 2023 20:26
@martinemde
Copy link
Copy Markdown
Contributor Author

It's hard to tell if this code is fine or it's just always circumvented. I did run the suite without the guard before pushing and it passed, but I wonder if we should even bother with the guard because the .to_s is such a cheap operation.

@deivid-rodriguez
Copy link
Copy Markdown
Contributor

It's hard to tell if this code is fine or it's just always circumvented.

You mean that our test suite is maybe never instantiating a NameTuple with a Platform parameter? If that's the case, I wonder if we should consider that unsupported and introduce that breaking change?

Regardless of that, I like this patch because it makes it clear how we are monkey patching NameTuple, and when we'll be able to get rid of the monkeypatch.

@martinemde
Copy link
Copy Markdown
Contributor Author

martinemde commented Dec 11, 2023

Sorry, I meant that because there's a guard in place, that unless there's a test suite that runs new bundler against an old version of rubygems, it won't ever patch initialize.

I'm sure we have tests that use a Gem::Platform arg in a NameTuple. (Edit: uh oh, if you go look at that PR we aren't getting any failures other than the rubygems test for the behavior. I was so confident because I added this literally because of the places in bundler tests where this caused failures. Hmm. I'm happy to merge this as a better approach anyway.)

@deivid-rodriguez
Copy link
Copy Markdown
Contributor

Sorry, I meant that because there's a guard in place, that unless there's a test suite that runs new bundler against an old version of rubygems, it won't ever patch initialize.

There is! See .github/workflows/system-rubygems-bundler.yml.

@martinemde martinemde merged commit fb1819b into master Dec 11, 2023
@martinemde martinemde deleted the try-name-tuple-override-differently branch December 11, 2023 15:40
@deivid-rodriguez
Copy link
Copy Markdown
Contributor

Edit: uh oh, if you go look at that PR we aren't getting any failures other than the rubygems test for the behavior. I was so confident because I added this literally because of the places in bundler tests where this caused failures. Hmm. I'm happy to merge this as a better approach anyway.

Yeah! That's why when I opened #7229 completely removing this override, I was surprised that everything passed. Probably worth looking into why.

@martinemde
Copy link
Copy Markdown
Contributor Author

This did not prevent segfaults. I linked a recent one in the ruby bug tracker should have included this commit.

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