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

a-better-finder-attributes: move to a/a-better-finder-attributes #141893

Closed
wants to merge 1 commit into from

Conversation

cho-m
Copy link
Member

@cho-m cho-m commented Feb 25, 2023

Important: Do not tick a checkbox if you haven’t performed its action. Honesty is indispensable for a smooth review process.

In the following questions <cask> is the token of the cask you're submitting.

After making any changes to a cask, existing or new, verify:

Additionally, if adding a new cask:

  • Named the cask according to the token reference.
  • Checked the cask was not already refused.
  • Checked the cask is submitted to the correct repo.
  • brew audit --new-cask <cask> worked successfully.
  • brew install --cask <cask> worked successfully.
  • brew uninstall --cask <cask> worked successfully.

Experimenting with moving single cask to shard.

I think it will fail style assuming CI runs:

brew style a-better-finder-attributes
a/a-better-finder-attributes.rb:1:1: C: [Correctable] Missing frozen string literal comment.
cask "a-better-finder-attributes" do
^

1 file inspected, 1 offense detected, 1 offense autocorrectable

@github-actions
Copy link

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

@github-actions github-actions bot added the stale Issue which has not received any feedback for some time. label Mar 23, 2023
@bevanjkay bevanjkay added in progress and removed stale Issue which has not received any feedback for some time. labels Mar 23, 2023
@p-linnane
Copy link
Member

@cho-m Do we still need this open?

@cho-m
Copy link
Member Author

cho-m commented Mar 31, 2023

We should figure out how to shard the repo. I think we will want to commit at least one cask to test with before a full migration, whether this one or another option (3rd check item below).

Some things that may need to be done:

  • Agree on sharding approach, e.g. the a/ - z/ directories? leave [0-9].* casks alone or also use 0/ directory? etc.
  • I mentioned on Slack some URLs that won't work for any Cask we migrate. Not sure if we want to handle before or just leave them broken (assuming low impact), e.g.
  • Commit a small number of Cask migrations to test everything is working, e.g. API installs
  • Potential mass migration or migrate in phases. EDIT: May also do history rewrite at same time? Not sure how we were planning to handle this as would probably be impacting to non-API users.
  • Add some checks, e.g. new Casks should be in correct shard.

@reitermarkus
Copy link
Member

reitermarkus commented Apr 14, 2023

I have tried a few sharding approaches, but haven't found a definitive one yet. These two seem like the best ones far.

Approach 1

In order to not have any hotspots with directories that are huge, we'd probably need to have dynamically sized shards, i.e. start with a/, and if a/ grows too large, add a second letter and move all casks into the next level a/{a-z0-9}/.

Approach 2

Use first and last letter. Currently, the biggest directory using this approach (sr/) would contain 63 casks, which I think is acceptable. This also avoids having paths containing - (i.e. a/-/a-better-finder-attributes.rb in approach 1), which look a bit weird.

Copy link
Member

@carlocab carlocab left a comment

Choose a reason for hiding this comment

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

Just testing something. Feel free to ignore.

@razvanazamfirei
Copy link
Member

@reitermarkus, approach 2 makes a lot of sense to me. Last I checked, we would end up with ~500 directories.

I've been playing around with md5 hashing the file name and taking a modulo of the hash to control the number of directories. If we use it to assign cask to directories, the distribution of casks/shard is reasonable. It would be decently easy to implement and audit, but very user-unfriendly.

@MikeMcQuaid
Copy link
Member

Use first and last letter. Currently, the biggest directory using this approach (sr/) would contain 63 casks, which I think is acceptable. This also avoids having paths containing - (i.e. a/-/a-better-finder-attributes.rb in approach 1), which look a bit weird.

First and last letter seems weird to me. Better to do first and second letter IMO and can treat symbols differently e.g. skip them, put them into the directory without, etc.

If we use it to assign cask to directories, the distribution of casks/shard is reasonable. It would be decently easy to implement and audit, but very user-unfriendly.

Agreed. This is too user unfriendly.

Note: even sharding by a-z will be a noted improvement on what we have today. For Homebrew/homebrew-core we're likely to do something like:

>> h={};CoreTap.instance.formula_names.each {|f| key = f[0]; key = "lib" if f.start_with?("lib"); h[key] ||= []; h[key] << f }; nil; h.each_key {|k| puts "#{k}: #{h[k].size}" }; nil
a: 330
b: 244
c: 509
d: 303
e: 148
f: 249
g: 523
h: 158
i: 155
j: 130
k: 126
l: 216
lib: 487
m: 386
n: 201
o: 209
p: 454
q: 59
r: 232
s: 536
t: 311
u: 83
v: 126
w: 129
x: 104
y: 51
z: 75

which gets us reasonable levels of balance (none under 50, none over 550).

@razvanazamfirei
Copy link
Member

/rebase

@BrewTestBot BrewTestBot force-pushed the a/a-better-finder-attributes branch from 5426cc0 to a1fb4a4 Compare June 8, 2023 23:12
@p-linnane
Copy link
Member

Closing this out now that sharding was completed in #152603.

@p-linnane p-linnane closed this Aug 9, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 10, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants