Opened 7 years ago

Closed 5 years ago

#8306 closed bug (fixed)

[PATCH] Allow entire tree to be compiled with DEBUG=2

Reported by: umccullough Owned by: umccullough
Priority: normal Milestone: R1/beta1
Component: - General Version: R1/Development
Keywords: DEBUG Cc:
Blocked By: Blocking:
Has a Patch: no Platform: All

Description

I decided to try and compile a Haiku image with the following:

jam -q -sDEBUG=2 -sHAIKU_IMAGE_SIZE=1024 @image

but ran into a few issues. A couple of broken TRACE() statements have already been fixed by modeenf, but the remainder are redefinitions of DEBUG that cause compiler warnings.

It's my opinion that we should not explicitly enable DEBUG 1 in our code, but it seems there are several places that do that. I think they should be removed/commented.

However, I have provided some patches that simple check before defining them instead, just in case these are intentional #defines. The only one I removed entirely was in the neomagic driver, which is very similar to the nvidia and matrox drivers in structure - it didn't seem like a good idea to leave it enabled there.

Comments?

Change History (15)

comment:1 Changed 7 years ago by umccullough

Has a Patch: set

comment:2 Changed 7 years ago by umccullough

Hmm, i just noticed the neomagic driver is no different than the other video drivers when it comes to defining DEBUG = 1 in be_driver_proto.h - it seems the other drivers are simply excluded from the -Werror in BuildSetup.

I'm going to obsolete that patch for now - it seems we should probably decide if all of these need to be changed instead.

comment:3 Changed 7 years ago by bonefish

Component: Build System- General
Owner: changed from bonefish to nobody
Status: newassigned

Yes, defining DEBUG in the sources is not really recommended. If a certain debug level is required, it could be set in the Jamfile. In some cases (e.g. graphics drivers + accelerants) the debug level might need to be set consistently in several subdirectories. A comment to that respect should be added then. Or DEBUG could be set in a single place (BuildSetup) for all concerned subdirectories.

comment:4 Changed 7 years ago by nielx

Just a gentle reminder: any progress on these patches? Perhaps some can be already committed?

Otherwise, please obsolete them.

comment:5 Changed 7 years ago by umccullough

Well, I obsoleted the one that I felt wasn't applicable.

Since I don't have commit access, I'm sort of dependent on others to determine if these patches are satisfactory or not.

I don't have much time to dig much deeper at the moment, but the fact that the entire tree cannot be compiled with DEBUG=2 (or any value > 1 for that matter) probably should be resolved eventually.

comment:6 in reply to:  3 ; Changed 7 years ago by nielx

In comment:3 Ingo wrote:

If a certain debug level is required, it could be set in the Jamfile. In some cases (e.g. graphics drivers + accelerants) the debug level might need to be set consistently in several subdirectories. A comment to that respect should be added then. Or DEBUG could be set in a single place (BuildSetup) for all concerned subdirectories.

So I understood that he actually was suggesting that this patch works, but is not the preferred way, which would be through the Jamfiles (or BuildSetup). Maybe he can confirm whether I understand correctly. It might be that these patches are good for now, in which case he (or I) can apply them.

comment:7 in reply to:  6 Changed 7 years ago by umccullough

Replying to nielx:

So I understood that he actually was suggesting that this patch works, but is not the preferred way, which would be through the Jamfiles (or BuildSetup). Maybe he can confirm whether I understand correctly. It might be that these patches are good for now, in which case he (or I) can apply them.

Yes, that is correct - and in the original bug description, I also made the suggestion that the code we were "fixing" should have been done differently.

So, the ticket description and Ingo's comment are in agreement - and the attached patches don't solve the problem properly (and they don't even solve all the problems in the tree, leaving the original issue still present).

If/when I have some time to revisit this, perhaps I'll put together a better solution - if obsoleting the existing patches is preferred - that can be done I suppose.

comment:8 Changed 7 years ago by umccullough

Has a Patch: unset

comment:9 Changed 6 years ago by umccullough

Owner: changed from nobody to umccullough

comment:10 Changed 5 years ago by pulkomandy

Resolution: fixed
Status: assignedclosed

Fixed in hrev48188.

Note: See TracTickets for help on using tickets.