feat: allow font install on linux #18874

SMillerDev · 2024-12-04T21:57:53Z

Have you followed the guidelines in our Contributing document?
Have you checked to ensure there aren't other open Pull Requests for the same change?
Have you added an explanation of what your changes do and why you'd like us to include them?
Have you written new tests for your changes? Here's an example.
Have you successfully run brew style with your changes locally?
Have you successfully run brew typecheck with your changes locally?
Have you successfully run brew tests with your changes locally?

This should disable quarantine on Linux, allow installing fonts and set a location for the fonts. Marking this as draft until I've had a moment to test it.

Waiting for #18808 to be merged

SMillerDev · 2024-12-14T14:31:18Z

Installing fonts now works, although it is still installing in the macOS location. I can't get the DEFAULTS to work.

SMillerDev · 2025-01-07T14:30:45Z

Library/Homebrew/extend/os/linux/cask/config.rb

+        }.freeze, T::Hash[Symbol, T.nilable(String)])
+
+        sig { returns(T::Hash[Symbol, T.untyped]) }
+        def self.defaults


@Homebrew/brew I can't get the code to pick up this extended version, no matter what I try. Does anyone have a suggestion how to do that?

@dduugg may know!

Ugh, guilty 😬

prepending (like include) only affects instance methods. If I understand what you're trying to do here, you'll want to write this as an instance method in a submodule (e.g. ClassMethods) which is then prepended to the singleton class. See Keg for an example of this dual-prepend approach, specifically:

brew/Library/Homebrew/extend/os/mac/keg.rb

Lines 130 to 131 in e23d079

Keg.singleton_class.prepend(OS::Mac::Keg::ClassMethods)

Keg.prepend(OS::Mac::Keg)

(I'm entirely to blame for this convoluted mess, and I'm open to alternatives. It's probably useful to document this for others, maybe in Typechecking.md, though it has discoverability issues.)

That doesn't look all that bad, I wouldn't have guessed that this was type check related though, so I probably wouldn't have found it in the documentation.

I've done this now, but it somehow doesn't get invoked when I run brew install --cask font-<something>. And I have no idea why.

To (hopefully) ask the obvious, you're running that command on Linux, correct?

Yeah, was running it in a codespace.

I think this will fix you up (I know it's confusing…): https://github.com/Homebrew/brew/pull/18874/files#r1909348690

Library/Homebrew/extend/os/linux/cask/quarantine.rb

Library/Homebrew/extend/os/linux/cask/installer.rb

dduugg · 2025-01-09T19:42:33Z

Library/Homebrew/extend/os/linux/cask/config.rb

+          }.freeze, T::Hash[Symbol, T.nilable(String)])
+
+          sig { returns(T::Hash[Symbol, T.untyped]) }
+          def self.defaults


Needs to be an instance method for prepend to work:

Suggested change

def self.defaults

def defaults

(It will become a class method once prepended to the singleton class 🤷 🤕 )

dduugg · 2025-01-09T19:42:59Z

Library/Homebrew/extend/os/linux/cask/config.rb

+end
+
+Cask::Config.singleton_class.prepend(OS::Linux::Cask::Config::ClassMethods)
+Cask::Config.prepend(OS::Linux::Cask::Config)


I don't think this is necessary

Suggested change

Cask::Config.prepend(OS::Linux::Cask::Config)

dduugg · 2025-01-09T19:43:49Z

Library/Homebrew/extend/os/linux/cask/config.rb

+        extend T::Helpers
+
+        requires_ancestor { ::Cask::Config }
+


This shouldn't be necessary:

Suggested change

extend T::Helpers

requires_ancestor { ::Cask::Config }

dduugg · 2025-01-09T19:44:48Z

Library/Homebrew/extend/os/linux/cask/config.rb

+          }.freeze, T::Hash[Symbol, T.nilable(String)])
+
+          sig { returns(T::Hash[Symbol, T.untyped]) }


We should be able to tighten up the types:

