net-p2p/classified-ads: v0.10 version bump. #1406
Conversation
Package-Manager: portage-2.2.26
|
|
||
| EAPI=6 | ||
| PLOCALES="en fi sv da uk" | ||
| PLOCALE_BACKUP="en" |
There was a problem hiding this comment.
and where is the rest of the l10n eclass?
There was a problem hiding this comment.
It does appear this eclass is not inherited. PLOCALES && PLOCALE_BACKUP are indeed vars from the l10n eclass.
There was a problem hiding this comment.
Although inherited, still not implemented.
|
Also do not append periods after commit message headers |
|
rindeal, also mention points are relatively harmless and trivial when they are relatively harmless and trivial. While correct, I probably would not have bothered mention myself |
monsieurp
added
version bump
assigned
|
I'm using the unreleased version of repoman and here's the output: Per GLEP 68, <upstream> <maintainer> has to have a <name> element. Unfortunately, current portage does not catch this so it's normal that this is missed. |
There is nothing banning the use of '.' at the end of the summary line. There are commits in both forms in the repo. It's a stylistic nitpick but I will admit that I personally don't append periods either. Just wanted to clarify. |
|
Hi, I see that you are actually the upstream for this package. Any reason why you'd want to download by commit hash when you already released a version which can be downloaded via the following link: https://github.com/operatornormal/classified-ads/archive/0.10.tar.gz ? |
Should include |
|
you are all keen. I was awaiting him to make the changes before doing testing. This now needs user to push next set of changes to catch up |
|
@idella I see that I may be interfering with your interaction with the user here. I apologize for that. Unsubscribing myself. Good luck! |
|
Thanks everybody for comments, the ebuild in question is actually starting to look cleaner after all the suggestions I've got. Some suggestions were not not implemented, I'll try to briefly comment each suggestion above, everything combined into same posting but grouped by person: Rindeal:
|
|
Continuation from comment at several minutes ago:
Any additional suggestions to lift sw quality slihtly up? |
|
gktrk not interfering from my opinion. I was noting your reviewing and input was somewhat ahead of mine in that I was holding back on runtesting until after user had applied changes from feedback. afaiac your input was well intended and on track |
|
sys-devel/gettext comes with @System, we discussed this and decided it is redundant, strongly discouraged form inclusion under deps. |
|
Pushed again, sys-devel/gettext is not there any more. Also added $S to sub-directory reference as it turned out that the package could not be merged if both doc&test features were used at the same time. Antti |
|
despite runtesting ok, seems you have repeated the same commit msg for the 2 edits since re-making this pull req afresh. Can you squash them into one. If at all, each commit need state the new change, in this case addition of a missed eclass, removal of unrequired dep. You can alternatively over write all 3 with a new one outlining the changes in the bumped ebuild compared to the prior version last added. Points like "VIRTUALX_DEPEND is gone" do not warrant mention. You don't need to say something you added wrongly only to have reversed it. |
|
Another inchannel also observed that the IUSE debug is never actually used in the ebuild. Another innocent oversight I suspect. |
review comments: added "inherit l10n", added dependency to virtual/libintl, removed dependency to $VIRTUALX_DEPEND, changed again way how test suite is configured and run. Package-Manager: portage-2.2.26
operatornormal
force-pushed
the
classified_ads_v010_2
branch
from
3746c03
to
87e478d
Compare
|
Those identical commit messages were a misunderstanding, I was under impression that keeping commits messages short is considered the approved way to go. I now tried to reset the latest commits and make a new one, how does the "history" now look? At least I'm seeing only 2 commits now.. Antti |
|
I could edit the commit msg on commit to tidy up the syntax. As much as I'd like to add this there are a few little points that may as well be tidied. pwd is already "${S}" therefore redundant. It does not break but it is also not 'right'
|
|
On checking, running it in src_configure is fine. It generates the makefile rather than running it |
|
Regarding usage of ${S}: while both USE doc and test are selected, current directory after "cd doc" will be "doc" and at test phase it will attempt to cd to directory test from But I can tidy the commit message and change ||die -calls in case when cd fails. |
|
OH I see; so go |
Based on comments at pull request - used pushd/popd instead of cd in ebuild - removed IUSE debug that was not referenced anywhere Package-Manager: portage-2.2.26
|
ok this is fine now, (I missed prompting with "> dev/null> for pushd also, I shall add it myself) I am miffed about the commit msgs which I thought you'd be more versed with. These were in fact part of the list of the short list of that bug. The commit msgs don't need such a full running commentary on events leading to the change. They just need the change. I would now be looking at putting fwd one for stabilising (remember deps need a stable candidate) and removing old. I'd also suggest 'backporting' the improvements made in this bump to the ebuilds you select to remain after purging old. Do that all later in a fresh pull req |
added locally due to tech difficulties of applying the 3 commits in the one patch from #1406, submitted by user maintianer, which reports full history of the changes and input leading to this vn. bump - deps upgraded - testsuite upgraded - refinements to basic code and to selection of phases to build and install the package. Closes Pull request: #1406 Package-Manager: portage-2.2.28
|
|
||
| src_install() { | ||
| emake install INSTALL_ROOT="${D}" DESTDIR="${D}" | ||
| use doc && dodoc -r doc/doxygen.generated/html/ |
There was a problem hiding this comment.
Better alternative:
use doc && HTML_DOCS+=( doc/doxygen.generated/html )
einstalldocsThere was a problem hiding this comment.
I'll try to have this documentation installation method in next release. About appending to /dev/null I've noticed that my /dev/null is not full yet but I'm working on that..
There was a problem hiding this comment.
The best is always to start the new branch right away, but as you're currently busy with /dev/null, just take your time.
Classified ads upstream version 0.10 bump.
This PR is continuation of #1327. See also QA issues listed in https://bugs.gentoo.org/show_bug.cgi?id=581910.
Compared to previously released version 0.09_p20151220 changes include:
Compared to last version of #1327 changes include
Btw, it seems to me that while previous #1327 got reverted and ebuilds in directory are 4 months old as they should, the manifest is more recent. Was manifest reverted too? This PR here now includes a new manifest and git didn't report any conflict yet.
Package-Manager: portage-2.2.26