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 support for passing arguments to services #386

Merged
merged 2 commits into from
Oct 8, 2024

Conversation

q66
Copy link
Contributor

@q66 q66 commented Oct 2, 2024

The argument is a part of the full service name always. Therefore, each argument instantiation is a separate instance of the service, with separate loading.

The following cases are supported:

  1. passing the argument to any dinitctl command that takes an a service name
  2. dependencies with an argument
  3. using the argument in any context that does variable substitution
  4. using the argument in dependency arguments (performing a restricted version of variable substitution)

documentation is updated, load+integration tests are updated/added

while at it, i found that depends-ms.d was being parsed wrong (it was never reading the dir) so i fixed that too

@q66 q66 force-pushed the service-arg branch 2 times, most recently from 4a909f0 to c05717d Compare October 2, 2024 12:59
@mobin-2008 mobin-2008 added Bug/Bugfix Enhancement/New Feature Improving things or introduce new feature A-Importance: Normal C-dinitctl Things about dinitctl C-dinit-service Things about dinit-service C-dinitcheck Things about dinitcheck labels Oct 2, 2024
@q66
Copy link
Contributor Author

q66 commented Oct 2, 2024

i'm a bit confused by the aarch64 failure after i force pushed?

@q66 q66 force-pushed the service-arg branch 2 times, most recently from dceaa89 to 1729dbc Compare October 2, 2024 15:39
@q66
Copy link
Contributor Author

q66 commented Oct 2, 2024

actually that was probably just a readiness race for the dependency chain, i made all the dependencies scripted (that should address that)

@davmac314
Copy link
Owner

I had a quick look and it seems ok, I'll review properly on the weekend

@davmac314
Copy link
Owner

Ok, I've looked at this fairly thoroughly. The only issue I have is that it's introducing a separate function to do another kind of substitution (what you've referred to in the docs as "minimal variable substitution"). While I get the point of minimal substitution (i.e. mostly to simplify implementation of dependency resolution) and I'm somewhat ok with not having environment variables available for these substitutions, I don't see any real point in having different rules and and especially implementation of the substitution function.

Why not make substitution for dependencies just support the full, normal substitution syntax, with the only caveat that the substitution happens before the env-file is loaded?

Also, I think that $N for any integer N (not just 1, not even just a single digit) should be parsed as a variable. Only $1 may actually resolve to a service argument, of course, but it makes me uneasy that for example $2 undergoes no substitution but $1 does. I also think that it would be better if a reference to a missing argument gave an error, rather than substituting a blank value (so $2 for example would always give an error, and $1 would give an error if no argument had been supplied). I just can't see a case where an optional argument would be useful (let me know if you do have something specific in mind though), and I think that silently allowing it may raise confusion if the service doesn't work as expected as a result.

A couple of minor formatting nits: there is an } else j++; i.e. else on the same line as the closing brace, the existing style is to always put else on the next line. In man page sources, new sentences should always start on a new line (or putting it another way, ends of sentences should always be at the end of a line) - there are a few existing exceptions to that, those are actually mistakes. (For the man pages this can apparently affect the spacing of output when they are converted to printable format). I will likely go over the man pages and tweak wording though anyway, so I can fix the formatting for those up myself.

@q66
Copy link
Contributor Author

q66 commented Oct 7, 2024

yeah that's fair, i can make these changes

wrt manpages we should probably consider moving to something better to generate them, e.g. https://git.sr.ht/~sircmpwn/scdoc because the m4 macros are a serious pain

@davmac314
Copy link
Owner

You mean the roff macros, right? Yeah I've been thinking for a while about a better solution. scdoc doesn't look like it supports the classic .TP-style paragraphs though ("definition list" style) which would be a deal breaker unless there is a workaround. I may have a play around with it.

@q66
Copy link
Contributor Author

q66 commented Oct 7, 2024

i did the changes as you asked, let me know if it's as you wanted it

@davmac314
Copy link
Owner

Maybe a couple of other minor things, I'll do a full review this evening (AU time).

@davmac314
Copy link
Owner

I need to clean up the loading code anyway, for now i'm merging as-is.

@davmac314 davmac314 merged commit d8a7ba1 into davmac314:master Oct 8, 2024
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Importance: Normal Bug/Bugfix C-dinit-service Things about dinit-service C-dinitcheck Things about dinitcheck C-dinitctl Things about dinitctl Enhancement/New Feature Improving things or introduce new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants