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

timvisher · 2022-04-28T03:36:08Z

Still cleans info/dir and info/<arch>/dir files.

MikeMcQuaid

Looks good so far!

MikeMcQuaid · 2022-04-28T08:07:19Z

Library/Homebrew/cleaner.rb

@@ -31,7 +31,11 @@ def clean
    # Get rid of any info 'dir' files, so they don't conflict at the link stage
    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 if info_dir_file == Pathname("#{@f.info}/#{@f.name}/dir")


Can this get a new comment? It's not clear why this is fine to not remove.

Suggested change

next if info_dir_file == Pathname("#{@f.info}/#{@f.name}/dir")

next if info_dir_file == @f.info/@f.name/"dir"

I think we can also simplify it to this.

Yes but: I still don't understand the "why" from this code 😅

Oh, I agree with adding a comment explaining why. I'm just allergic to unnecessary interpolation, since they're harder to read and makes code less compact.

carlocab · 2022-04-28T08:43:00Z

Library/Homebrew/test/cleaner_spec.rb

+    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"


Suggested change

name_file = f.info/"#{f.name}/dir"

name_file = f.info/f.name/"dir"

I guess it shows that I'm neither a Ruby hacker nor a Homebrew dev. xD

I'll try these suggestions out. :)

carlocab · 2022-04-28T08:43:12Z

Library/Homebrew/test/cleaner_spec.rb

+
+      f.info.mkpath
+      (f.info/"i686-elf").mkpath
+      (f.info/ + f.name.to_s).mkpath


Suggested change

(f.info/ + f.name.to_s).mkpath

(f.info/f.name).mkpath

carlocab · 2022-04-28T08:44:19Z

Library/Homebrew/test/cleaner_spec.rb

+      (f.info/"i686-elf").mkpath
+      (f.info/ + f.name.to_s).mkpath


Or, maybe more simply:

Suggested change

(f.info/"i686-elf").mkpath

(f.info/ + f.name.to_s).mkpath

arch_file.dirname.mkpath

name_file.dirname.mkpath

timvisher · 2022-04-28T14:23:11Z

Library/Homebrew/cleaner.rb

+    #
+    # [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


@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?

No, this is absolutely wonderful, nice work.

timvisher · 2022-04-28T14:24:03Z

Library/Homebrew/test/cleaner_spec.rb

@@ -96,7 +96,7 @@
    it "removes '.la' files" do
      file = f.lib/"foo.la"

-      f.lib.mkpath
+      file.dirname.mkpath


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

Still cleans `info/dir` and `info/<arch>/dir` files. Fixes Homebrew/homebrew-core#100190

MikeMcQuaid · 2022-04-29T13:16:45Z

Library/Homebrew/cleaner.rb

+    #
+    # [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


No, this is absolutely wonderful, nice work.

MikeMcQuaid · 2022-04-29T13:17:04Z

Thanks so much for your incredible first contribution! Without people like you submitting PRs we couldn't run this project. You rock, @timvisher!

timvisher · 2022-04-29T13:47:41Z

@MikeMcQuaid @carlocab Thanks for the feedback and the merge! FWIW I was never able to actually test that this actually fixes the upstream issue with including the share/info/emacs/dir file in the bottle. I'm not sure if there's a good way for me to check that or not but I can always just look at the bottles once they get regenerated, I suppose?

carlocab · 2022-04-29T13:52:04Z

You can try doing brew reinstall -s emacs with these changes applied. brew update will do that if you are on the developer version of brew (check with brew developer).

timvisher · 2022-04-29T14:37:25Z

@carlocab Looks good on my end then! :D

What's the release process for bottles? If I'm reading things correctly it looks like they only get generated on a new changeset to the formula. I'm not finding any examples of this (where there's literally no change to the Formula other than that it should be rebuilt) but maybe the best way to approach that would be a change to the Emacs formula that adds a revision to it?

Edit: Maybe Homebrew/homebrew-core#100400?

$ if brew developer | grep -q 'Developer mode is enabled'; then git -C "$(brew --repo)" log -1 --oneline --author='Tim Visher'; brew reinstall -s emacs; emacs --batch --eval '
  (progn
    (info)
    (message "%S" Info-directory-list)
    (occur "Htmlfontify")
    (with-current-buffer "*Occur*"
     (message "%s" (buffer-substring (point-min)
                                       (point-max)))))'; fi
