Version 0.41 does not properly set `/etc/cgroup' as configuration directory; And init are useless and full of bash-isms. Please fix! Reproducible: Always
Volunteering to proxy-maintain this package.
Created attachment 396520 [details, diff] libcgroup-0.41.ebuild a& init script patch First, I just wanted to set up a simple thing to get processes classification (mainly for JACK/RT audio.) And cgconfig init script is just... completly usesless and full of bashisms. Why that complicated _default_ group setting?! Without any possibility to go otherwise... I started by removing the bashisms and then... I've just simply killed the default group thing because it's completly useless and users cannot go otherwise without too much pain and trial and errors. DOES NOT WORTH IT clearly. I was going to remove the package completly because of it. So, just let the user set a rules file and pick it up instead of imposing a monolithic default control group. And then, just sanitized cgred init script. Modern OpenRC has a nice CGroup feature, so no need to jeopardize that setup without any choice. [NOTE: Unfortunately, I don't want to have a clone of portage git tree... otherwise I could go the git pull way.]
The other ebuild could get a little clean up afterwards. Leaving it for now.
The `dosym' is unecessary... would be removed in the next iterration of the patch.
Almost forgot this... removed the pkg_postinst() completly because that's no place to place entire documentation.
You have changed a lot of thing in the ebuild and it makes it very hard to review so I suggest two things Either 1) Create separate patches for your changes so we can understand what you are trying to achieve. Or 2) Use the proxy-maintainer git repository on github to provide a pull request splitting each change into separate logical patches so we can review it properly
Lastly, this would fix bug #434698 entirely.
The ebuild just get a minor clean up... the only noticeable change is the removal of pkg_postinst(). cgred.initd has a minor clean up--sanitize the init script. cgconfig.initd: completly cleaned. This might be the major issue here. So, what to do? I don't want to waste much time on cgconfig! So, if that major clean up doesn't fit in. Fine, I can completly leave the init script untouched if necessary. Removing the bashisms can be done easily... well, once one get what the init script does. I can make a separate patch for it if necessary (without actual real testing! -- Sorry, don't want to waste my time here.)
For a bit more clarity, I can split the patch to libcgroup-0.41.ebuild.patch, cgconfig.conf-init-d.patch & cgred.conf-init-d.patch. Is it really necessary? Thanks.
(In reply to tokiclover from comment #9) > For a bit more clarity, I can split the patch to > libcgroup-0.41.ebuild.patch, cgconfig.conf-init-d.patch & > cgred.conf-init-d.patch. > > Is it really necessary? > > Thanks. nothing is really necessary. But the bigger the past the longer it will take me to review it :) So stay tuned and I will reply back when I have more comments before I commit it. Thank you for your contribution.
Created attachment 396730 [details, diff] libcgroup-0.41-2.patch Reading FDO[1] recommendations... makes mounting CGroups,--_only_ those mount points managed by CGConfig and not all mount points (like what does the old init script),--necessary. Here's a second patch, on top of the first, to do just that--unmount CGroups on service shutdown. [1]: http://www.freedesktop.org/wiki/Software/systemd/PaxControlGroups
Created attachment 396792 [details, diff] libcgroup-0.41-3.patch And... a third patch (on top of the previous) that improves libcgroup-0.41-2.patch by removing extraneous commands (`cut') in cgconfig_umount().
Created attachment 396804 [details, diff] libcgroup-0.41-3.patch (In reply to tokiclover from comment #12) > Created attachment 396792 [details, diff] [details, diff] > libcgroup-0.41-3.patch > > And... a third patch (on top of the previous) that improves > libcgroup-0.41-2.patch by removing extraneous commands (`cut') in > cgconfig_umount(). Deprecated previous patch in favor of this one which has a little clean up along the way.
Created attachment 396926 [details, diff] libcgroup-0.41.ebuild.patch A new round of patches (v2)! -- splited to libcgroup-0.41.ebuild.patch, cgconfig.con-init-d.patch & cgred.conf-init-d.patch. (Just decided to make this to ease reviewing and include the mentioned (above) `dosym' removal.) libcgroup-0.41.ebuild.patch: * remove src_test(): completly useless... or am I missing something; * remove pkg_postinst(): this no place for entire documention!; * fix configuration directory `/etc/cgroup'; * tiny clean up;
Created attachment 396928 [details, diff] cgred.conf-init-d.patch Complete clean up and sanitation of the init script... no need to define start(), stop() when defining three environment variable (`command', `command_args' & `command_background' is enough.)
Created attachment 396930 [details, diff] cgconfig.conf-init-d.patch Complete overhaul and clean up of the init script as said above--no need to jeopardize CGroups without any choice (user side) and meddle _ALL_ the CGroups when it's clearly not necessary and wanted (user choice again).
Bump. It's been a whole month... an eternity if you ask me.
I have patched the libcgroup-0.41.ebuild.patch to the current libcgroup-0.41.ebuild and ran it to src_install, using a copy revbumped. ~/cvsPortage/gentoo-x86/dev-libs/libcgroup $ ebuild libcgroup-0.41-r1.ebuild clean install >>> Completed installing libcgroup-0.41-r1 into /mnt/gen2/TmpDir/portage/dev-libs/libcgroup-0.41-r1/image/ * QA Notice: Unrecognized configure options: * * --disable-debug * --disable-debug strip: x86_64-pc-linux-gnu-strip --strip-unneeded -R .comment -R .GCC.command.line -R .note.gnu.gold-version Note: KEYWORDS="amd64 ppc ~ppc64 x86" This is stabled for 3 arches, so any change requires making a revbump and switching any stabled arches to testing. The --disable-debug I am not familiar with but it does not appear in a configure phase of the ebuild. It's non fatal. The init scripts pertain only to runtime which I am not venturing into: 1. Markos started this with you and is familiar with this type of package and ebuild. 2. He has limited free time to attend, but he surely hasn't forgotten. 3. You need exercise further patience and await Markos to review the patches. I have merely runtested phases up to and including src_install.
@Ian: You have not to do that... if not exactly knowing what changed... Even if marked stable for some arch(s), the ebuild is broken because of a partial failure to configure `/etc/cgroup' as the configuration directory. Nothing would properly work before venturing into serious debugging. So, that single fix would require bumping to a new `-rN' version. Second, the init script are completely broken with a partial port from (RH) dev init script full of bashisms and not meant to work properly with OpenRC. Nothing has changed for debug USE flag, still, judging from that partial output I'd say a minor change to configure phase would be enough e.g. `$(usex debug '--enable-debug' '')'--marked to be included to the next version of the patch then. Or, else Markos could include it at hand because it should work. Thanks for testing anyway! I've already made the testing on ~amd64 and the init scripts work as expected since the first iterration of the path. You can still test the init with something like: --/etc/cgroup/cgconfig-- group audio { perm { task { uid = root; gid = audio; } admin { uid = root; gid = audio; } } cpu { cpu.shares = 500; } } --EOF-- --/etc/cgroup/cgrules.conf-- *:jackd cpu audio/ *:jackdbus cpu audio/ *:mpv cpu audio/ *:ardour cpu audio/ --EOF-- (Or something along the lines.) Actually I am running the init services with higher CPU share with a `audio/rt' sub-group.
Huh... thanks again Ian! I've just checked everything right now an... the `command_background=1' line should be removed for cgred to start properly! Sorry for this, it just I am using Runit as SysVinit and service-manager--replacing OpenRC for daemons--for sometimes, so I miss or forget a few rechecks sometimes. Thanks again!
(In reply to tokiclover from comment #20) > Huh... thanks again Ian! I've just checked everything right now an... the > `command_background=1' line should be removed for cgred to start properly! > Sorry for this, it just I am using Runit as SysVinit and > service-manager--replacing OpenRC for daemons--for sometimes, so I miss or > forget a few rechecks sometimes. > > Thanks again! SysVinit replacement and service-manager replacement of OpenRC that is. And previous comment, some typos as well, `/etc/cgroup/cgconfig.conf' instead of `/etc/cgroup/cgconfig' etc... easy ones.
@tokiclover <that single fix would require bumping to a new `-rN' version> and <any change requires making a revbump> amount to saying the same thing. <the ebuild is broken because of a ...> no disagreement here. <judging from that partial output I'd say a minor change to configure phase would be enough........> looks good, keep working that My points were that 1. The init scripts are run time issues and I am holding back from dealing with those. Markos will. 2. He is more versed in bash init scripts and started the actions for making you proxy. 3. He is also the dev who started this proxy-maint project and has higher authority. It is better for him to peruse your patches and review. 4. You need wait for Markos to get back to you. The main purpose of my entry was to assure you your input is neither ignored nor forgotten. It also shows you there are others of us here ready to help. While I could just review and commit your patches, it is not good form. <Thanks for testing anyway!> sure. If you wish to pursue ebuild writing you can ping me in irc. Contact via bugzilla is ofc very restrictive.
0.41-r1 version is broken badly, I've attached a contents of 0.41 and 0.41-r1 to get a clue.
Created attachment 406828 [details] 0.41
Created attachment 406830 [details] 0.41-r1
I think we might need to do this ourselves.
Please assign this bug to the right maintainer! Anthony G. B. took maintainership a week ago; so, let's drop the hot potato to him then [irony]. I've opened a pull request[1] on proxymaint-repos[2], so, he might interested in reading the commits and comments there if need be. I will close it once or when the maintainer would not need it. Thanks! [1]: https://github.com/gentoo/proxy-maintainers/pulls/33 [2]: https://github.com/gentoo/proxy-maintainers
(In reply to Oleg from comment #23) > 0.41-r1 version is broken badly, I've attached a contents of 0.41 and > 0.41-r1 to get a clue. @Oleg: Why are you attaching those files (files list of the package) whithout any comment? Please, do not post unrelated stuff to not add additional confusion to a confusing and big patch. Tnaks.
(In reply to tokiclover from comment #27) > Please assign this bug to the right maintainer! Anthony G. B. took > maintainership a week ago; so, let's drop the hot potato to him then [irony]. > > I've opened a pull request[1] on proxymaint-repos[2], so, he might > interested in reading the commits and comments there if need be. I will > close it once or when the maintainer would not need it. > > Thanks! > > [1]: https://github.com/gentoo/proxy-maintainers/pulls/33 > [2]: https://github.com/gentoo/proxy-maintainers Those links lead to empty pull requests. Can you attach patches against libcgroup-0.41-r1.ebuild to this bug report. One patch for each issue you'd like addressed so I cna review each issue and the fix individually.
Anthony, pull/33. typo of one letter in the link. Use the singular. (In reply to tokiclover from comment #27) > I've opened a pull request[1] on proxymaint-repos[2], so, he might > interested in reading the commits and comments there if need be. I will > close it once or when the maintainer would not need it. > It seems this patch has been sitting unknown and unobserved for weeks. Attached files here we devs cannot miss. I would never have gone looking on that site. > [1]: https://github.com/gentoo/proxy-maintainers/pulls/33 See the plural. tokiclover while the added files might not be helpful to your eye, it's similarly unhelpful expressing such blatant tones of disapproval. It's bad enough the actual devs do so much of it. w/e they are and for, he was trying to help by providing data. Statements starting with "Please, do not" just now make me cringe. Saying please does not soften the blow. If it were me I'd be asking him to clarify why and what they are there for. Anthony does proxy-maint still belong in CC?
(In reply to Ian Delaney from comment #30) > Anthony, > > pull/33. typo of one letter in the link. Use the singular. > > (In reply to tokiclover from comment #27) > > > I've opened a pull request[1] on proxymaint-repos[2], so, he might > > interested in reading the commits and comments there if need be. I will > > close it once or when the maintainer would not need it. > > > > It seems this patch has been sitting unknown and unobserved for weeks. > Attached files here we devs cannot miss. I would never have gone looking on > that site. > > > [1]: https://github.com/gentoo/proxy-maintainers/pulls/33 > > See the plural. > > tokiclover while the added files might not be helpful to your eye, it's > similarly unhelpful expressing such blatant tones of disapproval. It's bad > enough the actual devs do so much of it. w/e they are and for, he was > trying to help by providing data. Statements starting with "Please, do not" > just now make me cringe. Saying please does not soften the blow. If it were > me I'd be asking him to clarify why and what they are there for. > > Anthony does proxy-maint still belong in CC? I'm not merging those en masse. Also they should be applied against 0.41-r1. I need them broken out and github makes it hard to do that. @tokiclover, please attach them to this bug.
(In reply to Anthony Basile from comment #31) > (In reply to Ian Delaney from comment #30) > > Anthony, > > > > pull/33. typo of one letter in the link. Use the singular. > > > > (In reply to tokiclover from comment #27) > > > > > I've opened a pull request[1] on proxymaint-repos[2], so, he might > > > interested in reading the commits and comments there if need be. I will > > > close it once or when the maintainer would not need it. > > > > > > > It seems this patch has been sitting unknown and unobserved for weeks. > > Attached files here we devs cannot miss. I would never have gone looking on > > that site. > > > > > [1]: https://github.com/gentoo/proxy-maintainers/pulls/33 > > > > See the plural. > > > > tokiclover while the added files might not be helpful to your eye, it's > > similarly unhelpful expressing such blatant tones of disapproval. It's bad > > enough the actual devs do so much of it. w/e they are and for, he was > > trying to help by providing data. Statements starting with "Please, do not" > > just now make me cringe. Saying please does not soften the blow. If it were > > me I'd be asking him to clarify why and what they are there for. > > > > Anthony does proxy-maint still belong in CC? > > > I'm not merging those en masse. Also they should be applied against > 0.41-r1. I need them broken out and github makes it hard to do that. > > @tokiclover, please attach them to this bug. okay i finally reviewed and rewrote these so things are working. thanks for the patches.