Suggested change

}.freeze, T::Hash[Symbol, T.nilable(String)])

sig { returns(T::Hash[Symbol, T.untyped]) }

}.freeze, T::Hash[Symbol, String])

sig { returns(T::Hash[Symbol, String]) }

dduugg · 2025-01-09T19:46:17Z

Library/Homebrew/extend/os/cask/artifact/moved.rb

+# frozen_string_literal: true
+
+require "extend/os/mac/cask/artifact/moved" if OS.mac?
+require "extend/os/linux/cask/artifact/moved" if OS.linux?


Not something to take on in this PR, but I wonder if we should have a single require file for all of an OS's extensions, rather than introduce three more require files here. 🤔

dduugg · 2025-01-09T19:46:55Z

Library/Homebrew/extend/os/linux/cask/quarantine.rb

+        def self.check_quarantine_support
+          :linux
+        end


Suggested change

def self.check_quarantine_support

:linux

end

def self.check_quarantine_support = :linux

SMillerDev · 2025-01-09T22:13:53Z

Thanks for the pointers @dduugg !

SMillerDev force-pushed the feat/cask/font_install branch 3 times, most recently from 801829e to e5c2c8c Compare December 5, 2024 08:11

SMillerDev changed the title ~~feat: only block cask install on Linux~~ feat: allow font install on linux Dec 9, 2024

SMillerDev force-pushed the feat/cask/font_install branch from e5c2c8c to d1b5f68 Compare December 14, 2024 14:30

SMillerDev force-pushed the feat/cask/font_install branch 3 times, most recently from 99e0454 to ed30ba7 Compare December 14, 2024 15:14

SMillerDev force-pushed the feat/cask/font_install branch from ed30ba7 to 378ff7c Compare December 21, 2024 16:45

SMillerDev commented Jan 7, 2025

View reviewed changes

dduugg reviewed Jan 7, 2025

View reviewed changes

Library/Homebrew/extend/os/linux/cask/quarantine.rb Outdated Show resolved Hide resolved

dduugg reviewed Jan 7, 2025

View reviewed changes

Library/Homebrew/extend/os/linux/cask/installer.rb Outdated Show resolved Hide resolved

SMillerDev force-pushed the feat/cask/font_install branch 2 times, most recently from 198387f to 6e8fc52 Compare January 7, 2025 20:16

feat: allow font install on linux

75ac2b5

SMillerDev force-pushed the feat/cask/font_install branch from 6e8fc52 to 75ac2b5 Compare January 7, 2025 20:36

dduugg reviewed Jan 9, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: allow font install on linux #18874

feat: allow font install on linux #18874

SMillerDev commented Dec 4, 2024 •

edited

Loading

SMillerDev commented Dec 14, 2024

SMillerDev Jan 7, 2025

MikeMcQuaid Jan 7, 2025

dduugg Jan 7, 2025

SMillerDev Jan 7, 2025

SMillerDev Jan 7, 2025

dduugg Jan 8, 2025

SMillerDev Jan 8, 2025

dduugg Jan 9, 2025

dduugg Jan 9, 2025

dduugg Jan 9, 2025

dduugg Jan 9, 2025

dduugg Jan 9, 2025

dduugg Jan 9, 2025 •

edited

Loading

dduugg Jan 9, 2025

SMillerDev commented Jan 9, 2025

	Keg.singleton_class.prepend(OS::Mac::Keg::ClassMethods)
	Keg.prepend(OS::Mac::Keg)

		}.freeze, T::Hash[Symbol, T.nilable(String)])

		sig { returns(T::Hash[Symbol, T.untyped]) }

feat: allow font install on linux #18874

Are you sure you want to change the base?

feat: allow font install on linux #18874

Conversation

SMillerDev commented Dec 4, 2024 • edited Loading

SMillerDev commented Dec 14, 2024

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

dduugg Jan 9, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SMillerDev commented Jan 9, 2025

SMillerDev commented Dec 4, 2024 •

edited

Loading

dduugg Jan 9, 2025 •

edited

Loading