The approach to call eclass functions in the toplevel of the ebuild is ugly enough, but setting dependencies this way does not work. DEP="foo" need_apache results in DEP="foo apache", RDEP="foo apache" but: DEP="foo" RDEP="foo bar" need_apache results in DEP="foo apache", RDEP="foo bar" If you set the the apache runtime dependency in this function, you have the problem the other way around. You may say - eh, but there are other ebuilds/eclasses (e.g. kde) doing ist this way. Yeah, it's broken as well, has to change in future and does only "work somehow" because portage itself is broken and doesn't distinct between R/DEPENDs correctly. We definitely don't need more eclasses breaking dependencies this way.
Well, the solution (in my mind) would be to apply the same rules to RDEPEND as we do for DEPEND in depend.apache.eclass. As we should be doing that anyway (because Apache is both a build and run-time dependency for modules), this should't be a problem. Any disagreements in doing this? Btw Carlo, are you serious re the status? ;) Afaik, portage is still broken (even in .51.16), so this in no way heads development/whatnot, thus warranting a blocker.
>Btw Carlo, are you serious re the status Yes I am. Calling eclass functions in the toplevel of the ebuild isn't sane. There should be a big fat rule that it is forbidden. >Well, the solution (in my mind) would be to apply the same rules to RDEPEND as we do for DEPEND in depend.apache.eclass. No. Assumed need_apache adds apache as dependency and runtime dependency as well, it would cause the following problem: DEP="foo bar" need_apache results in DEP="foo bar apache", RDEP="apache"
> >Btw Carlo, are you serious re the status > Yes I am. Calling eclass functions in the toplevel of the ebuild isn't sane. > There should be a big fat rule that it is forbidden. Fair enough; code in global scope is never nice, but from where I'm standing it's a necessary evil to cut down the amount of code reproduction in all of the Apache module ebuilds. > > Well, the solution (in my mind) would be to apply the same rules to > > RDEPEND as we do for DEPEND in depend.apache.eclass. > No. Assumed need_apache adds apache as dependency and runtime dependency as > well, it would cause the following problem: Well, in need_apache (the only need_apache function that has 'dynamic' dependencies) you have this snippet: DEPEND="${DEPEND} ${APACHE_DEPEND}" Please correct me if I'm wrong, but that apache2 USE-flag conditional in $APACHE_DEPEND (in depend.apache.eclass) is just that, so it shouldn't break things - it's always constant. So.. the solution I proposed should still fix things (once portage is fixed), correct?
I think what he means, is that if RDEPEND isn't set, portage sets it to DEPEND. So if we set both DEPEND and RDEPEND, RDEPEND wouldn't be what the documentation says it should be, because only DEPEND is set in the ebuild. so we just need to add a check for this in the eclass. (psuedocode) if RDEPEND then RDEPEND=RDEPEND APACHE_DEPEND else RDEPEND=DEPEND APACHE_DEPEND fi the same way portage does. We should probably check portage's code to see what exactly they do, see if there are any other checks. A better solution, would be if there was a ECLASS_DEPEND and ECLASS_RDEPEND that eclasses could use to add to DEPEND and RDEPEND safely. (Maybe this already exists and I don't know about it?)
Right. Which is pretty much what I meant when I said the fix would likely be to apply the same rule to RDEPEND as we do with DEPEND in need_apache (and since we don't touch RDEPEND now, that change would imply adding such RDEPEND chacking code)..
I just took a look at the ebuild.sh which handles inherit and related eclass functions (in fact, all of the bash part of portage). the depend.apache eclass needs to set RDEPEND, or even better use the newdepend and newrdepend functions available in ebuild.sh for adding dependencies. (but those functions essentially do the same thing we're doing in the eclass). ebuild.sh does this on inherit: DEPEND,RDEPEND (and many others) -> B_DEPEND,B_RDEPEND source eclass DEPEND,RDEPEND -> E_DEPEND,E_RDEPEND B_DEPEND,R_DEPEND -> DEPEND,RDEPEND so the result is that the DEPEND after the eclass is in E_DEPEND, while DEPEND is as it was before the eclass was sourced. Later, ebuild.sh does the following: if RDEPEND unset then RDEPEND = DEPEND and then after that: DEPEND = DEPEND E_DEPEND RDEPEND = RDEPEND E_RDEPEND So currently, with the eclass not setting RDEPEND, you get the following results: neither DEPEND or RDEPEND set DEPEND=apache RDEPEND= DEPEND=foo DEPEND=foo apache RDEPEND=foo RDEPEND=bar DEPEND=apache RDEPEND=bar DEPEND=foo RDEPEND=bar DEPEND=foo apache RDEPEND=bar This is incorrect, as apache should both be a DEPEND and RDEPEND. However, setting RDEPEND in the eclass fixes this: neither DEPEND or RDEPEND set DEPEND=apache RDEPEND=apache DEPEND=foo DEPEND=foo apache RDEPEND=foo apache RDEPEND=bar DEPEND=apache RDEPEND=bar apache DEPEND=foo RDEPEND=bar DEPEND=foo apache RDEPEND=bar apache This is how it should be. Unless there are any objections, I will fix the eclass to use newdepend and newrdepend.
E_DEPEND and E_RDEPEND exist and it's a possible way to go, but they are not used by any ebuild or eclass, yet. I'd say you'd have to get the o.k. to use it by our portage developer overlords, especially carpaski. The alternative - still a bit ugly - but it works (tm), is to set a variable NEED_APACHE="x" before the inherit line and evaluate it in the toplevel of the eclass. I favor this way, because the code in the toplevel of the ebuild will stay static. This dependency checking just to be able to call eclass functions where you never should is damn ugly. Please don't.
newdepend and newrdepend where deprecated long ago - see Bug 25013
Carlo, your $NEED_APACHE is perfect! :-) Does anyone mind making that change (calling need_apache -> setting NEED_APACHE) ?
Sigh, I should have read the last bit if comment #7. I know code global scope is ugly, causes breakages if not used carefully, etc., etc., but could everyone please read the following before cringing (because you previously saw 'code in global' ;) If we remove the use of _one_ call in global scope, which tests a single USE flag, and adds an atom to DEPEND (and soon, RDEPEND) - which would have to have been done in the ebuild anyway - we have to add all of that back to to the ebuilds. Not to mention that, in removing the call to need_apache{,1,2}, we would also have to test which version of Apache we are currently building for (via the apache2 USE flag), which would mean we would have to re-introduce approximately ~1k of code into _each_ Apache module ebuild to check for such things. Now, is it really worth removing? I'd really like to see how much this slows down dependency scanning...
the code is cleaner how it is now then if we changed to using a NEED_APACHE line before the inherit. The reason is that some modules can support multiple versions of apache. So we use need_apache for those modules, need_apache1 for the ones that support apache-1.x and need_apache2 for the ones that support apache-2.x. all three functions just set a DEPEND (and soon to be RDEPEND as well), with the need_apache setting a more complex DEPEND that includes USE-flag conditionals. If we were to move to using NEED_APACHE, our code would need to be something like: (psuedocode) switch ${NEED_APACHE) case 1 DEPEND=... RDEPEND=... APACHE_VERSION=1 case 2 DEPEND=... RDEPEND=... APACHE_VERSION=2 case ANY DEPEND=... RDEPEND=... if useq apache2 then APACHE_VERSION=2 else APACHE_VERSION=1 With how we have it setup now, we achive the same result, but in a much cleaner way, without having to use switch. IMHO functions are much cleaner then switches. In either case, it'd be running the same code, just the way it chooses the path is different (function vs. switch). Also, changing all the modules ebuilds would be a rather large task. Does the way we're doing it now actually break anything, or is there a policy document somewhere saying the way we're doing it is wrong and not allowed? If not, then we aren't going to change it. It works, and is less ugly then NEED_APACHE="x". RDEPEND fix in in CVS.
RDEPEND -> fixed. NEED_APACHE=X instead of need_apachex -> wontfix resolving.
