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

Add --with-idle-exit-timeout configure option #294

Merged
merged 1 commit into from Nov 29, 2021

Conversation

zdohnal
Copy link
Member

@zdohnal zdohnal commented Nov 25, 2021

Updated message:

The option enables setting IdleExitTimeout to a desired value - f.e.
setting to zero will disable automatic shutdown, which is useful on servers,
where cupsd is expected to run even if there is no web interface, no jobs
and CUPS doesn't share queues.

Original message:

On servers, CUPS is expected to be running permanently, even if there
are no jobs and no web interface, and CUPS doesn't share queues.

The option will set IdleExitTimeout to 0 if used - if not used or used
with 'disable', it sets the directive to default 60s.

@zdohnal zdohnal added enhancement New feature or request priority-medium labels Nov 25, 2021
@zdohnal zdohnal added this to the v2.4 milestone Nov 25, 2021
Copy link
Member

@michaelrsweet michaelrsweet left a comment

Choose a reason for hiding this comment

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

I'm not sure why this needs to be a configure option, but I would suggest making it --with-idle-timeout=NNN instead of enable/disable since that is more consistent with autoconf usage and would allow a packager to configure a different timeout value as needed (vs. requiring a patch, which I suspect is what you are doing now?)

@zdohnal
Copy link
Member Author

zdohnal commented Nov 25, 2021

@michaelrsweet

I'm not sure why this needs to be a configure option,

The idea is that server oriented OSes can ship preconfigured CUPS which should suit better for server use cases to prevent consuming users time and additional reports, which would end up in recommendation to set the directive.

but I would suggest making it --with-idle-timeout=NNN instead of enable/disable since that is more consistent with autoconf usage and would allow a packager to configure a different timeout value as needed (vs. requiring a patch, which I suspect is what you are doing now?)

I tried to bypass value validation and reports of type 'I've set the value too low/too high and now I have problems' with choosing only unlimited option.
In this case the values are validate during reading the config, but in case of the other PR I'm planning to implement (setting systemd unit directive TimeoutStartSec for cups.service, because cups.service takes too long to start due slow disk, big data etc...) the validation on our side can happen only in .m4 file, so I've found it useful to prevent bad input at the first place.

Or we can trust in our and systemd validation code for values. WDYT?

@michaelrsweet
Copy link
Member

@zdohnal I think we can trust the input validation of cupsd and systemd to Do The Right Thing, and we can also trust that the people packaging CUPS and using this configure option have a modicum of intelligence if they decide to override the defaults. Also, --without-idle-timeout gives you your simple on/off option, just make sure to map a value of "no" to 0... :)

The option enables setting IdleExitTimeout to a desired value - f.e.
setting to zero will disable automatic shutdown, which is useful on servers,
where cupsd is expected to run even if there is no web interface, no jobs
and CUPS doesn't share queues.
@zdohnal zdohnal changed the title Add --enable-unlimited-idleexittime configure option Add --with-idle-exit-timeout configure option Nov 29, 2021
@zdohnal
Copy link
Member Author

zdohnal commented Nov 29, 2021

@michaelrsweet ok, updated - thx for the tips, I didn't think about 'no' option at all...

Copy link
Member

@michaelrsweet michaelrsweet left a comment

Choose a reason for hiding this comment

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

LGTM

@zdohnal zdohnal merged commit cb7a7b5 into OpenPrinting:master Nov 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request priority-medium
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants