Add Nice option to service #14936

jorpilo · 2023-03-09T16:08:00Z

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 locall

Added service.nice to allow to define default nice levels in plist and systemd units.

MikeMcQuaid

Thanks for the PR. Looking good so far!

MikeMcQuaid · 2023-03-09T16:24:27Z

Library/Homebrew/service.rb

      @run&.map(&:to_s)
    end

+    sig { params(value: T.nilable(Integer)).returns(T.nilable(Integer)) }


Why/when would you want to pass in a value: nil?

Suggested change

sig { params(value: T.nilable(Integer)).returns(T.nilable(Integer)) }

sig { params(value: Integer).returns(T.nilable(Integer)) }

may make more sense if not

Correct, we can remove nil but I was following the format of the rest of parameters like restart_delay which is also defined nilable

brew/Library/Homebrew/service.rb

Line 211 in 0d70ca7

sig { params(value: T.nilable(Integer)).returns(T.nilable(Integer)) }

CC @SMillerDev for thoughts here, I think it'd probably make sense to make none of these nilable.

Most of these methods simply return the current value when nil (i.e. no argument) is passed. We don't seem to be calling any of these without an argument though.

I will remove the nil to also resolve the typecheck issue but we might want to also change the rest of parameters. Please verify that the change is correct.

Also if we remove the nilable, the TypeError is unrechable as it expects an integer. Should I remove the it?

@dduugg @reitermarkus Also would welcome your thoughts on how, in a future PR, we could refactor this type handling to lean more on Sorbet (bearing in mind that e.g. formulae will almost never be evaluated at runtime to use Sorbet and are currently never type checked by Sorbet)

I don't think i have any especially creative suggestions. You can try to bump these files to typed: strict to catch more edge cases. I believe we do runtime typechecking in tests, so covering every service DSL in tests could do the trick. A rubocop that flags unacceptablenice values could help too (it's tricky if you're allowing vars to be passed to nice, but it could enforce that nice is followed by a literal Integer in the required range).

Thanks @dduugg!

CC @SMillerDev for thoughts here, I think it'd probably make sense to make none of these nilable.

Not a clue, pretty sure I just copied this from livecheck

docs/Formula-Cookbook.md

revert table header Co-authored-by: Mike McQuaid <mike@mikemcquaid.com>

MikeMcQuaid · 2023-03-10T08:58:48Z

Thanks @jorpilo! Run brew typecheck locally and fix the error: https://github.com/Homebrew/brew/actions/runs/4379299101/jobs/7665097153#step:7:12

Essentially it's saying the Integer check here is no longer needed. If you're willing: would be good to clean up the rest of the file in this way, too, after this PR is merged!

MikeMcQuaid · 2023-03-10T09:33:09Z

Library/Homebrew/service.rb

    end

+    sig { params(value: Integer).returns(T.nilable(Integer)) }
+    def nice(value)


Suggested change

def nice(value)

def nice(value = nil)

Sorry, this bit is still needed and then then when nil below will also be needed.

We want users to be able to call nice() or nice(5) but not nice("5") or nice(nil).

Does that make sense?

Sorry for all the back and forth!

Sorry it was my bad as I don't get the objective of calling nice() without a value?
calling nice() without value shouldn't add anything to the plist or unit.

Also if we want to preserve the nil default value we need to make the argument nilable or typecheck fails.

I have reverted back to be nilable. typecheck and style pass locally. Please let me know if there is any further modification needed

Thanks

When would we want to call nice without arguments though?

I don't really see a reason why any of these methods would need to behave like a getter.

As I mentioned, you don't really want to call nice without arguments. Is there any other better way to get the value? if not, then I do not see the issue in keep it

Also, if you try to use it as a getter like the following example, it will return nil instead of 5

f = stub_formula do service do run opt_bin/"beanstalkd" nice(5) end end expect(f.service.nice).to eq(5) # This fails as it returns nil instead of 5

if I do an instance_eval in the nice case, then it does return the correct value

when nil instance_eval(&@service_block) @nice

Signed-off-by: Jorge Pinilla López <jorpilo@gmail.com>

Library/Homebrew/service.rb

dduugg · 2023-03-10T17:03:41Z

Library/Homebrew/service.rb

+      when nil
+        @nice
+      when Integer
+        if (T.must(value) < -20) || (T.must(value) > 19)


Suggested change

if (T.must(value) < -20) || (T.must(value) > 19)

unless (-20..19).cover?(nice)

This is concise, and also lets you avoid the T.musts. (You could even extract the magic numbers into a const, e.g. NICE_RANGE = -20..19)

Changed to use cover and NICE_RANGE. Thank you for the suggestion.

Any way I can use NICE_RANGE in the tests?

Could you elaborate on the issue in using NICE_RANGE in tests?

carlocab · 2023-03-11T04:34:16Z

Library/Homebrew/service.rb

