Bug 194825 - [PATCH] Make sysutils/mcelog build on DragonFly
Summary: [PATCH] Make sysutils/mcelog build on DragonFly
Status: Closed FIXED
Alias: None
Product: Ports & Packages
Classification: Unclassified
Component: Individual Port(s) (show other bugs)
Version: Latest
Hardware: Any Any
: --- Affects Many People
Assignee: John Marino
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-11-04 21:08 UTC by ftigeot
Modified: 2014-11-05 23:48 UTC (History)
3 users (show)

See Also:
jdc: maintainer-feedback+


Attachments
Make sysutils/mcelog build on DragonFly (1.39 KB, patch)
2014-11-04 21:08 UTC, ftigeot
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description ftigeot 2014-11-04 21:08:51 UTC
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.
Comment 1 Bugzilla Automation freebsd_committer freebsd_triage 2014-11-04 21:08:51 UTC
Maintainer CC'd
Comment 2 Jeremy Chadwick 2014-11-04 22:24:14 UTC
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.
Comment 3 John Marino freebsd_committer freebsd_triage 2014-11-05 07:16:33 UTC
(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.
Comment 4 Jeremy Chadwick 2014-11-05 17:44:03 UTC
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.  :-)
Comment 5 John Marino freebsd_committer freebsd_triage 2014-11-05 17:47:19 UTC
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.
Comment 6 Jeremy Chadwick 2014-11-05 21:19:52 UTC
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.
Comment 7 John Marino freebsd_committer freebsd_triage 2014-11-05 21:28:26 UTC
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?
Comment 8 John Marino freebsd_committer freebsd_triage 2014-11-05 22:00:50 UTC
s/download/downside/
Comment 9 Jeremy Chadwick 2014-11-05 22:01:50 UTC
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.
Comment 10 John Marino freebsd_committer freebsd_triage 2014-11-05 22:29:18 UTC
(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.
Comment 11 Jeremy Chadwick 2014-11-05 23:22:52 UTC
Fine. Go ahead and commit the reporters' patch improvement, please.
Comment 12 commit-hook freebsd_committer freebsd_triage 2014-11-05 23:47:26 UTC
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
Comment 13 John Marino freebsd_committer freebsd_triage 2014-11-05 23:48:38 UTC
ok, done. thanks guys!