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

Keep info/#{f.name}/dir files in cleaner #13215

Merged
merged 5 commits into from
Apr 29, 2022
Merged

Keep info/#{f.name}/dir files in cleaner #13215

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
fbb3ccb
Keep `info/#{f.name}/dir` files in cleaner
Apr 28, 2022
48db5ef
lint
Apr 28, 2022
59649a2
Use more `next`
Apr 28, 2022
8852208
feedback
Apr 28, 2022
0d81864
Use file.dirname in most cleaner tests
Apr 28, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 23 additions & 1 deletion Library/Homebrew/cleaner.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,9 +29,31 @@ def clean
[@f.bin, @f.sbin, @f.lib].each { |d| clean_dir(d) if d.exist? }

# Get rid of any info 'dir' files, so they don't conflict at the link stage
#
# The 'dir' files come in at least 3 locations:
#
# 1. 'info/dir'
# 2. 'info/#{name}/dir'
# 3. 'info/#{arch}/dir'
#
# Of these 3 only 'info/#{name}/dir' is safe to keep since the rest will
# conflict with other formulae because they use a shared location.
#
# See [cleaner: recursively delete info `dir`s by gromgit · Pull Request
# #11597][1], [emacs 28.1 bottle does not contain `dir` file · Issue
# #100190][2], and [Keep `info/#{f.name}/dir` files in cleaner by
# timvisher][3] for more info.
#
# [1]: https://github.com/Homebrew/brew/pull/11597
# [2]: https://github.com/Homebrew/homebrew-core/issues/100190
# [3]: https://github.com/Homebrew/brew/pull/13215
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@MikeMcQuaid What do you think? It doesn't seems like we try to stay overly concise in most of the comments in this file but maybe this is a bit too flowery for what you were going for?

Copy link
Member

Choose a reason for hiding this comment

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

No, this is absolutely wonderful, nice work.

Dir.glob(@f.info/"**/dir").each do |f|
info_dir_file = Pathname(f)
observe_file_removal info_dir_file if info_dir_file.file? && [email protected]_clean?(info_dir_file)
next unless info_dir_file.file?
next if info_dir_file == @f.info/@f.name/"dir"
next if @f.skip_clean?(info_dir_file)

observe_file_removal info_dir_file
end

rewrite_shebangs
Expand Down
28 changes: 24 additions & 4 deletions Library/Homebrew/test/cleaner_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,7 @@
it "removes '.la' files" do
file = f.lib/"foo.la"

f.lib.mkpath
file.dirname.mkpath
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@carlocab I liked your suggestion so much I ported it over to the other tests that I cargo culted the original test from. :)

touch file

cleaner.clean
Expand All @@ -107,7 +107,7 @@
it "removes 'perllocal' files" do
file = f.lib/"perl5/darwin-thread-multi-2level/perllocal.pod"

(f.lib/"perl5/darwin-thread-multi-2level").mkpath
file.dirname.mkpath
touch file

cleaner.clean
Expand All @@ -118,7 +118,7 @@
it "removes '.packlist' files" do
file = f.lib/"perl5/darwin-thread-multi-2level/auto/test/.packlist"

(f.lib/"perl5/darwin-thread-multi-2level/auto/test").mkpath
file.dirname.mkpath
touch file

cleaner.clean
Expand All @@ -129,12 +129,32 @@
it "removes 'charset.alias' files" do
file = f.lib/"charset.alias"

f.lib.mkpath
file.dirname.mkpath
touch file

cleaner.clean

expect(file).not_to exist
end

it "removes 'info/**/dir' files except for 'info/<name>/dir'" do
file = f.info/"dir"
arch_file = f.info/"i686-elf/dir"
name_file = f.info/f.name/"dir"

file.dirname.mkpath
arch_file.dirname.mkpath
name_file.dirname.mkpath

touch file
touch arch_file
touch name_file

cleaner.clean

expect(file).not_to exist
expect(arch_file).not_to exist
expect(name_file).to exist
end
end

Expand Down