Hello. Example ebuild and patch for gen-b0rk attached. repoman is 9999 @ 6c1264e. Please fix.
Created attachment 437674 [details] minorsyn-quoting-multiline-0.ebuild
Created attachment 437676 [details, diff] 0001-ebuild-test-add-test-for-variables-inside-multiline-.patch
Current repoman report: RepoMan scours the neighborhood... ebuild.minorsyn 3 ebuild-test/minorsyn-quoting-multiline/minorsyn-quoting-multiline-0.ebuild: Unquoted Variable on line: 16 ebuild-test/minorsyn-quoting-multiline/minorsyn-quoting-multiline-0.ebuild: Unquoted Variable on line: 17 ebuild-test/minorsyn-quoting-multiline/minorsyn-quoting-multiline-0.ebuild: Unquoted Variable on line: 18 Expected repoman report: "If everyone were like you, I'd be out of business!"
So how would you propose such variables be quoted? As long this string isn't passed to some command that cares about paths, it really doesn't matter. Your example just shows a message that will be printed for the user; there's no need to print quotes in such a massage.
Sorry, I misread your report. Please disregard my previous comment.
This is a common complaint. However I argue that the warning is correct, on the grounds that the message refers to variables that can contain whitespace but fails to quote those variables. Note that repoman will not warn if the variables are quoted as follows: MESSAGE=" ${CATEGORY}/${PN}-${PVR} doesn't install files to '${EPREFIX}/', doesn't use temporary directory '${T}', or build directory '${S}', there are no patches in '${FILESDIR}' too. "
Also, note that changing be behavior of this quoting check might render the check useless, so we might as well remove the quoting check.
(In reply to Zac Medico from comment #6) > This is a common complaint. However I argue that the warning is correct, on > the grounds that the message refers to variables that can contain whitespace > but fails to quote those variables. The whole right side of ${MESSAGE} variable assignment is inside double quotes. Where did I fail to quote a variable? > Note that repoman will not warn if the variables are quoted as follows: > > MESSAGE=" > ${CATEGORY}/${PN}-${PVR} doesn't install files to '${EPREFIX}/', > doesn't use temporary directory '${T}', or build directory '${S}', > there are no patches in '${FILESDIR}' too. > " Sorry, but this has different contents: new quotes are added, which I don't want/need.
(In reply to Coacher from comment #8) > (In reply to Zac Medico from comment #6) > > This is a common complaint. However I argue that the warning is correct, on > > the grounds that the message refers to variables that can contain whitespace > > but fails to quote those variables. > The whole right side of ${MESSAGE} variable assignment is inside double > quotes. > Where did I fail to quote a variable? The QA check in question is more properly defined as a "whitespace leakage" check than a quote check. It checks for cases where variables that might contain whitespace are not quoted in a way that separates whitespace inside the variables from the surrounding whitespace. For example "install files to ${EPREFIX}/," does not have quotes separating whitespace in the ${EPREFIX} variable from the whitespace the immediately precedes it (the space after the word "to"). > > Note that repoman will not warn if the variables are quoted as follows: > > > > MESSAGE=" > > ${CATEGORY}/${PN}-${PVR} doesn't install files to '${EPREFIX}/', > > doesn't use temporary directory '${T}', or build directory '${S}', > > there are no patches in '${FILESDIR}' too. > > " > Sorry, but this has different contents: new quotes are added, which I don't > want/need. As explained above, the quotes that you don't want are needed to separate whitespace in the variable from surrounding whitespace.
(In reply to Zac Medico from comment #9) > The QA check in question is more properly defined as a "whitespace leakage" > check than a quote check. It checks for cases where variables that might > contain whitespace are not quoted in a way that separates whitespace inside > the variables from the surrounding whitespace. For example "install files to > ${EPREFIX}/," does not have quotes separating whitespace in the ${EPREFIX} > variable from the whitespace the immediately precedes it (the space after > the word "to"). Thanks, I understand now. Then this check is inconsistent because according to it ``` MESSAGE=" Bla bla ${EPREFIX}/, ${T}, ${S}. " einfo ${MESSAGE} ``` is illegal, but ``` einfo "Bla bla ${EPREFIX}/, ${T}, ${S}." ``` is perfectly fine. IMHO both of these should be right or wrong at the same time.
(In reply to Coacher from comment #10) > IMHO both of these should be right or wrong at the same time. Sure, but I'm afraid it might trigger a backlash from ebuild developers. :) The difference in behavior traces back to this commit: https://gitweb.gentoo.org/proj/portage.git/commit/?id=420a428b6e59afd5e5743c8ac9b19a281410e9f1
(In reply to Zac Medico from comment #11) This commit filters out "matches that appear to be an argument to a message command". Why have you said before that quoting is required in my original example with a multiline ${MESSAGE} variable? I also just pass this variable to the einfo command. Applying the above 'filtering out' logic repoman shouldn't warn about those unquoted variables. This is confusing. In the comments you say that quoting is required to separate variables from the surrounding whitespaces, but your commit allows to omit this quoting for messages, which is the most important (only?) place where these special variables with whitespaces need additional quotes (here I mean single quotes from your comment#6).
So einfo "${S}" is fine, but einfo " ${S} " is not. This doesn't make sense.
It only makes sense as a way to mitigate complains from ebuild developers.
The proper way is to add everywhere in messages extra quotes to separate variables from the surrounding whitespaces like this: einfo "'${EPREFIX}' this is easy to distinguish". And repoman allows things like this: einfo "${EPREFIX} this is hard to distinguish" just to avoid an avalanche of complaints. Right?
Correct.
(In reply to Zac Medico from comment #16) > Correct. Got it. Thank you very much for the explanation.
Check is still broken for multiline variables though. :) No warning: einfo "foo ${S} bar" Warning: einfo "foo ${S} bar" No warning: MESSAGE="foo ${S} bar"; einfo ${MESSAGE} Warning: MESSAGE="foo ${S} bar"; einfo ${MESSAGE}
repoman support has been removed per bug 835013. Please file a new bug (or, I suppose, reopen this one) if you feel this check is still applicable to pkgcheck and doesn't already exist.
