My last attempt at installing wine failed with doins being unable to find gecko. While I'm not sure if this was the root cause, while checking for signs of it I noticed a few QA problems with the ebuild. Most importantly, you are overriding global GV & MV variables in pkg_setup(). This is invalid per PMS since those variables are defined in global ebuild scope. As a result, PM sourcing the ebuild again (e.g. to update it) will reset GV & MV to global values, therefore discarding the overrides. This must not be used. Not to mention that this random override is thoroughly confusing to anyone reading the ebuild and finding out that the conditional from SRC_URI is not repeated in src_install(). What you do instead, is override variables used only in src_install(), a few phases back in pkg_setup().
Other nitpicks: > EGIT_BRANCH="master" 'master' seems to be the default upstream. Why repeat it? > ( "${T}"/pr66838 || false ) >/dev/null 2>&1 What does the '|| false' do there? AFAICS it only resets the return code to 1 on test failure... but then, both 1 and any random number should work the same there. > eend $? > if [[ $? -ne 0 ]] ; then eend copies its parameter as exit status. So you can do: if ! eend $?; then > if use abi_x86_32 && use opencl && [[ x$(eselect opencl show 2> /dev/null) = "xintel" ]]; then You really don't need to play with x-es in bash. > local oss_vers=$(best_version media-sound/oss) > if [[ -z ${oss_vers} ]] || ! version_is_at_least "4" ${oss_vers}; then How about: if has_version '>=media-sound-oss-4'; then ? > EGIT_REPO_URI=${STAGING_EGIT_REPO_URI} > unset ${PN}_LIVE_{REPO,BRANCH,COMMIT} EGIT_COMMIT; Playing with internals will only cause the ebuild to fail everytime a new feature is added, not to mention you're going to seriously confuse smart-live-rebuild. The eclass provides git-r3_{fetch,checkout} API to be used for additional repositories. > unpack ${P}.tar.bz2 > use staging && unpack "${STAGING_P}.tar.gz" > ... Since you are unpacking all files anyway, why not call default and let it unpack all files? > if [[ "$(md5sum server/protocol.def)" != "${md5}" ]]; then The usual way of verifying checksums is to call 'md5sum -c'. This ensures that random differences in output (like whitespace) won't cause the match to fail. > l10n_get_locales > po/LINGUAS || die. File writes can fail.
(In reply to Michał Górny from comment #0) > My last attempt at installing wine failed with doins being unable to find > gecko. While I'm not sure if this was the root cause, while checking for > signs of it I noticed a few QA problems with the ebuild. The root cause of this was that I do not propagate the desired value into src_install. I opted to override the global variable because that seemed the easiest approach. I'll obviously have to change that. > > Most importantly, you are overriding global GV & MV variables in > pkg_setup(). This is invalid per PMS since those variables are defined in > global ebuild scope. As a result, PM sourcing the ebuild again (e.g. to > update it) will reset GV & MV to global values, therefore discarding the > overrides. This must not be used. No problem. Please file a big with repoman to include a check for assignment of global variables. > > Not to mention that this random override is thoroughly confusing to anyone > reading the ebuild and finding out that the conditional from SRC_URI is not > repeated in src_install(). What you do instead, is override variables used > only in src_install(), a few phases back in pkg_setup(). (In reply to Michał Górny from comment #1) > Other nitpicks: > > > EGIT_BRANCH="master" > > 'master' seems to be the default upstream. Why repeat it? > Because I am dealing with multiple repos and semantically, it makes most sense to make sure I am on the right branch for the right repo (if necessary) > > ( "${T}"/pr66838 || false ) >/dev/null 2>&1 > > What does the '|| false' do there? AFAICS it only resets the return code to > 1 on test failure... but then, both 1 and any random number should work the > same there. > It might, but it was a compiler check added before my time, and since I cannot confirm that things work properly after changing it, I'd rather not risk breaking the compiler check > > eend $? > > if [[ $? -ne 0 ]] ; then > > eend copies its parameter as exit status. So you can do: > > if ! eend $?; then > Noted, will update, thank you. > > if use abi_x86_32 && use opencl && [[ x$(eselect opencl show 2> /dev/null) = "xintel" ]]; then > > You really don't need to play with x-es in bash. > More of a stylistic concern, but once again, I have no way of checking this currently, and I'd rather not risk breaking something something as important as this over a stylistic concern. > > local oss_vers=$(best_version media-sound/oss) > > if [[ -z ${oss_vers} ]] || ! version_is_at_least "4" ${oss_vers}; then > > How about: > > if has_version '>=media-sound-oss-4'; then > > ? > I'll have to check to see if that does the trick. Thanks for the suggestion. > > EGIT_REPO_URI=${STAGING_EGIT_REPO_URI} > > unset ${PN}_LIVE_{REPO,BRANCH,COMMIT} EGIT_COMMIT; > > Playing with internals will only cause the ebuild to fail everytime a new > feature is added, not to mention you're going to seriously confuse > smart-live-rebuild. The eclass provides git-r3_{fetch,checkout} API to be > used for additional repositories. > I asked two years ago what the best way to handle multiple repositories, and this is what I was told by several devs, after much discussion, in #gentoo-dev. If there is a better way to do it, please explicitly describe it here. > > unpack ${P}.tar.bz2 > > use staging && unpack "${STAGING_P}.tar.gz" > > ... > > Since you are unpacking all files anyway, why not call default and let it > unpack all files? > Sure, I'll take a look at that. > > if [[ "$(md5sum server/protocol.def)" != "${md5}" ]]; then > > The usual way of verifying checksums is to call 'md5sum -c'. This ensures > that random differences in output (like whitespace) won't cause the match to > fail. > I'll look into this method. Thanks. > > l10n_get_locales > po/LINGUAS > > || die. File writes can fail. Will update.
Okay, the two QA issues: reassignment of global variable GV/MV and missing die on write to file have been fixed in dd9a77d4bd4929d83b16c7d7ef8a364e254d2378. Please follow through with filing a bug against repoman to add a warning when a global variable is reassigned in local scope.
(In reply to NP-Hardass from comment #2) > (In reply to Michał Górny from comment #0) > > Most importantly, you are overriding global GV & MV variables in > > pkg_setup(). This is invalid per PMS since those variables are defined in > > global ebuild scope. As a result, PM sourcing the ebuild again (e.g. to > > update it) will reset GV & MV to global values, therefore discarding the > > overrides. This must not be used. > No problem. Please file a big with repoman to include a check for > assignment of global variables. I can't think of a good way to check that. To be more precise, PMS prohibits relying on any of the two values of those variables. So it's technically fine to define a global variable and override it in a phase function, as long as you don't use it after the phase. > (In reply to Michał Górny from comment #1) > > > if use abi_x86_32 && use opencl && [[ x$(eselect opencl show 2> /dev/null) = "xintel" ]]; then > > > > You really don't need to play with x-es in bash. > > > More of a stylistic concern, but once again, I have no way of checking this > currently, and I'd rather not risk breaking something something as important > as this over a stylistic concern. You can trust me :-P. > > > EGIT_REPO_URI=${STAGING_EGIT_REPO_URI} > > > unset ${PN}_LIVE_{REPO,BRANCH,COMMIT} EGIT_COMMIT; > > > > Playing with internals will only cause the ebuild to fail everytime a new > > feature is added, not to mention you're going to seriously confuse > > smart-live-rebuild. The eclass provides git-r3_{fetch,checkout} API to be > > used for additional repositories. > > > I asked two years ago what the best way to handle multiple repositories, and > this is what I was told by several devs, after much discussion, in > #gentoo-dev. If there is a better way to do it, please explicitly describe > it here. I named the two functions you need. They are documented in the eclass. I suppose you don't need me to copy-paste the docs for you, do you?
(In reply to Michał Górny from comment #4) > (In reply to NP-Hardass from comment #2) > > (In reply to Michał Górny from comment #0) > > > Most importantly, you are overriding global GV & MV variables in > > > pkg_setup(). This is invalid per PMS since those variables are defined in > > > global ebuild scope. As a result, PM sourcing the ebuild again (e.g. to > > > update it) will reset GV & MV to global values, therefore discarding the > > > overrides. This must not be used. > > No problem. Please file a big with repoman to include a check for > > assignment of global variables. > > I can't think of a good way to check that. To be more precise, PMS prohibits > relying on any of the two values of those variables. So it's technically > fine to define a global variable and override it in a phase function, as > long as you don't use it after the phase. > Keep track of global vars, and then scan the contents of each function to see if they have an assignment statement, would be how I'd do it. Not the simplest, but gets the job done. And obviously would just be a warning, as you state it just can't be expected to apply beyond the scope of that function/phase. > > > (In reply to Michał Górny from comment #1) > > > > if use abi_x86_32 && use opencl && [[ x$(eselect opencl show 2> /dev/null) = "xintel" ]]; then > > > > > > You really don't need to play with x-es in bash. > > > > > More of a stylistic concern, but once again, I have no way of checking this > > currently, and I'd rather not risk breaking something something as important > > as this over a stylistic concern. > > You can trust me :-P. > > > > > EGIT_REPO_URI=${STAGING_EGIT_REPO_URI} > > > > unset ${PN}_LIVE_{REPO,BRANCH,COMMIT} EGIT_COMMIT; > > > > > > Playing with internals will only cause the ebuild to fail everytime a new > > > feature is added, not to mention you're going to seriously confuse > > > smart-live-rebuild. The eclass provides git-r3_{fetch,checkout} API to be > > > used for additional repositories. > > > > > I asked two years ago what the best way to handle multiple repositories, and > > this is what I was told by several devs, after much discussion, in > > #gentoo-dev. If there is a better way to do it, please explicitly describe > > it here. > > I named the two functions you need. They are documented in the eclass. I > suppose you don't need me to copy-paste the docs for you, do you? I was advised back then to unset the variables as the proper way of doing it. I didn't understand why then, and still don't understand why now. So asking me to change it, requires that I understand why changing it works better and doesn't cause breakage. If I go to fetch multiple repos, does it need to be done in a certain way? Does not unsetting those vars cause one repo's vars to propogate to successive calls for different repos, etc? So yes, more verbosity would be appreciated, and not just a dump of the eclass documentation (since the quirks of multiple repo usage is not covered there.)
(In reply to NP-Hardass from comment #5) > I was advised back then to unset the variables as the proper way of doing > it. I didn't understand why then, and still don't understand why now. So > asking me to change it, requires that I understand why changing it works > better and doesn't cause breakage. If I go to fetch multiple repos, does it > need to be done in a certain way? Does not unsetting those vars cause one > repo's vars to propogate to successive calls for different repos, etc? > > So yes, more verbosity would be appreciated, and not just a dump of the > eclass documentation (since the quirks of multiple repo usage is not covered > there.) Long story short, because they're user configuration variables and you're not supposed to touch them. If that's going to convince you better, I'll make them readonly in the next eclass update. Furthermore, there's an open patch to add better control of repository replacements. Once that's in place, you will have to update your ebuilds to account for those. Then for any other change in eclass. That's why eclasses are supposed to do stuff, and not ebuild copy random stuff from eclasses. git-r3_fetch and git-r3_checkout are the specific functions used to implement the specific operations done by the eclass. src_unpack() is merely a wrapper calling them both with no parameters which implies using EGIT_*. If you want to fetch multiple times, then you have to use function parameters to specify the repos and not alter the global state that is supposed to be static. Now, why as the eclass author do I have to *convince* you to use the eclass API rather than random hacks suggested by random people?
(In reply to Michał Górny from comment #6) > (In reply to NP-Hardass from comment #5) > > I was advised back then to unset the variables as the proper way of doing > > it. I didn't understand why then, and still don't understand why now. So > > asking me to change it, requires that I understand why changing it works > > better and doesn't cause breakage. If I go to fetch multiple repos, does it > > need to be done in a certain way? Does not unsetting those vars cause one > > repo's vars to propogate to successive calls for different repos, etc? > > > > So yes, more verbosity would be appreciated, and not just a dump of the > > eclass documentation (since the quirks of multiple repo usage is not covered > > there.) > > Long story short, because they're user configuration variables and you're > not supposed to touch them. If that's going to convince you better, I'll > make them readonly in the next eclass update. > > Furthermore, there's an open patch to add better control of repository > replacements. Once that's in place, you will have to update your ebuilds to > account for those. Then for any other change in eclass. That's why eclasses > are supposed to do stuff, and not ebuild copy random stuff from eclasses. > > git-r3_fetch and git-r3_checkout are the specific functions used to > implement the specific operations done by the eclass. src_unpack() is merely > a wrapper calling them both with no parameters which implies using EGIT_*. > > If you want to fetch multiple times, then you have to use function > parameters to specify the repos and not alter the global state that is > supposed to be static. > > Now, why as the eclass author do I have to *convince* you to use the eclass > API rather than random hacks suggested by random people? It's mostly a concern, rather than asking you to convince me to use it. You, yourself, have told me before that the eclass wasn't designed for multiple different upstreams/repos. As such, presumably, the hacks have a purpose which is to avoid some problem that might arise because the eclass wasn't designed to do that in the first place. I am just asking for assurance that changing it isn't going to give me or my users a regression in functionality, and since this isn't intended or documented ( and the hacks exist because they must have been some unintended behavior) I was asking how to do this nonstandard operation properly, since you recommended changing it to a different setup.
(Cough) sorry to but in on a closed bug report... I've just been updating my Layman Overlay app-emulation/wine package with these Q/A suggestions. Just wanted to get clarification that it would be recommended to use: git-r3_src_unpack - for the primary repository. something like: git-r3_fetch "${STAGING_EGIT_REPO_URI}" git-r3_checkout "${STAGING_EGIT_REPO_URI}" "${STAGING_DIR}" - for the secondary repository(ies). Should I, in addition, pass in a remote reference to git-r3_fetch() for the secondary repository... So as not to interfere with the global variables: EGIT_COMMIT & EGIT_BRANCH ? As in: git-r3_fetch "${STAGING_EGIT_REPO_URI}" "${REF}" # <<--- is this right ?? Thanks Robert
(In reply to Bob Wya from comment #8) > (Cough) sorry to but in on a closed bug report... I've just been updating my > Layman Overlay app-emulation/wine package with these Q/A suggestions. > > Just wanted to get clarification that it would be recommended to use: > > git-r3_src_unpack > > - for the primary repository. > > something like: > > git-r3_fetch "${STAGING_EGIT_REPO_URI}" > git-r3_checkout "${STAGING_EGIT_REPO_URI}" "${STAGING_DIR}" Yes, something like this... > - for the secondary repository(ies). > > Should I, in addition, pass in a remote reference to git-r3_fetch() for the > secondary repository... So as not to interfere with the global variables: > EGIT_COMMIT & EGIT_BRANCH ? As in: > > git-r3_fetch "${STAGING_EGIT_REPO_URI}" "${REF}" # <<--- is this right ?? Well, if either of them is set and you need to fetch another ref, you indeed need to pass something there explicitly. HEAD is usually the choice ;-).
Thanks, that's what I thought!
