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

clamav-server: Fix startupitems for MacPorts 2.6.3 #7917

Merged
merged 1 commit into from
Aug 20, 2020

Conversation

essandess
Copy link
Contributor

clamav-server: Fix startupitems for MacPorts 2.6.3

Description

Type(s)
  • bugfix
  • enhancement
  • security fix
Tested on

macOS 10.15.6 19G73
Xcode 11.6 11E708

Verification

Have you

  • checked your Portfile with port lint?
  • tried existing tests with sudo port test?
  • tried a full install with sudo port -vst install?
  • tested basic functionality of all binary files?

@macportsbot macportsbot added maintainer maintainer: open Affects an openmaintainer port type: bugfix labels Aug 1, 2020
@macportsbot
Copy link

Travis Build #13052 Failed.

Lint results
--->  Verifying Portfile for clamav-server
--->  0 errors and 0 warnings found.

Port clamav-server fail on xcode10.3. Log
Port clamav-server fail on xcode9.4. Log
Port clamav-server fail on xcode8.3. Log
Port clamav-server fail on xcode7.3. Log

@ryandesign
Copy link
Contributor

Note that if you don't want MacPorts to interpolate variable values and procedure calls inside a string, you can save yourself a lot of backslashes by using { } as your string delimiters instead of " ".

@ryandesign
Copy link
Contributor

using { } as your string delimiters instead of " ".

However \t and \n aren't interpolated in { } strings either, so in the cases where you want those to be substituted you'll have to stick with " " strings.

@essandess
Copy link
Contributor Author

essandess commented Aug 16, 2020

using { } as your string delimiters instead of " ".

However \t and \n aren't interpolated in { } strings either, so in the cases where you want those to be substituted you'll have to stick with " " strings.

@ryandesign Do you know a one-liner to coax tcl into prepending a tab onto a { } string?

This obvious answer doesn’t work and ignores the leading whitespace:

$ echo [concat "\t" {foo bar}]
foo bar

@ryandesign
Copy link
Contributor

concat is documented to trim leading and trailing whitespace from its arguments before joining them with a space.

The simplest workaround would be to use spaces instead of tabs.

@essandess
Copy link
Contributor Author

concat is documented to trim leading and trailing whitespace from its arguments before joining them with a space.

The simplest workaround would be to use spaces instead of tabs.

I found a less morally objectionable approach:

% echo [string replace {\tfoo bar} 0 1 "\t"]
	foo bar

@ryandesign
Copy link
Contributor

That works. But isn't indenting with spaces easier to read in the portfile, and isn't whitespace irrelevant in the resulting shell script?

@essandess
Copy link
Contributor Author

That works. But isn't indenting with spaces easier to read in the portfile, and isn't whitespace irrelevant in the resulting shell script?

echo {  	foo bar}
	foo bar

🤯 🤓 Been thinking way too hard about this one.

@essandess
Copy link
Contributor Author

The trailing backslashes present other issues with tcl strings. It's easiest just to do string control within "…" and just use all the necessary backslashes.

@macportsbot
Copy link

Travis Build #13443 Passed.

Lint results
--->  Verifying Portfile for clamav-server
--->  0 errors and 0 warnings found.

Port clamav-server success on xcode10.3. Log
Port clamav-server success on xcode9.4. Log
Port clamav-server success on xcode8.3. Log
Port clamav-server success on xcode7.3. Log

@ryandesign
Copy link
Contributor

I'll merge this, but I hope you will continue to work toward simplifying the port and making it easier to read. In its current state, it is too complex for any other developer to be likely to want to touch it, and that would become a problem for us if at some point you became unable to continue maintaining the port. The same applies to some of your other ports.

Just to speak one last time about the backslashes and provide a concrete example, your portfile now says:

stop {
                "if \[ -f \"\${pidfile}\" \]; then"
                "\t/bin/kill `/bin/cat \"\${pidfile}\"` && /bin/rm -f \"\${pidfile}\""
                "else"
                "\t/usr/bin/kill -SIGUSR1 `/usr/bin/pgrep -u root fswatch` 2>/dev/null"
                "fi"
}

I find that hard to read with all the backslashes. I find the following a little easier to read:

stop {
                {if [ -f "${pidfile}" ]; then}
                {    /bin/kill `/bin/cat "${pidfile}"` && /bin/rm -f "${pidfile}"}
                {else}
                {    /usr/bin/kill -SIGUSR1 `/usr/bin/pgrep -u root fswatch` 2>/dev/null}
                {fi}
}

It could even be:

stop {
            {   if [ -f "${pidfile}" ]; then                                            }
            {       /bin/kill `/bin/cat "${pidfile}"` && /bin/rm -f "${pidfile}"        }
            {   else                                                                    }
            {       /usr/bin/kill -SIGUSR1 `/usr/bin/pgrep -u root fswatch` 2>/dev/null }
            {   fi                                                                      }
}

You don't have to use the same string style on every line. If you want variable interpolation on some lines, you can use " " strings on those lines.

Just some things to think about.

@ryandesign ryandesign merged commit a48c7cf into macports:master Aug 20, 2020
@essandess
Copy link
Contributor Author

@ryandesign I have a version that does what you suggest; it’s also important to keep the bash script readable.

I threw in the towel when none of the tcl tricks I’m aware of worked to add a trailing backslash \ line continuation within { } strings.

If you or someone knows the trick for that, please post, and I’ll convert to more readable strings within the Portfile.

@essandess
Copy link
Contributor Author

You don't have to use the same string style on every line. If you want variable interpolation on some lines, you can use " " strings on those lines.

Perhaps this is the answer.

@ryandesign
Copy link
Contributor

add a trailing backslash \ line continuation within { } strings

I don't recall encountering this situation before, but, trying it now, I see what you mean. {\\} is a string containing two backslashes, and {\} is a syntax error because the backslash escapes the closing brace. There doesn't appear to be a simple way to put a single backslash into a brace-enclosed string.

I just think it reinforces what I said before elsewhere: constructing a shell script or function line by line in Tcl code is going to be inherently cumbersome and difficult to read. Yes, startupitems has this capability to build up scripts by specifying successive lines as arguments, but the feature is so seldom used that I don't know of any ports that use it other than yours. Especially when you don't need to interpolate values from Tcl, it could be easier to put the script into a separate file in the files directory—easier to read, easier to edit. You could install the separate scripts somewhere and have the script in startupitems call them. Or with a little more work you could have the portfile read in the contents of those separate scripts, perhaps even replace some placeholders with variable values if needed, and then pass the contents directly to startupitems.

@essandess essandess deleted the clamav-server branch August 21, 2020 12:09
essandess added a commit to essandess/macports-ports that referenced this pull request Aug 21, 2020
…sible

* See macports#7917 (comment)
* Add necessary Full Disk Access notes for macOS 10.14+
ryandesign pushed a commit that referenced this pull request Aug 26, 2020
…sible

* See #7917 (comment)
* Add necessary Full Disk Access notes for macOS 10.14+
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

3 participants