+      when nil
+        @nice
+      when Integer
+        if (T.must(value) < -20) || (T.must(value) > 19)


Do you need sudo for negative nice?

You are correct, thank you for noticing. you do need root to run negative nice levels. If you try to run without root it will just run with nice level of 0 (no error will trigger).
You can run positive nice levels as user.

I can add a check for requires_root if the nice level is negative. Not sure if we should throw an error or just a warning. Also where would it best to set the check? at the nice function or when generating plist/units?

I guess a warning when you call brew services without sudo suffices here. But CC @SMillerDev and @MikeMcQuaid for thoughts here.

Since this is not something that should happen I think throwing an error is the best course of action.

It seems reasonable to truncate the nice level if you don't want to run brew services with sudo (which is what will happen automatically), though, so throwing an error seems a bit much here.

I believe requires_root still allows you to run as a user, it'll just warn against doing so. So if we require that with the nice level you can still opt to have it truncated, brew will just suggest the fully functional option.

Sounds like we should just have nice < 0 imply requires_root then, regardless of whether the service block includes it. Not sure what the value of forcing its inclusion by throwing an error is.

It would make it clear to developers and maintainers why it requires root. Instead of only telling users that they need to use root.

I don't think an extra line that says require_root true is going to make the reason more transparent.

Signed-off-by: Jorge Pinilla López <jorpilo@gmail.com>

jorpilo · 2023-03-09T22:11:56Z

Library/Homebrew/service.rb

      @run&.map(&:to_s)
    end

+    sig { params(value: T.nilable(Integer)).returns(T.nilable(Integer)) }


I will remove the nil to also resolve the typecheck issue but we might want to also change the rest of parameters. Please verify that the change is correct.

Also if we remove the nilable, the TypeError is unrechable as it expects an integer. Should I remove the it?

jorpilo · 2023-03-09T22:22:37Z

Library/Homebrew/service.rb

      options << "ExecStart=#{cmd}"

      options << "Restart=always" if @keep_alive.present? && @keep_alive[:always].present?
      options << "RestartSec=#{restart_delay}" if @restart_delay.present?


Is this correct or should it be the following:

Suggested change

options << "RestartSec=#{restart_delay}" if @restart_delay.present?

options << "RestartSec=#{@restart_delay}" if @restart_delay.present?

@SMillerDev can confirm

@ seems more consistent

jorpilo · 2023-03-10T09:38:39Z

Library/Homebrew/service.rb

    end

+    sig { params(value: Integer).returns(T.nilable(Integer)) }
+    def nice(value)


Sorry it was my bad as I don't get the objective of calling nice() without a value?
calling nice() without value shouldn't add anything to the plist or unit.

jorpilo · 2023-03-10T10:10:14Z

Library/Homebrew/service.rb

    end

+    sig { params(value: Integer).returns(T.nilable(Integer)) }
+    def nice(value)


Also if we want to preserve the nil default value we need to make the argument nilable or typecheck fails.

I have reverted back to be nilable. typecheck and style pass locally. Please let me know if there is any further modification needed

Thanks

Library/Homebrew/service.rb

jorpilo · 2023-03-11T05:14:02Z

Library/Homebrew/service.rb

+      when nil
+        @nice
+      when Integer
+        if (T.must(value) < -20) || (T.must(value) > 19)


You are correct, thank you for noticing. you do need root to run negative nice levels. If you try to run without root it will just run with nice level of 0 (no error will trigger).
You can run positive nice levels as user.

I can add a check for requires_root if the nice level is negative. Not sure if we should throw an error or just a warning. Also where would it best to set the check? at the nice function or when generating plist/units?

jorpilo · 2023-03-11T05:20:45Z

Library/Homebrew/service.rb

+      when nil
+        @nice
+      when Integer
+        if (T.must(value) < -20) || (T.must(value) > 19)


Changed to use cover and NICE_RANGE. Thank you for the suggestion.

Any way I can use NICE_RANGE in the tests?

jorpilo · 2023-03-11T05:24:58Z

Library/Homebrew/service.rb

    end

+    sig { params(value: Integer).returns(T.nilable(Integer)) }
+    def nice(value)


As I mentioned, you don't really want to call nice without arguments. Is there any other better way to get the value? if not, then I do not see the issue in keep it

jorpilo · 2023-03-12T06:23:31Z

Library/Homebrew/service.rb

    end

+    sig { params(value: Integer).returns(T.nilable(Integer)) }
+    def nice(value)


Also, if you try to use it as a getter like the following example, it will return nil instead of 5

f = stub_formula do service do run opt_bin/"beanstalkd" nice(5) end end expect(f.service.nice).to eq(5) # This fails as it returns nil instead of 5

if I do an instance_eval in the nice case, then it does return the correct value

when nil instance_eval(&@service_block) @nice

jorpilo · 2023-03-12T21:32:59Z

Also sorry for the late reply. I had all my comments as "pending" and I didn't realised I do need to accept the comments for them to be visible....

