Opened 4 years ago
Last modified 2 years ago
#62642 new defect
cmake 1.1 portgroup sets -DDEBUG for the +debug variant
Reported by: | szhorvat (Szabolcs Horvát) | Owned by: | |
---|---|---|---|
Priority: | Normal | Milestone: | |
Component: | ports | Version: | 2.6.4 |
Keywords: | portgroup | Cc: | RJVB (René Bertin), cooljeanius (Eric Gallager) |
Port: |
Description (last modified by ryandesign (Ryan Carsten Schmidt))
The cmake 1.1 portgroup automatically creates the +debug
variant, and will use the compiler option -DDEBUG
. As far as I can tell, this is a completely arbitrary choice. Unlike NDEBUG
, DEBUG
is not a standard macro name, and should not be used for all ports.
In fact, this breaks the +debug
variant of the igraph port. This was completely unexpected to the igraph project: we simply did not anticipate that anyone would try to compile the library with -DDEBUG
. There is some borrowed code within igraph which had some broken printf
s fenced by #ifdef DEBUG
s, which caused the failure. I will fix this within igraph, but regardless of that, MacPorts's behaviour seems unjustified here.
I suggest simply not using -DDEBUG
by default. If a port needs it for its debug mode, it should include it explicitly, not the reverse (i.e. having to remove it explicitly).
This behaviour was originally added here: [e8f9c3dbe31f1b604b076a8753b3ea26d265e276/macports-ports]
Change History (10)
comment:1 Changed 4 years ago by szhorvat (Szabolcs Horvát)
Component: | ports → base |
---|
comment:2 follow-up: 4 Changed 4 years ago by RJVB (René Bertin)
comment:3 Changed 4 years ago by szhorvat (Szabolcs Horvát)
NDEBUG
is an _actual_ standard (it's part of the C standard): https://en.cppreference.com/w/c/error/assert It is also added by CMake's own built-in Debug
build type. All projects should be prepared to deal with the presence of an NDEBUG
.
DEBUG
is not comparable: it's not even a de-facto standard. It's something some projects happen to use for certain purposes (not necessarily for their standard debug build). I would say that unlike NDEBUG
, DEBUG
is not something that all projects _must_ be prepared to compile correctly with. Defining preprocessor symbols arbitrarily, without the package explicitly calling for it, does not look like good practice to me.
Would it not make more sense to add a feature to portfiles that lets people define what flags are needed for that specific port's debug build? These might very well not be preprocessor symbols, but CMake switches. That would be a robust solution.
Since MacPorts does not build +debug
variants by default, I don't think we really know what effect this has. Do some ports actually need it? Having DEBUG
in their source does not mean they do: for example igraph has several different SOMETHING_DEBUG
macros which are meant for igraph developers only, to debug one specific part of the library, but must not be enabled in a debug build. Do some port break with it? The only reason I even noticed this problem with igraph is that I manually tried to build the +debug
variant and we had the (IMO overzealous) -Werror
set. Otherwise, it would have just started writing undesired stuff to the terminal, which is not intended for a standard igraph debug build. This would not have been noticed.
My point is that it is unclear what the actual effects of this feature are. However, it seems to me that it leads to unpredictability, which is a negative. Having a documented way not to _remove_ this, but to add arbitrary debug-build-specific options is a predictable and much cleaner solution.
comment:4 Changed 4 years ago by szhorvat (Szabolcs Horvát)
(Just to be clear: for igraph I already fixed this upstream. I merely use igraph's situation as an illustration of how this features leads to surprising effects.)
comment:5 Changed 4 years ago by mf2k (Frank Schima)
Component: | base → ports |
---|
comment:6 Changed 4 years ago by ryandesign (Ryan Carsten Schmidt)
Description: | modified (diff) |
---|
Now that this matter has been brought to our attention, yes, it seems reasonable that the portgroup should not add -DDEBUG
by default. However, there are over 500 ports already using this portgroup. Who agrees to audit all of these ports to determine whether this change in the portgroup changes how those ports build with the +debug variant?
comment:7 Changed 4 years ago by RJVB (René Bertin)
My point exactly (but quantified :)). One of the places where I found use of the token is in X11 headers. Which stands to reason as their use of it probably goes back even further than mine. It's all nice and well that NDEBUG
was added to a C standard somewhere in the very late 90s (if that's indeed what C99 stands for), but C had been around for a very long time already by then. It beats me why no one saw a need to include a token that's the opposite of NDEBUG
(maybe the standard drafters consider debug builds the default?!). For me use of the token was clearly obvious enough not to see any need to document its inclusion in the +debug
variant - which btw is intended to build software with the maximum possible amount of debugging info.
I was planning to attach just a patchfile but since I saw a few other minor usability improvements in my own version of the PG I decide to throw them in and create a PR after all: https://github.com/macports/macports-ports/pull/10610
comment:8 Changed 4 years ago by szhorvat (Szabolcs Horvát)
If you make changes to that PortGroup file anyway, it might be a good idea to add a comment with a link to this issue, in case other wonder about why DEBUG
is present.
comment:9 Changed 3 years ago by cooljeanius (Eric Gallager)
Cc: | cooljeanius added |
---|
comment:10 Changed 2 years ago by mascguy (Christopher Nielsen)
Keywords: | portgroup added |
---|
Add keyword portgroup, to pg-related tickets
I don't know if you're referring to an official standard here, but a quick search on linux (
fgrep ' DEBUG' -R /usr/include
) shows that theDEBUG
token isn't that uncommon as you seem to think. And that frankly stands to reason given that the explicit opposite (NDEBUG
) is an apparently less disputed standard.I'm not in favour of changing this given that apparently it is an issue only now and only with a single port, not knowing how many users depend on the fact that
DEBUG
is being defined. If it weren't for that I'd probably concur with you that ports could add the token definition (and even then, most port maintainers are not also developers of the software provided by their port so you can't expect them to know this kind of detail).As it stands I'd say it's easier for ports where this definition does pose a problem to filter it out from the standard debug options set by +debug. That's not currently supported in the released version of the PG I see. I'll attach a patch that achieves this ASAP.