My PM warns me about this: Resolving: 1 steps, 2 metadata (2 gentoo) cave@1444210340: [QA e.child.message] In thread ID '2233': ... In ebuild pipe command handler for 'dev-ros/laser_assembler-9999::gentoo': ... Variable 'KEYWORDS' should not be set by /usr/portage/eclass/ros-catkin.eclass Although this is only for live ebuilds it would be cleaner to do it in the actual ebuilds.
no thanks: like this, I have 1 line in ebuild, 5 in eclass. 5 lines in ebuild * 400 ebuilds is not an option. also, please fix your PM: keywords are *unset* in eclass.
(In reply to Alexis Ballier from comment #1) > no thanks: like this, I have 1 line in ebuild, 5 in eclass. 5 lines in > ebuild * 400 ebuilds is not an option. > > > also, please fix your PM: keywords are *unset* in eclass. The _variable_ is set, there is nothing to be fixed in the PM.
(In reply to Julian Ospald (hasufell) from comment #2) > (In reply to Alexis Ballier from comment #1) > > no thanks: like this, I have 1 line in ebuild, 5 in eclass. 5 lines in > > ebuild * 400 ebuilds is not an option. > > > > > > also, please fix your PM: keywords are *unset* in eclass. > > The _variable_ is set, there is nothing to be fixed in the PM. Yes, and KEYWORDS are unset. The pure syntactic stuff could probably be fixed by replacing it by "unset KEYWORDS". But then again, this'd have greater impact if you fixed your PM to make that distinction.
(In reply to Alexis Ballier from comment #3) > (In reply to Julian Ospald (hasufell) from comment #2) > > (In reply to Alexis Ballier from comment #1) > > > no thanks: like this, I have 1 line in ebuild, 5 in eclass. 5 lines in > > > ebuild * 400 ebuilds is not an option. > > > > > > > > > also, please fix your PM: keywords are *unset* in eclass. > > > > The _variable_ is set, there is nothing to be fixed in the PM. > > Yes, and KEYWORDS are unset. > > The pure syntactic stuff could probably be fixed by replacing it by "unset > KEYWORDS". But then again, this'd have greater impact if you fixed your PM > to make that distinction. No, "unset KEYWORDS" is not allowed as per PMS. You _must_ set the variable, see https://dev.gentoo.org/~ulm/pms/head/pms.html#x1-630007 I don't think it makes sense to let the PM parse the content of the variable and then decide randomly over corner cases that might or might not be weird. As said, in this case it's a minor issue, but keep https://bugs.gentoo.org/show_bug.cgi?id=342185#c16 in mind.
(In reply to Julian Ospald (hasufell) from comment #4) > > No, "unset KEYWORDS" is not allowed as per PMS. You _must_ set the variable, > see https://dev.gentoo.org/~ulm/pms/head/pms.html#x1-630007 > Actuall this is incorrect, since KEYWORDS is under "7.3 Optional Ebuild-defined Variables" so just omitting it instead of KEYWORDS="" would probably fix the QA warning.
https://github.com/gentoo/gentoo/pull/160
<QA hat on> It is thoroughly incorrect to reset KEYWORDS via eclass like this. You are confusing other developers and arch teams, and asking for problems. Your laziness is no excuse to break fundamental QA. And before you ask why I haven't told you this earlier: it's because you didn't even give us a full week to review your eclasses, and I assumed there's no point reviewing them when I had time to once you committed them. </QA hat on>
(In reply to Julian Ospald (hasufell) from comment #4) > As said, in this case it's a minor issue, but keep > https://bugs.gentoo.org/show_bug.cgi?id=342185#c16 in mind. This is actually wanted :) In other packages that I don't plan to commit, KEYWORDS are set *after* inherit so that live ebuilds can be installed since there's no release. (In reply to Michał Górny from comment #7) > <QA hat on> > > It is thoroughly incorrect to reset KEYWORDS via eclass like this. You are > confusing other developers and arch teams, and asking for problems. Your > laziness is no excuse to break fundamental QA. How so? Fundamental QA is not to set KEYWORDS for a group of packages in an eclass, but rather in each ebuild. Fundamental QA is also about having empty KEYWORDS for live ebuilds. How is an eclass providing helpers to ensure both breaking fundamental QA? This is even defined in eclass API... # @DESCRIPTION: # Provides function for building ROS packages on Gentoo. # It supports selectively building messages, multi-python installation, live ebuilds (git only). About lazyness: I don't think factoring code is lazyness, but basic SW development good practices. Also, for the record, before this was public, live ebuild handling was done per-ebuild, and really, it was a serious mess of code duplication. > And before you ask why I haven't told you this earlier: it's because you > didn't even give us a full week to review your eclasses, and I assumed > there's no point reviewing them when I had time to once you committed them. I assumed 5 days, with a week-end in between, was plenty of time for people to at least shout "hey, I'd like to review it but have no time until XXX"
(In reply to Alexis Ballier from comment #8) > (In reply to Julian Ospald (hasufell) from comment #4) > > As said, in this case it's a minor issue, but keep > > https://bugs.gentoo.org/show_bug.cgi?id=342185#c16 in mind. > > This is actually wanted :) In other packages that I don't plan to commit, > KEYWORDS are set *after* inherit so that live ebuilds can be installed since > there's no release. This is just insane. Please add some conditionals on phase of the moon to complete your API. > (In reply to Michał Górny from comment #7) > > <QA hat on> > > > > It is thoroughly incorrect to reset KEYWORDS via eclass like this. You are > > confusing other developers and arch teams, and asking for problems. Your > > laziness is no excuse to break fundamental QA. > > How so? Fundamental QA is not to set KEYWORDS for a group of packages in an > eclass, but rather in each ebuild. Fundamental QA is also about having empty > KEYWORDS for live ebuilds. How is an eclass providing helpers to ensure both > breaking fundamental QA? It *does not* provide helpers. It intrudes in metadata in unexpected and confusing ways. > This is even defined in eclass API... > > # @DESCRIPTION: > # Provides function for building ROS packages on Gentoo. > # It supports selectively building messages, multi-python installation, live > ebuilds (git only). How so? 'It supports [...] live ebuilds' != 'it randomly kills mis-declared ebuild KEYWORDS'. > About lazyness: I don't think factoring code is lazyness, but basic SW > development good practices. Also, for the record, before this was public, > live ebuild handling was done per-ebuild, and really, it was a serious mess > of code duplication. If you call this 'good development practice', then please don't ever developer any kind of software because we don't really want software that kills all of data unless we click the 'save' button in correct order. > > And before you ask why I haven't told you this earlier: it's because you > > didn't even give us a full week to review your eclasses, and I assumed > > there's no point reviewing them when I had time to once you committed them. > > > I assumed 5 days, with a week-end in between, was plenty of time for people > to at least shout "hey, I'd like to review it but have no time until XXX" And how were we supposed we need to shout? Your initial message didn't specify any deadline. As far as I'm aware, the common practice is to wait *a week*.
(In reply to Michał Górny from comment #9) > > I assumed 5 days, with a week-end in between, was plenty of time for people > > to at least shout "hey, I'd like to review it but have no time until XXX" > > And how were we supposed we need to shout? By email. It is difficult to distinguish between lack of response meaning "all good", "i don't care", "i have no time currently", etc. As for the rest of your reply, please chill, and let's continue this discussion in a few days.
it should simply be a fatal repoman warning, can we do that?
(In reply to Julian Ospald (hasufell) from comment #11) > it should simply be a fatal repoman warning, can we do that? i dont think repoman checks eclasses
(In reply to Alexis Ballier from comment #12) > (In reply to Julian Ospald (hasufell) from comment #11) > > it should simply be a fatal repoman warning, can we do that? > > i dont think repoman checks eclasses And anyway, even if this was possible, you're assuming that this is unwanted, which I can see true only based on misunderstanding the issue at hand. (In reply to Michał Górny from comment #9) > (In reply to Alexis Ballier from comment #8) > > (In reply to Julian Ospald (hasufell) from comment #4) > > > As said, in this case it's a minor issue, but keep > > > https://bugs.gentoo.org/show_bug.cgi?id=342185#c16 in mind. > > > > This is actually wanted :) In other packages that I don't plan to commit, > > KEYWORDS are set *after* inherit so that live ebuilds can be installed since > > there's no release. > > This is just insane. Please add some conditionals on phase of the moon to > complete your API. You don't seem to understand how inherit works: it is 'source' + some exceptions that do not apply here. > > (In reply to Michał Górny from comment #7) > > > <QA hat on> > > > > > > It is thoroughly incorrect to reset KEYWORDS via eclass like this. You are > > > confusing other developers and arch teams, and asking for problems. Your > > > laziness is no excuse to break fundamental QA. > > > > How so? Fundamental QA is not to set KEYWORDS for a group of packages in an > > eclass, but rather in each ebuild. Fundamental QA is also about having empty > > KEYWORDS for live ebuilds. How is an eclass providing helpers to ensure both > > breaking fundamental QA? > > It *does not* provide helpers. It intrudes in metadata in unexpected and > confusing ways. You don't seem to understand what metadata is and how it is generated. Maybe it's confusing or intruding for you, but for me it's not, and this can easily be fixed by documenting it better. > > This is even defined in eclass API... > > > > # @DESCRIPTION: > > # Provides function for building ROS packages on Gentoo. > > # It supports selectively building messages, multi-python installation, live > > ebuilds (git only). > > How so? 'It supports [...] live ebuilds' != 'it randomly kills mis-declared > ebuild KEYWORDS'. Nothing is random. Nothing is mis-declared. (I'll skip replying to the rest, rather insulting and full of FUD, part of your reply) PS: Again, if you want this to move forward, please give some actual arguments, not random untrue assertions. So far, only thing I've seen is a QA check trying to catch real issues but also catching a non-issue; natural conclusion is to fix the QA check.
Not interested in further discussion/mail-notifications specific to this eclass. The rest will happen on the relevant QA bug.
<mgorny> 1) it works only if KEYWORDS are set before inherit <mgorny> so if someone else touches the ebuild and moves variables to common locations, BOOM -*- zlogene against such approach <mgorny> 2) it is confusing as hell <zlogene> remember there was discuss about it <mgorny> 3) if you read the ebuild, you are wtf that there are keywords but they don't apply magically <mgorny> 4) it relies on KEYWORDS being non-stackable -- unlike other metadata vars, which adds to confusion <zlogene> i see a lots of - and only one + <zlogene> so <mgorny> well, i just told him that you agree with me <mgorny> if that doesn't convince him, i guess i'll start the official process <mgorny> whatever that would e <ulm> oh my, 24 hours ago I was worried that there was virtually no discussion following my eapi 6 announcement <ulm> and now all hell broke loose <zlogene> mgorny, yes, please Do we really need to do this the hard way?
instead of saying "it is bad because I don't like it/don't understand it", what about doing real QA and proposing solutions ? so far only hasufell did, and it ended up using bump scripts, which isn't good either.
QA is about preventing future mistakes too. Confusing solutions like yours are bound to cause issues in the future. As far as I'm concerned, there's a number of alternative solutions widely deployed, and you seem to be the only one having trouble. For example, the closest solution to what you want to have is to: # in the usual place, not before inherit KEYWORDS='~foo ~bar' ... [[ ${PV} == 9999 ]] && KEYWORDS=""
(In reply to Michał Górny from comment #17) > QA is about preventing future mistakes too. Confusing solutions like yours > are bound to cause issues in the future. As far as I'm concerned, there's a > number of alternative solutions widely deployed, and you seem to be the only > one having trouble. > > For example, the closest solution to what you want to have is to: > > # in the usual place, not before inherit > KEYWORDS='~foo ~bar' > > ... > > [[ ${PV} == 9999 ]] && KEYWORDS="" Feel free to propose such a PR; I'm willing to accept it if that can calm your false accusations :) (and you don't even need your QA powers to forcibly push it!) [ "${PV}" = "9999" ] || KEYWORDS='~foo ~bar' is even shorter actually and probably more resilient to ekeyword sometimes weird behavior What I didn't like about this is that it is eclass who decides when to do a live fetch and when to do a tarball fetch, meaning that after this change, 'PV == 9999 is equivalent to live ebuild' is part of eclass API. It is arguably somewhat part of it, so that's why I'm willing to accept it.
(In reply to Alexis Ballier from comment #18) > (In reply to Michał Górny from comment #17) > > QA is about preventing future mistakes too. Confusing solutions like yours > > are bound to cause issues in the future. As far as I'm concerned, there's a > > number of alternative solutions widely deployed, and you seem to be the only > > one having trouble. > > > > For example, the closest solution to what you want to have is to: > > > > # in the usual place, not before inherit > > KEYWORDS='~foo ~bar' > > > > ... > > > > [[ ${PV} == 9999 ]] && KEYWORDS="" > > Feel free to propose such a PR I certainly won't waste my time on that again. I think it's on you to fix your ebuilds.
