Retire WITH_DEBUG and make it a proper OPTION

[madpilot]

Summary

As described in the title, I'd like to remove the global WITH_DEBUG
configuration and make it a proper option leveraging the OPTIONS
framework.

I have prepared a patch to that effect which generally works, altough
there are a few edges which need smoothing.

I "engineered" the patch by hiding the WITH_DEBUG as _WITH_DEBUG,
leveraging existing code using the old flag and adding a mechanism
for global options forced on all ports, so they can silently affect
all ports if enabled, but not show as an option dialog in ports not
already showing one. (that is, not using OPTIONS_DEFINE itself)

In writing the patch I tried to keep the behaviour of ports the
same as it is now while moving from WITH_DEBUG to DEBUG option. If,
in some part of the patch, I have diverged from this guideline
please point me at it, with the few exceptions I'll explain shortly.

Advantage for moving to DEBUG are gaining all the tools the options
framework gives us, f.e. an easier way to manage it globally from
make.conf, option helpers and so on.

The global WITH_DEBUG will still work, thanks to options compatibility
still living in bsd.options.mk. I also preserved the WITH_DEBUG_PORTS
behaviour, renaming it to DEBUG_PORTS, but I suspect keeping the
old name would be better for POLA.

Many ports are already providing an options framework driven DEBUG
option, but the dualism with WITH_DEBUG also creates confusion.
While cooking up this patch I also noticed some antipatters and
errors I have corrected when seen:

• redefining STRIP="" (Already done by framework)
• wrapping ${STRIP} in .if (not needed due to the above)
• wrapping INSTALL_TARGET=install-strip in .if (substituted with install by framework)

Some ports are already failing (due to check-sanity) when globally
setting WITH_DEBUG or the DEBUG option, due to them having other
options conflicting with DEBUG (usually OPTIMIZED_CFLAGS or more
rarely RELEASE, due to SINGLE or PREVENTS usage) enabled by default.
In the patch I address it in the java/openjdk8 port, which I modified
to avoid the issue. I'd like feedback on what I've done there. I'm
especially not sure how fragile the double definition of JDK_BUILD_TYPE
is, maybe there's a better way.

some examples I have observed showing the above behaviour (which
I have not modified in the patch) are:

www/gatling
net/udpxy
math/pari

There are a few others, if a complete list is required I'll try to
create one.

Regarding this ports, which are already failing if WITH_DEBUG is
defined globally and no further configuration is performed, is it
required for my patch to fix them or can these be dealt with and
new guidelines added to tha porter's handbook added to explain one
should take care not to conflict with DEBUG option in the future?

Thanks for all suggestions and reviews!

Test Plan

The patch needs some feedback and, I'm almost sure, improivement.
Once the strategic direction has been set and the main issues are
addressed two full tree expruns with DEBUG enabled and disabled
will be needed to make sure we're not breaking anything.

[mat]

I really think this is a bad idea.

I think it should be kept as far away as options as possible. It should be the other way around, decouple it from options processing completely, like, if there is a DEBUG option, it should not set WITH_DEBUG, there are many ports with a DEBUG option that are unaware of what it does under the hood.

The only thing that would then be needed would be to remove lines 426-428 from bsd.options.mk.

And then, it must be a user defined variable in make.conf, either globally, every port gets it when WITH_DEBUG, or on a per port basis with WITH_DEBUG_PORTS, like it already is.

Mk/bsd.options.mk

200–210

No, no, no, never. To be valid, an option MUST be in OPTIONS_DEFINE, the DOCS, NLS, EXAMPLES and IPV6 are already magic enough.

[madpilot]

In D15773#333347, @mat wrote:

I really think this is a bad idea.

Thanks for the quick feedback.

I have been sitting on this patch for a while. I posted it here exactly to get this kind of feedback to understand if I was moving in the right direction or not.

Since you make clear this is not the right direction I'll steal a little more of your time with questions to understand what could be a correct direction and, time allowing, work on a different solution.

I think it should be kept as far away as options as possible. It should be the other way around, decouple it from options processing completely, like, if there is a DEBUG option, it should not set WITH_DEBUG, there are many ports with a DEBUG option that are unaware of what it does under the hood.

To make sure I understan: you are suggesting ports defining their own "DEBUG" option and getting WITH_DEBUG defined by that as it is now is the wrong? You'd rather see a patch removing the DEBUG option from the tree and forcing users and maintainers to play well with WITH_DEBUG?

I have no objection on this. What I tried to do with my patch is solving the present confusion, with ports mixing WITH_DEBUG and the DEBUG option and sometimes rolling their own. I too have some ports I maintain with a DEBUG option.

To go down this way I think the first thing would be to expand documentation about WITH_DEBUG.

The only thing that would then be needed would be to remove lines 426-428 from bsd.options.mk.

And then, it must be a user defined variable in make.conf, either globally, every port gets it when WITH_DEBUG, or on a per port basis with WITH_DEBUG_PORTS, like it already is.

