Summary: | webapp.eclass: webapp_serverowned should die if arguments start with ${D} | ||
---|---|---|---|
Product: | Gentoo Linux | Reporter: | Martin Mokrejš <mmokrejs> |
Component: | Eclasses | Assignee: | Gentoo Web Application Packages Maintainers <web-apps> |
Status: | CONFIRMED --- | ||
Severity: | normal | CC: | weaver |
Priority: | Normal | ||
Version: | unspecified | ||
Hardware: | All | ||
OS: | Linux | ||
Whiteboard: | |||
Package list: | Runtime testing required: | --- | |
Attachments: | GBrowse-2.39.ebuild |
Description
Martin Mokrejš
2011-08-01 18:14:48 UTC
I wonder if it dies for you or not, because we already have a post src_install check in bin/misc-functions.sh that's supposed detect this case and abort: if [[ -d ${D}/${D} ]] ; then declare -i INSTALLTOD=0 for i in $(find "${D}/${D}/"); do eqawarn "QA Notice: /${i##${D}/${D}} installed in \${D}/\${D}" ((INSTALLTOD++)) done die "Aborting due to QA concerns: ${INSTALLTOD} files installed in ${D}/${D}" unset INSTALLTOD fi (In reply to comment #1) > I wonder if it dies for you or not, because we already have a post src_install > check in bin/misc-functions.sh that's supposed detect this case and abort: > > if [[ -d ${D}/${D} ]] ; then > declare -i INSTALLTOD=0 > for i in $(find "${D}/${D}/"); do > eqawarn "QA Notice: /${i##${D}/${D}} installed in \${D}/\${D}" > ((INSTALLTOD++)) > done > die "Aborting due to QA concerns: ${INSTALLTOD} files installed in > ${D}/${D}" > unset INSTALLTOD > fi Hmm. I saw few times this error when tuning the ebuild but it certainly does NOT detect all cases, that I am quite sure. Maybe the bug is elsewhere? I was asked not to insert "|| die ..." anymore because EAPI=4 does that automagically. I think it happened to me with these dodir() or some webapp*() functions which should have died but did not. Look into current sci-biology/GBrowse-2.39, one one the lines should fail but does not. I will attach my current ebuild which still has to call chown() manually ... maybe that leads you to the answer? ;) Created attachment 281791 [details]
GBrowse-2.39.ebuild
My "current" ebuild.
(In reply to comment #2) > Hmm. I saw few times this error when tuning the ebuild but it certainly does > NOT detect all cases, that I am quite sure. Maybe the bug is elsewhere? I was > asked not to insert "|| die ..." anymore because EAPI=4 does that > automagically. I think it happened to me with these dodir() or some webapp*() Yes, I used to have in there: dodir /var/tmp/gbrowse2 || die "dodir /var/tmp/gbrowse2 failed" > functions which should have died but did not. Look into current > sci-biology/GBrowse-2.39, one one the lines should fail but does not. I will I mean the one in main tree, not the one attached here. > attach my current ebuild which still has to call chown() manually ... maybe > that leads you to the answer? ;) It's not supposed to die immediately after the dodir call. Portage is supposed to abort after src_install completes, using the code that I pasted in comment #1. The die should get triggered by a normal emerge invocation, by running `ebuild GBrowse-2.39.ebuild install`. `dodir "${D}"...` is wrong (In reply to comment #6) > `dodir "${D}"...` is wrong Would you agree that 'repoman' can catch this at the very beginning at least? Do I have to compile the whole thing to realize that? A repoman check would be nice. It would have to check docinto, docompress, dodir, insinto, into, exeinto, fowners, and fperms. Also, dohard and dosed, which are deprecated in EAPI 4. Are there any others? In addition to ${D}, beginning with EAPI 3 we should also check for ${ED} and ${EPREFIX}. i dont generally believe in python code parsing shell code and doing it sanely It seems that we already have this check, but it could be extended to include more of the helpers mentioned in comment #5: http://git.overlays.gentoo.org/gitweb/?p=proj/portage.git;a=commit;h=8e1c57067c4556ee2c730f0722d344c52cf4888c This is fixed in git: http://git.overlays.gentoo.org/gitweb/?p=proj/portage.git;a=commit;h=022e09dd6ddd8372a9ec2ef2284b7cb174e6e6b2 But the issue I happened to face was with dodir() so I think this code is maybe not called for EAPI4? Or would you mind checking the webapp-config code path? (In reply to comment #13) > But the issue I happened to face was with dodir() so I think this code is maybe > not called for EAPI4? This repoman check works for me with EAPI 4. Are you sure that you ran repoman and carefully reviewed the output? > Or would you mind checking the webapp-config code path? I'm not sure what you mean. Repoman doesn't execute any code, it just searches for patterns, so code paths should be irrelevant. (In reply to comment #14) > (In reply to comment #13) > > But the issue I happened to face was with dodir() so I think this code is maybe > > not called for EAPI4? > > This repoman check works for me with EAPI 4. Are you sure that you ran repoman > and carefully reviewed the output? Well, not sure at which moment did that or had this very version of the ebuild. Will test later once the changes get released for test arches. > > > Or would you mind checking the webapp-config code path? > > I'm not sure what you mean. Repoman doesn't execute any code, it just searches > for patterns, so code paths should be irrelevant. The line in original report and its associated error webapp_serverowned -R "${D}"/var/tmp/gbrowse2 "${D}"/var/db/gbrowse2 (In reply to comment #15) > The line in original report and its associated error > webapp_serverowned -R "${D}"/var/tmp/gbrowse2 "${D}"/var/db/gbrowse2 That explains it. The repoman check doesn't work for webapp_serverowned since it's an eclass function, not a portage function. I'd prefer not to add check like this for eclass APIs. Other things, like paths installed to $D via Makefile internals, can't be checked until after src_install. I think that's acceptable, especially since an ebuild writer typically learns pretty quickly not to make this kind of mistake. It's just part of the learning curve. (In reply to comment #16) > (In reply to comment #15) > > The line in original report and its associated error > > webapp_serverowned -R "${D}"/var/tmp/gbrowse2 "${D}"/var/db/gbrowse2 > > That explains it. The repoman check doesn't work for webapp_serverowned since > it's an eclass function, not a portage function. I'd prefer not to add check > like this for eclass APIs. Other things, like paths installed to $D via > Makefile internals, can't be checked until after src_install. I think that's Why cannot repoman check for the leading "${D}" and report that? > acceptable, especially since an ebuild writer typically learns pretty quickly > not to make this kind of mistake. It's just part of the learning curve. Took me several days and current maintainer had no time for years and sci herd is overloaded with complex package bundles. And bugzilla is full of requests involving webapp-config interaction. This is the difficult part, not running econf or sed to fix some Makefile.am files, etc. Notably, I realized the error only because I directly tried to chmod some perms in "${D}" and chmod complained file does not exist. Took me while then to get the webapp_serverowned -related error to have something clear to be reported as an example. So what I mean is that silently I pushed via "ebuild ... qmerge" into the filesystem "${D}"/"${D}"/usr/.../whatever. Please try to do something in this regard. (In reply to comment #17)> > Why cannot repoman check for the leading "${D}" and report that? A leading "${D}" maybe be appropriate for some APIs and not others. > > acceptable, especially since an ebuild writer typically learns pretty quickly > > not to make this kind of mistake. It's just part of the learning curve. > > Took me several days and current maintainer had no time for years and sci herd > is overloaded with complex package bundles. And bugzilla is full of requests > involving webapp-config interaction. This is the difficult part, not running > econf or sed to fix some Makefile.am files, etc. > > Notably, I realized the error only because I directly tried to chmod some perms > in "${D}" and chmod complained file does not exist. Took me while then to get > the webapp_serverowned -related error to have something clear to be reported as > an example. So what I mean is that silently I pushed via "ebuild ... qmerge" > into the filesystem "${D}"/"${D}"/usr/.../whatever. Please try to do something > in this regard. I would suggest to modify the eclass function to check for this case and die if it is detected. Here's an example of what the beginning of the eclass function should look like: [[ ${1} == "${D}"* ]] && die "Arguments should not begin with \${D}: ${1}" (In reply to comment #18) > I would suggest to modify the eclass function to check for this case and die if > it is detected. Here's an example of what the beginning of the eclass function > should look like: > > [[ ${1} == "${D}"* ]] && die "Arguments should not begin with \${D}: ${1}" Yes, please. ;) (In reply to comment #19) > (In reply to comment #18) > > I would suggest to modify the eclass function to check for this case and die if > > it is detected. Here's an example of what the beginning of the eclass function > > should look like: > > > > [[ ${1} == "${D}"* ]] && die "Arguments should not begin with \${D}: ${1}" > > Yes, please. ;) Okay, reassigning to web-apps then. |