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.

Sign up for GitHub

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

Jump to bottom

Remove cityhash from Gemfile #2155

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Remove cityhash from Gemfile #2155

wants to merge 1 commit into from

Conversation

andyw8
Copy link
Contributor

@andyw8 andyw8 commented Jan 17, 2025 •
edited
Loading

This was added b2caf74 but doesn't seem to be used for anything.

@andyw8 andyw8 force-pushed the andyw8/remove-cityhash branch from d31b776 to 78a6339 Compare January 17, 2025 21:51
@paracycle
Copy link
Member

paracycle commented Jan 20, 2025 •
edited
Loading

I don't understand how it is not used. It is used for this: https://github.com/Shopify/identity_cache/blob/8d70a6f2f0f2a61020635cd20317c6fb9dd3c8e5/lib/identity_cache/cache_hash.rb#L5

Also explained in the README installation instructions: https://github.com/Shopify/identity_cache/blob/8d70a6f2f0f2a61020635cd20317c6fb9dd3c8e5/README.md#installation

@andyw8
Copy link
Contributor Author

andyw8 commented Jan 20, 2025

@paracycle that's helpful, but if it's optional for performance, why does Tapioca need it?

@andyw8 andyw8 added the chore label Jan 20, 2025
@andyw8
Copy link
Contributor Author

andyw8 commented Jan 20, 2025

(CI failures are due to #2158)

@andyw8
Copy link
Contributor Author

andyw8 commented Jan 21, 2025

(Rails 7.0 failures are being addressed in #2161)

@andyw8 andyw8 marked this pull request as ready for review January 21, 2025 18:22
@andyw8 andyw8 requested a review from a team as a code owner January 21, 2025 18:22
@andyw8 andyw8 requested a review from KaanOzkan January 21, 2025 18:23
@andyw8 andyw8 force-pushed the andyw8/remove-cityhash branch from 78a6339 to 4482392 Compare January 21, 2025 18:45
paracycle
paracycle reviewed Jan 21, 2025
View reviewed changes
Gemfile.lock
Comment on lines +379 to +388
aarch64-linux
aarch64-linux-gnu
aarch64-linux-musl
arm64-darwin
universal-darwin
x86_64-darwin
x86_64-linux
x86_64-linux-gnu
x86_64-linux-musl
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't all of this just be?

PLATFORMS
  aarch64-linux
  universal-darwin
  x86_64-linux

Copy link
Contributor Author

@andyw8 andyw8 Jan 21, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bundler 2.6 output a message advising running --normalize-platforms:

https://bundler.io/blog/2024/12/19/bundler-v2-6.html

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, I get that, but universal-darwin already includes arm64-darwin and x86_64-darwin, and similarly x86_64-linux is a superset of x86_64-linux-{gnu,musl}. So we should not need the finer grained ones.

@andyw8 andyw8 force-pushed the andyw8/remove-cityhash branch from 4482392 to 2d10608 Compare January 21, 2025 19:32
@andyw8 andyw8 force-pushed the andyw8/remove-cityhash branch from 2d10608 to e344086 Compare January 21, 2025 19:33
@andyw8 andyw8 enabled auto-merge January 21, 2025 19:54
@KaanOzkan
Copy link
Contributor

@paracycle that's helpful, but if it's optional for performance, why does Tapioca need it?

Does this mean identity_cache tests will be slower now?

@andyw8
Copy link
Contributor Author

andyw8 commented Jan 21, 2025

Running shows no difference.

@andyw8 andyw8 removed the request for review from KaanOzkan January 21, 2025 21:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@paracycle paracycle paracycle approved these changes

Assignees
No one assigned
Labels
chore
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@andyw8 @paracycle @KaanOzkan