Created attachment 149050 [details] Make sysutils/mcelog build on DragonFly * We use the USES=alias mechanism for that * Unfortunately, the upstream sources hardcode CFLAGS, which breaks it. Modify files/patch-Makefile to remove the CFLAGS= line.
Maintainer CC'd
I approve of this patch (updating Makefile and files/patch-Makefile. One thing I want to make clear here, however: the CFLAGS definition (of -Os -g) in the software's Makefile is somewhat justified. If you read the top of that Makefile, they explain why: it has to do with compiler warning flags catching certain scenarios only when -Os is in use (compared to just -O2 or -O or stock defaults). That's something that needs to be kept in mind going forward.
(In reply to Jeremy Chadwick from comment #2) > I approve of this patch (updating Makefile and files/patch-Makefile. > > One thing I want to make clear here, however: the CFLAGS definition (of -Os > -g) in the software's Makefile is somewhat justified. If you read the top > of that Makefile, they explain why: it has to do with compiler warning flags > catching certain scenarios only when -Os is in use (compared to just -O2 or > -O or stock defaults). That's something that needs to be kept in mind going > forward. Wouldn't that be essentially a developer flag then? In any case, the blocking of CFLAGS from ports is considered an error. There are three ways to resolve the error. 1) Remove the pre-definition (as ftigeot did) 2) Change the makefile to accept the addition of new CFLAGS (not possible here because -g can't be switched off and -O would conflict) 3) Remove the definition from vendor makefile but put them back in ports makefile (where they can be changed or overridden easily) I think the approach ftigeot is using is okay. As ports users we're not that interested in compilers warning about actual code, those are for the developers and the code is considered release quality now.
Correct, John. But rather than add OPTIONS_DEFINE=DEBUG and then change mcelog's Makefile to "do the right thing" with CFLAGS, I'm inclined to just say apply ftigeot's patch and be done with it. :-)
That's fine, but to be thorough you don't need to add a debug option. WITH_DEBUG defined will alter CFLAGS appropriately. In fact, this will still work with ftigeot's patch -- the CFLAG still gets set at the ports level and not overridden later at the vendor level.
I wasn't aware (until now) of the WITH_DEBUG part of the Mk framework. (Unrelated: It looks like I need to update sysutils/bsdhwmon/Makefile to honour that setting.) WITH_DEBUG will just (effectively) add -g to CFLAGS. As the mcelog software Makefile says, it's the combination of -Os -g which triggers certain warnings. So to deal with this properly, the mcelog port Makefile would need to have WITH_DEBUG knowledge + add -Os (since WITH_DEBUG will add -g itself) to do the right thing. I can write that.
I don't know how these "warnings" are bad. We should never build with -Werror, so warnings are just annoying at best. And as I said before, only the developer should care -- this should be release quality code so whatever warning we get should be harmless right? What's the download of just ignoring this "-g Os" hack?
s/download/downside/
The downside is that any warnings caused by either a) compiler changes, or b) other patches (for the BSDs, presumably), would be unseen. If the WITH_DEBUG methodology was put into place (adding -Os to CFLAGS, and letting bsd.port.mk add -g per WITH_DEBUG), then developers using WITH_DEBUG (even if just for that port) could then potentially detect bugs in advance. You need to understand something about this port: the people using it have historically been developers -- people like John Baldwin and myself. Given what MCEs are/what MCA is about, making sure this software works correctly (as much as possible) is fairly important. It's disappointing that more people do not use -Werror and that the mcelog software's Makefile doesn't use it. If I was to implement WITH_DEBUG in the port Makefile for this port, I would be adding -Werror to CFLAGS. The reason for that is that this is a highly Linux-centric piece of software and the likelihood of a warning helping shed some light on a bug that affects BSD only is semi-high. So here's the TL;DR version: I'm 100% fine with ftigeot's patch being applied as-is right now. But in the future I'll submit a PR to have the Makefile updated to include WITH_DEBUG support which will add -Os -Werror to CFLAGS.
(In reply to Jeremy Chadwick from comment #9) > The downside is that any warnings caused by either a) compiler changes, or > b) other patches (for the BSDs, presumably), would be unseen. > > If the WITH_DEBUG methodology was put into place (adding -Os to CFLAGS, and > letting bsd.port.mk add -g per WITH_DEBUG), then developers using WITH_DEBUG > (even if just for that port) could then potentially detect bugs in advance. The -Os optimization itself has issues though. I thought it was pretty messed up for gcc right now. Has overriding a -O flag and replacing with -Os violates POLA right? with a compiler change, normally it's a newer compiler that comes up with a new warning. So code that looked fine before is suddenly "bad" (usually ambiguous I guess). Regarding new patches, that should be responsibility of patcher to get it right. I still can't shake that this is primarily a "pre-release" issue. > You need to understand something about this port: the people using it have > historically been developers -- people like John Baldwin and myself. Given > what MCEs are/what MCA is about, making sure this software works correctly > (as much as possible) is fairly important. > > It's disappointing that more people do not use -Werror and that the mcelog > software's Makefile doesn't use it. You need to be careful with this. :) The pro-Werror and anti-Werror in release software factions have great wars and nobody backs done. The anti-Werror folks generally outnumber the pro-Werror folks. I have same opinion: Werror is for development and not for release. A new compiler comes up with some dumb and innocuous warning and suddenly the package is broken when it shouldn't be. without Werror the warning is still logged, so it's not suppressed. > If I was to implement WITH_DEBUG in the > port Makefile for this port, I would be adding -Werror to CFLAGS. Bapt will have you remove it immediately if it's unconditional. We have standing blanket permission to remove Werror from ports (portmgr is against it too) > The reason for that is that this is a highly Linux-centric piece of software and > the likelihood of a warning helping shed some light on a bug that affects > BSD only is semi-high. > > So here's the TL;DR version: > > I'm 100% fine with ftigeot's patch being applied as-is right now. But in > the future I'll submit a PR to have the Makefile updated to include > WITH_DEBUG support which will add -Os -Werror to CFLAGS. If -Os and -Werror are only present when WITH_DEBUG is set, I'm fine with that. WITH_DEBUG is not set by default.
Fine. Go ahead and commit the reporters' patch improvement, please.
A commit references this bug: Author: marino Date: Wed Nov 5 23:47:05 UTC 2014 New revision: 372212 URL: https://svnweb.freebsd.org/changeset/ports/372212 Log: sysutils/mcelog: Fix DF build by respecting CFLAGS and USE+=alias PR: 194825 Submitted by: ftigeot Approved by: maintainer (Jeremy Chadwick) Changes: head/sysutils/mcelog/Makefile head/sysutils/mcelog/files/patch-Makefile
ok, done. thanks guys!