I think I understand. I can try working on this way but it will require time, and as I said above more documentation to get maintainer not to "roll their own".

Mk/bsd.options.mk

200–210

I understand the requirement.

[mat]

In D15773#333417, @madpilot wrote:

In D15773#333347, @mat wrote:

I really think this is a bad idea.

Thanks for the quick feedback.

I have been sitting on this patch for a while. I posted it here exactly to get this kind of feedback to understand if I was moving in the right direction or not.

Since you make clear this is not the right direction I'll steal a little more of your time with questions to understand what could be a correct direction and, time allowing, work on a different solution.

I think it should be kept as far away as options as possible. It should be the other way around, decouple it from options processing completely, like, if there is a DEBUG option, it should not set WITH_DEBUG, there are many ports with a DEBUG option that are unaware of what it does under the hood.

To make sure I understan: you are suggesting ports defining their own "DEBUG" option and getting WITH_DEBUG defined by that as it is now is the wrong? You'd rather see a patch removing the DEBUG option from the tree and forcing users and maintainers to play well with WITH_DEBUG?

Well, I think that about half of the ports that have a DEBUG option are not aware that it will set WITH_DEBUG and ship huge packages will all symbols and debugging information. So, no, I'd rather that when an option named DEBUG is enabled, it does not imply WITH_DEBUG=yes. It is subtlety different :-)
It is fine that ports have an option named DEBUG, some time, it is to enable more logging, or other stuff, it rarely is so that debug symbols are kept.

I have no objection on this. What I tried to do with my patch is solving the present confusion, with ports mixing WITH_DEBUG and the DEBUG option and sometimes rolling their own. I too have some ports I maintain with a DEBUG option.

To go down this way I think the first thing would be to expand documentation about WITH_DEBUG.

The thing is, WITH_DEBUG is a local, user facing variable, not a porter's one, it is something the user sets so that they get binaries/libraries with debugging symbols, it is not something you enable in a port and ship it that way.

The only thing that would then be needed would be to remove lines 426-428 from bsd.options.mk.

And then, it must be a user defined variable in make.conf, either globally, every port gets it when WITH_DEBUG, or on a per port basis with WITH_DEBUG_PORTS, like it already is.

I think I understand. I can try working on this way but it will require time, and as I said above more documentation to get maintainer not to "roll their own".

Could you give an example of a maintainer "rolling their own" so that I can understand what you mean by that?

Note that setting WITH_DEBUG is very different than setting STRIP to an empty string.

[madpilot]

To make sure I understan: you are suggesting ports defining their own "DEBUG" option and getting WITH_DEBUG defined by that as it is now is the wrong? You'd rather see a patch removing the DEBUG option from the tree and forcing users and maintainers to play well with WITH_DEBUG?

Well, I think that about half of the ports that have a DEBUG option are not aware that it will set WITH_DEBUG and ship huge packages will all symbols and debugging information. So, no, I'd rather that when an option named DEBUG is enabled, it does not imply WITH_DEBUG=yes. It is subtlety different :-)

It is fine that ports have an option named DEBUG, some time, it is to enable more logging, or other stuff, it rarely is so that debug symbols are kept.

I understand, but as I said and explain later it clearly creates confusion

The thing is, WITH_DEBUG is a local, user facing variable, not a porter's one, it is something the user sets so that they get binaries/libraries with debugging symbols, it is not something you enable in a port and ship it that way.

I see and agree with this. This kind of information needs to be in the porter's handbook and the handbook I think.

The only thing that would then be needed would be to remove lines 426-428 from bsd.options.mk.

And then, it must be a user defined variable in make.conf, either globally, every port gets it when WITH_DEBUG, or on a per port basis with WITH_DEBUG_PORTS, like it already is.

I think I understand. I can try working on this way but it will require time, and as I said above more documentation to get maintainer not to "roll their own".

Could you give an example of a maintainer "rolling their own" so that I can understand what you mean by that?

Note that setting WITH_DEBUG is very different than setting STRIP to an empty string.

I understand the difference, but I have seen various ports using the DEBUG option to explicitly set CFLAGS=-O0 -g (or variations thereof), setting STRIP_CMD="" or also conditionally choosing the INSTALL_TARGET, which are all automatically done by the framework.

Such nonsense needs to be cleaned up, and is in fact a NOP, but the confusion needs to be cleared to avoid this kind of things to creep back in.

To summarize, I'm not criticizing, in fact it looks to me we are fundamentally agreeing. I'm trying to imagine a plan of action to get the DEBUG/WITH_DEBUG mess cleaned.

From this conversation I gather it's not something that can be fixed with a single big commit, but needs a concerted work with documentation, a cleanup sweep in the tree and some persuasion work on both users and maintainers.

This is bigger than what I set out to do, but not impossible. So the first question is, what are the steps? Especially which one should be the first step?

I'd go for documentation first, cleanup in the while, since it does not need to be done in one sweep at this point.

[madpilot]

Abandoning since this idea is being rejected.