0d8186459 (public/fix/keep-info_name_dir-files-on-clean, fix/keep-info_name_dir-files-on-clean) Use file.dirname in most cleaner tests
Warning: Treating emacs as a formula. For the cask, use homebrew/cask/emacs
==> Downloading https://ftp.gnu.org/gnu/emacs/emacs-28.1.tar.xz
Already downloaded: /Users/timvisher/Library/Caches/Homebrew/downloads/81fae34a5dbd8042af6a70512829e9d4a11e31e7067bf4da5c5bded31f757129--emacs-28.1.tar.xz
==> Reinstalling emacs
==> ./configure --enable-locallisppath=/usr/local/share/emacs/site-lisp --infodir=/usr/local/Cellar/emacs/28.1/share/info/emacs --prefix=/usr/local/Cellar/emacs/28.1 --with-gnutls --wit
==> make
==> make install
==> Caveats
To restart emacs after an upgrade:
  brew services restart emacs
Or, if you don't want/need a background service you can just run:
  /usr/local/opt/emacs/bin/emacs --fg-daemon
==> Summary
🍺  /usr/local/Cellar/emacs/28.1: 4,095 files, 110.3MB, built in 2 minutes 49 seconds
==> Running `brew cleanup emacs`...
Disable this behaviour by setting HOMEBREW_NO_INSTALL_CLEANUP.
Hide these hints with HOMEBREW_NO_ENV_HINTS (see `man brew`).
Composing main Info directory...
Composing main Info directory...done
("/usr/local/Cellar/emacs/28.1/share/info/emacs/" "/usr/local/share/info/" "/usr/share/info/")
Searched 1 buffer; 1 match for "Htmlfontify"
1 match for "Htmlfontify" in buffer: *info*
     70:* Htmlfontify: (htmlfontify).   Convert source code to html.

MikeMcQuaid reviewed Apr 28, 2022

View reviewed changes

carlocab reviewed Apr 28, 2022

View reviewed changes

timvisher commented Apr 28, 2022

View reviewed changes

Tim Visher added 5 commits April 28, 2022 10:40

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

fbb3ccb

Still cleans `info/dir` and `info/<arch>/dir` files. Fixes Homebrew/homebrew-core#100190

lint

48db5ef

Use more next

59649a2

feedback

8852208

Use file.dirname in most cleaner tests

0d81864

timvisher force-pushed the fix/keep-info_name_dir-files-on-clean branch from ca7954c to 0d81864 Compare April 28, 2022 14:40

timvisher marked this pull request as ready for review April 28, 2022 14:50

timvisher requested review from carlocab and MikeMcQuaid April 29, 2022 02:37

carlocab approved these changes Apr 29, 2022

View reviewed changes

MikeMcQuaid approved these changes Apr 29, 2022

View reviewed changes

MikeMcQuaid merged commit 2c0bd31 into Homebrew:master Apr 29, 2022

timvisher mentioned this pull request Apr 29, 2022

Force rebottling of emacs 28.1 Homebrew/homebrew-core#100400

Closed

github-actions bot added the outdated PR was locked due to age label May 30, 2022

github-actions bot locked as resolved and limited conversation to collaborators May 30, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

timvisher commented Apr 28, 2022

MikeMcQuaid left a comment

MikeMcQuaid Apr 28, 2022

carlocab Apr 28, 2022

MikeMcQuaid Apr 28, 2022

carlocab Apr 28, 2022

carlocab Apr 28, 2022

timvisher Apr 28, 2022

carlocab Apr 28, 2022

carlocab Apr 28, 2022

timvisher Apr 28, 2022

MikeMcQuaid Apr 29, 2022

timvisher Apr 28, 2022

MikeMcQuaid Apr 29, 2022

MikeMcQuaid commented Apr 29, 2022

timvisher commented Apr 29, 2022

carlocab commented Apr 29, 2022

timvisher commented Apr 29, 2022 •

edited

Loading

	next if info_dir_file == Pathname("#{@f.info}/#{@f.name}/dir")
	next if info_dir_file == @f.info/@f.name/"dir"

	name_file = f.info/"#{f.name}/dir"
	name_file = f.info/f.name/"dir"

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

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

Conversation

timvisher commented Apr 28, 2022

MikeMcQuaid left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MikeMcQuaid commented Apr 29, 2022

timvisher commented Apr 29, 2022

carlocab commented Apr 29, 2022

timvisher commented Apr 29, 2022 • edited Loading

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

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

timvisher commented Apr 29, 2022 •

edited

Loading