[logging] stop log spam introduced with ba34a62

[mkortstiege]

This disables the log spam introduced with ba34a62 while retaining CEC logging as only extra component that is enabled by default (as requested by @opdenkamp).

[MartijnKaijser]

tested on fresh install

[opdenkamp]

I'm not questioning whether or not this is correct (it is imo), but @topfs2, who is the RM, wanted all or none and I haven't seen any +1 from him on this change, or any comment on this subject.

Why is anyone touching green buttons now other than @topfs2? Or at least wait for a +1. It's an obvious change, sure, but only the only one touching buttons should be the RM.

#5669 and #5673 are a nice example of why I think only the RM should be touching buttons, and certainly not someone who's not a dev. (@MartijnKaijser please don't take this personal, but I really think that's how it should be at this stage)

[MartijnKaijser]

fyi i am the other RM

as for the others you mentioned, it was tested and discussed by 4 devs that fixed a crash. would a 5th dev in this case made any difference? I i would seriously need to wait for a +1 on this PR i might as well step aside and let topfs2 deal with it alone. Any other PRs where only a single dev is involved i would certainly not merge on my own.

[opdenkamp]

eeh right, I missed that vote that made you an RM then... but that doesn't change the fact that you don't have any insight in that code and can't judge whether it's okay to merge or not.

[opdenkamp]

so in short, even if you are an RM nowadays: I believe an RM who's a dev that understands what's going on exactly should be the only one merging code now (not talking about rebranding, add-ons and what have you, just code)

[Tolriq]

There was other cases of too quick merge :)

Anyway this fix does not fix my comment at all, it's just a hack around the main problem.

Main problem being : should specific component being able to spam logs when debug logging is not enabled ?

For me the answer is no or the description of the settings are not good at all.

So the fix should be to ensure that component does check if debug log is on before logging instead of current status where they log without this check. (Don't know how internals works, but this should be checked at the root of the logging functions and not directly on components).

cc @topfs2 to avoid copying the same answer in other PR.

[topfs2]

Yes I agree. We need to revert the change if it enabled component logging
while debug logging was off.
On 9 Nov 2014 21:09, "Tolriq" notifications@github.com wrote:

There was other cases of too quick merge :)

Anyway this fix does not fix my comment at all, it's just a hack around
the main problem.

Main problem being : should specific component being able to spam logs
when debug logging is not enabled ?

For me the answer is no or the description of the settings are not good at
all.

So the fix should be to ensure that component does check if debug log is
on before logging instead of current status where they log without this
check. (Don't know how internals works, but this should be checked at the
root of the logging functions and not directly on components).

cc @topfs2 https://github.com/topfs2 to avoid copying the same answer
in other PR.

—
Reply to this email directly or view it on GitHub
#5670 (comment).

[Montellese]

IIRC the idea was to be able to enable logging from a specific component without enabling debug logging. The problem with the recent changes however is that component logging is now enabled by default.

[topfs2]

Seems like the only sensible approach is to revert the change, as it proved
to not work well. Anyone able to do that (I'm on a train :) )
On 9 Nov 2014 21:49, "Sascha Montellese" notifications@github.com wrote:

IIRC the idea was to be able to enable logging from a specific component
without enabling debug logging. The problem with the recent changes however
is that component logging is now enabled by default.

—
Reply to this email directly or view it on GitHub
#5670 (comment).

[MartijnKaijser]

why is this still being discussed? it has been already changed for days.
This PR works as intended and only enables CEC logging.

[Montellese]

I thinks the discussion now is whether it should be possible to enable component specific logging without enabling debug logging (which is possible right now and IMO makes sense in certain situations).

[topfs2]

Because it created enormous logs on default settings, which in stable isn't
acceptable. 10mb in 2 days is to much, without rotations at least (which
AFAIK we don't have)
On 9 Nov 2014 21:52, "Martijn Kaijser" notifications@github.com wrote:

why is this still being discussed? it has been already changed for days

—
Reply to this email directly or view it on GitHub
#5670 (comment).

[MartijnKaijser]

@topfs2 those are from users who were complaining while using builds without this PR

[Tolriq]

Then description of the option should be changed or more clear, and obviously not enabled by default. (Description says to be included in the log with enable debug saying allow writing to the log, meaning : second option need first to work.)

[topfs2]

Oh lol. Its merged. Well this isn't good either and I explicitly was
against it in the other PR.
On 9 Nov 2014 21:54, "Tobias Arrskog" topfs2@kodi.tv wrote:

Because it created enormous logs on default settings, which in stable
isn't acceptable. 10mb in 2 days is to much, without rotations at least
(which AFAIK we don't have)
On 9 Nov 2014 21:52, "Martijn Kaijser" notifications@github.com wrote:

why is this still being discussed? it has been already changed for days

—
Reply to this email directly or view it on GitHub
#5670 (comment).

[MartijnKaijser]

What is the issue? This PR stopped the logspam and only leaves CEC for spamming.

[topfs2]

Yes. I was against letting cec be a special baby and being more worth than
other components. It should also be off by default.
On 9 Nov 2014 21:57, "Martijn Kaijser" notifications@github.com wrote:

What is the issue? This PR stopped the logspam and only leaves CEC for
spamming.

—
Reply to this email directly or view it on GitHub
#5670 (comment).

[MartijnKaijser]

that i agree with that CEC shouldn't need special casing. but before the paintbuckets got out for shed painting the rest had to at least be disabled

[topfs2]

Well its better but should have just reverted the initial change instead.

So if someone could make a PR for disable all (back to what if originally
was :) ) that would be awesome.
On 9 Nov 2014 22:02, "Martijn Kaijser" notifications@github.com wrote:

that i agree with that CEC shouldn't need special casing. but before the
paintbuckets got out shed painting the rest had to at least be disabled

—
Reply to this email directly or view it on GitHub
#5670 (comment).

[opdenkamp]

The initial change is what turned CEC into a component and made it a "special baby". You may revert that change, so it's back the way it was.

[topfs2]

Seriously we need to have progress on this. The decision is to make
everything components, which is how any project of decent size does
logging. You really need to look at the bigger picture on this.
On 9 Nov 2014 23:44, "Lars Op den Kamp" notifications@github.com wrote:

The initial change is what turned CEC into a component and made it a
"special baby". You may revert that change, so it's back the way it was.

—
Reply to this email directly or view it on GitHub
#5670 (comment).

[opdenkamp]

Not everything is a component, just CEC and a couple of others were turned into components, and I never objected against having it as component when it's enabled by default when people enable debug logging