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

Fix regression when caching gems from secondary sources #7659

Conversation

deivid-rodriguez
Copy link
Member

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

If cache_all_platforms setting is enabled, the secondary source was no longer considering cached gems.

That means that if the remote secondary source has removed its gems, then this was now resulting in an error while before the previously cached gem from the source would still be used.

This is a regression from #7516.

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

This commit restores previous behavior by explicit enabling cached gems when materializing gems for all platforms so that they can be cached in vendor/cache.

Make sure the following tasks are checked

If `cache_all_platforms` setting is enabled, the secondary source was
no longer considering cached gems.

That means that if the remote secondary source has removed its gems,
then this was now resulting in an error while before the previously
cached gem from the source would still be used.

This commit restores previous behavior.
@honeyankit
Copy link

honeyankit commented May 15, 2024

@deivid-rodriguez : Thank you for putting this. Is it possible to ship this tomorrow?

@deivid-rodriguez deivid-rodriguez merged commit 3fb1c37 into master May 16, 2024
83 checks passed
@deivid-rodriguez deivid-rodriguez deleted the deivid-rodriguez/fix-materialized-for-all-platforms-regression branch May 16, 2024 09:21
@deivid-rodriguez
Copy link
Member Author

No problem! Yes, my idea is to ship a new release this week.

That said, I think upgrading Dependabot may be a bit of work. In particular, while looking into this issue, I noticed Dependabot relies on the @unlock[:gems] value in Bundler::Definition, which is now removed at initialize time since #7558.

@sachin-sandhu
Copy link

@deivid-rodriguez , do you have a release

No problem! Yes, my idea is to ship a new release this week.

That said, I think upgrading Dependabot may be a bit of work. In particular, while looking into this issue, I noticed Dependabot relies on the @unlock[:gems] value in Bundler::Definition, which is now removed at initialize time since #7558.

Hi @deivid-rodriguez ,
Is the release complete, if not, can you please share the # for us to keep track.

Thanks

@deivid-rodriguez
Copy link
Member Author

Hei @sachin-sandhu! Sorry, I delayed last week release a bit because I saw some strange hang in CI and I was afraid it could be due to some recent change. But I haven't seen it anymore, either locally or in CI, so I'll move the release forward as planned. I'll try to wrap it up tomorrow.

@deivid-rodriguez
Copy link
Member Author

deivid-rodriguez commented May 23, 2024

Sorry once again for the delay here 🙏. I saw that hang again yesterday and I'm looking into it. Since this fix is a one liner, I would advice to monkeypatch the Bundler::SpecSet#materialize_for_all_platforms to get the fix for now. That way you don't need to block on the next Bundler release, nor on upgrading dependabot to use it, and can benefit from it inmediately.

@sachin-sandhu
Copy link

Sorry once again for the delay here 🙏. I saw that hang again yesterday and I'm looking into it. Since this fix is a one liner, I would advice to monkeypatch the Bundler::SpecSet#materialize_for_all_platforms to get the fix for now. That way you don't need to block on the next Bundler release, nor on upgrading dependabot to use it, and can benefit from it inmediately.

Thanks @deivid-rodriguez for the inputs, we are working on the monkey patch fix 👍🏻

@deivid-rodriguez
Copy link
Member Author

No problem, sounds good, let me know if you find any issues!

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

Successfully merging this pull request may close these issues.

None yet

3 participants