Library/Homebrew/service.rb

Co-authored-by: Douglas Eichelberger <dduugg@users.noreply.github.com>

SMillerDev · 2023-03-14T10:51:26Z

Library/Homebrew/test/service_spec.rb

    end
  end

+  describe "#nice" do


Can you add a bool check or something still?

reitermarkus · 2023-03-14T12:38:20Z

Library/Homebrew/test/service_spec.rb

+      f = stub_formula do
+        service do
+          run opt_bin/"beanstalkd"
+          nice(-21)


Suggested change

nice(-21)

nice -21

Or does RuboCop complain about this?

yeah... syntax

github-actions · 2023-04-06T00:06:02Z

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.

jorpilo added 3 commits March 10, 2023 03:06

Added service.nice

9256b1d

Added nice option to docs

54cee02

Nice tests

84dc857

jorpilo mentioned this pull request Mar 9, 2023

Nice and ThrottleInterval equivalence in brew services #14931

Closed

1 task

Corrected style

0d70ca7

MikeMcQuaid reviewed Mar 9, 2023

View reviewed changes

jorpilo and others added 3 commits March 10, 2023 03:31

Changed header

2df2867

revert table header Co-authored-by: Mike McQuaid <mike@mikemcquaid.com>

remove nilable from nice

f40c721

typo

101f06c

MikeMcQuaid approved these changes Mar 10, 2023

View reviewed changes

Removed unreachable code

d70ec99

MikeMcQuaid reviewed Mar 10, 2023

View reviewed changes

jorpilo added 2 commits March 10, 2023 21:18

reverted nice to be nilable

443c250

Signed-off-by: Jorge Pinilla López <jorpilo@gmail.com>

Added T.must to pass typecheck

2e5f317

Signed-off-by: Jorge Pinilla López <jorpilo@gmail.com>

jorpilo force-pushed the service-nice branch from 0b4159f to 2e5f317 Compare March 10, 2023 10:31

jorpilo added 2 commits March 10, 2023 22:04

Fixed test cases

2b25ee8

Signed-off-by: Jorge Pinilla López <jorpilo@gmail.com>

Fixed order in plist test case

43e3d81

Signed-off-by: Jorge Pinilla López <jorpilo@gmail.com>

MikeMcQuaid approved these changes Mar 10, 2023

View reviewed changes

MikeMcQuaid requested a review from SMillerDev March 10, 2023 12:36

dduugg reviewed Mar 10, 2023

View reviewed changes

Library/Homebrew/service.rb Outdated Show resolved Hide resolved

Library/Homebrew/service.rb Outdated Show resolved Hide resolved

dduugg reviewed Mar 10, 2023

View reviewed changes

carlocab reviewed Mar 11, 2023

View reviewed changes

jorpilo added 2 commits March 11, 2023 16:17

use cover to check range

35f9fc3

Signed-off-by: Jorge Pinilla López <jorpilo@gmail.com>

style

2dfad91

Signed-off-by: Jorge Pinilla López <jorpilo@gmail.com>

jorpilo commented Mar 12, 2023

View reviewed changes

dduugg reviewed Mar 13, 2023

View reviewed changes

Library/Homebrew/service.rb Outdated Show resolved Hide resolved

service: improve syntax.

5c567d5

Co-authored-by: Douglas Eichelberger <dduugg@users.noreply.github.com>

SMillerDev reviewed Mar 14, 2023

View reviewed changes

Library/Homebrew/test/service_spec.rb

end

end

describe "#nice" do

Copy link

Member

SMillerDev Mar 14, 2023

Choose a reason for hiding this comment

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

Can you add a bool check or something still?

reitermarkus reviewed Mar 14, 2023

View reviewed changes

reitermarkus mentioned this pull request Apr 3, 2023

Use type-safe exhaustiveness checking #15128

Closed

7 tasks

github-actions bot added the stale No recent activity label Apr 6, 2023

github-actions bot closed this Apr 13, 2023

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

github-actions bot locked as resolved and limited conversation to collaborators May 14, 2023

	sig { params(value: T.nilable(Integer)).returns(T.nilable(Integer)) }
	sig { params(value: Integer).returns(T.nilable(Integer)) }

	if (T.must(value) < -20) \|\| (T.must(value) > 19)
	unless (-20..19).cover?(nice)

	options << "RestartSec=#{restart_delay}" if @restart_delay.present?
	options << "RestartSec=#{@restart_delay}" if @restart_delay.present?

Uh oh!

Add Nice option to service #14936

Add Nice option to service #14936

Uh oh!

Conversation

jorpilo commented Mar 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MikeMcQuaid left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jorpilo Mar 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

MikeMcQuaid commented Mar 10, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

reitermarkus Mar 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jorpilo commented Mar 9, 2023 •

edited

Loading

jorpilo Mar 9, 2023 •

edited

Loading

reitermarkus Mar 11, 2023 •

edited

Loading