Bug 1278064 - building darktable does not honor RPM_OPT_FLAGS
building darktable does not honor RPM_OPT_FLAGS
Status: CLOSED EOL
Product: Fedora
Classification: Fedora
Component: darktable (Show other bugs)
22
Unspecified Unspecified
unspecified Severity high
: ---
: ---
Assigned To: Edouard Bourguignon
Fedora Extras Quality Assurance
: Reopened
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2015-11-04 11:06 EST by Ralf Corsepius
Modified: 2015-12-02 13:21 EST (History)
3 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2015-12-02 11:38:56 EST
Type: Bug
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---


Attachments (Terms of Use)
Comment out overriding CFLAGS etc. (3.93 KB, patch)
2015-11-05 09:44 EST, Ralf Corsepius
no flags Details | Diff
Apply -DCUSTOM_CFLAGS=ON (1.51 KB, patch)
2015-11-05 10:33 EST, Ralf Corsepius
no flags Details | Diff

  None (edit)
Description Ralf Corsepius 2015-11-04 11:06:04 EST
Description of problem:

Building darktable does not honor RPM_OPT_FLAGS/%optflags.

Version-Release number of selected component (if applicable):
darktable-1.6.9

How reproducible:
Always.

Steps to Reproduce:
1. build the package in mock
2. check build.log

Expected results:
The package to honor RPM_OPT_FLAGS.

Additional info:
Packages, which do not honor RPM_OPT_FLAGS need to be considered to be miscompiled and should be removed from Fedora. Actually, I am wonder why darktable currently doesn't FTBS, because cases like these, these days normally trigger an FTBFS.
Comment 1 Rex Dieter 2015-11-04 13:42:28 EST
darktable-1.6.9-1.fc21 build seems to use them ok, an example:

cd /builddir/build/BUILD/darktable-1.6.9/i686-redhat-linux-gnu/src/dtview && /usr/bin/cc  -DGDK_DISABLE_DEPRECATED -DGSEAL_ENABLE -DGTK_DISABLE_DEPRECATED -DGTK_DISABLE_SINGLE_INCLUDES -DHAVE_CONFIG_H -DHAVE_GPHOTO2 -DHAVE_GPHOTO_25_OR_NEWER -DHAVE_GRAPHICSMAGICK -DHAVE_HTTP_SERVER -DHAVE_KWALLET -DHAVE_LENSFUN -DHAVE_LIBSECRET -DHAVE_MAP -DHAVE_OPENCL -DHAVE_OPENEXR -DHAVE_OPENJPEG -DHAVE_OPENJPEG_ICC -DHAVE_RAWSPEED -DUSE_COLORDGTK -DUSE_GETTEXT -D_XOPEN_SOURCE=700 -D__GDK_KEYSYMS_COMPAT_H__ -O2 -g -pipe -Wall -Werror=format-security -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector-strong --param=ssp-buffer-size=4 -grecord-gcc-switches  -m32 -march=i686 -mtune=atom -fasynchronous-unwind-tables  -std=c99 -fopenmp -Wall -fno-strict-aliasing -pthread -mtune=generic -msse3 -g -mfpmath=sse -D_RELEASE -DNDEBUG -O3 -ffast-math -fno-finite-math-only -fexpensive-optimizations -I/builddir/build/BUILD/darktable-1.6.9/src -I/builddir/build/BUILD/darktable-1.6.9/src/external -I/builddir/build/BUILD/darktable-1.6.9/src/external/LibRaw -isystem /usr/include/glib-2.0 -isystem /usr/lib/glib-2.0/include -isystem /usr/include/gtk-2.0 -isystem /usr/lib/gtk-2.0/include -isystem /usr/include/cairo -isystem /usr/include/pango-1.0 -isystem /usr/include/atk-1.0 -isystem /usr/include/libxml2 -isystem /usr/include/libsoup-2.4 -isystem /usr/include/OpenEXR -isystem /usr/include/lensfun -isystem /usr/include/librsvg-2.0 -isystem /usr/include/gdk-pixbuf-2.0 -isystem /usr/include/json-glib-1.0 -isystem /usr/include/openjpeg-1.5 -isystem /usr/include/libsecret-1 -isystem /usr/include/GraphicsMagick -I/builddir/build/BUILD/darktable-1.6.9/i686-redhat-linux-gnu/src -I/builddir/build/BUILD/darktable-1.6.9/src/external/osm-gps-map/src -isystem /usr/include/colord-1 -I/builddir/build/BUILD/darktable-1.6.9/src/external/colord-gtk/src -isystem /usr/include/SDL -I/builddir/build/BUILD/darktable-1.6.9/src/dtview/.. -I/builddir/build/BUILD/darktable-1.6.9/i686-redhat-linux-gnu/src/dtview/..    -Werror=implicit-function-declaration -o CMakeFiles/darktable-viewer.dir/main.c.o   -c /builddir/build/BUILD/darktable-1.6.9/src/dtview/main.c
Comment 2 Ralf Corsepius 2015-11-04 13:58:16 EST
Reopening.

Have a closer look into your log:
...
-O3 -ffast-math -fno-finite-math-only -fexpensive-optimizations 
...

Putting aside the silly -msse* stuff, that's all overriding RPM_OPT_FLAGS == broken.
Comment 3 Rex Dieter 2015-11-04 14:05:58 EST
OK, thanks.  You should have included that detail in the original report.

I'd argue that those are probably mostly safe though, upstream *probabably* wouldn't have added them if they didn't know what they were doing, right? :)
Comment 4 Ralf Corsepius 2015-11-05 09:44 EST
Created attachment 1090133 [details]
Comment out overriding CFLAGS etc.

This attachment comments out most places which override CFLAGS etc. from src/CMakeLists.txt to let darktable better honor RPM_OPT_FLAGS.

I am aware this is more a brute-force hack but a real solution, but I am not sufficiently familiar with cmake to provide a better solution.
Comment 5 Rex Dieter 2015-11-05 09:47:17 EST
upstream has a better cmake flag supposedly
Comment 6 Rex Dieter 2015-11-05 09:48:02 EST
http://redmine.darktable.org/issues/10696#note-2

-DCUSTOM_CFLAGS=ON
Comment 7 Ralf Corsepius 2015-11-05 10:33 EST
Created attachment 1090183 [details]
Apply -DCUSTOM_CFLAGS=ON

Thx for the hint, Rex. Next attempt.
Comment 8 Rex Dieter 2015-11-05 11:05:06 EST
That adds -msse3 , isn't that dangerous?  (particularly on non x86_64 archs?)
Comment 9 Germano Massullo 2015-11-05 11:13:13 EST
One of upstream developer says:
[16:47] <LebedevRI> e.g. after line 83 http://pkgs.fedoraproject.org/cgit/darktable.git/tree/darktable.spec#n83
[16:48] <LebedevRI> add line    -DCUSTOM_CFLAGS=ON \

(In reply to Rex Dieter from comment #8)
> That adds -msse3 , isn't that dangerous?  (particularly on non x86_64 archs?)

Darktable needs SSE3 instruction set.
Tonight I will apply the changes
Comment 10 Rex Dieter 2015-11-05 11:16:06 EST
except, fedora's i686 arch support explicitly excludes that, doesn't it?
Comment 11 Germano Massullo 2015-11-05 11:24:30 EST
(In reply to Rex Dieter from comment #10)
> except, fedora's i686 arch support explicitly excludes that, doesn't it?

i686 should not exclude SSE3 support...
Comment 12 Rex Dieter 2015-11-05 11:35:01 EST
As far as I know, not even sse2 is supported (only cmov):

https://fedoraproject.org/wiki/Features/F12X86Support

which should still be true
Comment 13 Ralf Corsepius 2015-11-05 11:49:37 EST
(In reply to Rex Dieter from comment #8)
> That adds -msse3 , isn't that dangerous?  (particularly on non x86_64 archs?)
Yes. That's what I previously called the "-msse* stuff", darktable internally depends on -msse3, unconditionally adds it to its CFLAGS (on all archs) when not using -DCUSTOM_FLAGS and fails to build without it.

It's just that -DCUSTOM_CFLAGS makes this design flaw and non-portability explicit.

(In reply to Germano Massullo from comment #11)
> (In reply to Rex Dieter from comment #10)
> > except, fedora's i686 arch support explicitly excludes that, doesn't it?
> 
> i686 should not exclude SSE3 support...
Fedora packages are not supposed to use -msse3! This package will fail at runtime on certain processors, because it violates these processors' instruction sets. I *deliberately* ignored this aspect in my patches.

Pedantically speaking, we  would have to insist on the dependency on -msse3 to be removed or this package be removed from Fedora.

(In reply to Rex Dieter from comment #12)
> As far as I know, not even sse2 is supported (only cmov):
This matches with my memory.
Comment 14 Ralf Corsepius 2015-11-05 12:12:15 EST
Further additions:

- Darktable's implicit addition of -msse3 is visible in comment #1:
...
-fno-strict-aliasing -pthread -mtune=generic -msse3 -g -mfpmath=sse -D_RELEASE
...

- This happens when running darktable on a CPU without sse3:
$ darktable
[dt_init] unfortunately we depend on SSE3 instructions at this time.
[dt_init] please contribute a backport patch (or buy a newer processor).
Comment 15 Germano Massullo 2015-11-05 12:16:53 EST
(In reply to Germano Massullo from comment #9)
> Darktable needs SSE3 instruction set.
Comment 16 Ralf Corsepius 2015-11-05 12:21:30 EST
(In reply to Germano Massullo from comment #15)
> (In reply to Germano Massullo from comment #9)
> > Darktable needs SSE3 instruction set.

Your view - My view: Darktable needs to be fixed. It's a non-portable mess.
Comment 17 Ralf Corsepius 2015-11-06 22:32:51 EST
Germano, why did you close this bug?

This BZ is not about -msse3, this BZ is about this package doing crazy things to CFLAGS.
Comment 18 Rex Dieter 2015-11-07 08:08:20 EST
packaging guidelines only say:

"Overriding these flags for performance optimizations (for instance, -O3 instead of -O2) is generally discouraged."

1.  RPM_OPT_FLAGS are generally being used here

2.  are being overridden or added-to in some essential ways (-fsse3 is required, for example).


So, unless you can identify any concrete/real harm, I'd tend to side with upstream and the downstream maintainer opinion here.
Comment 19 Fedora End Of Life 2015-12-02 10:50:18 EST
Fedora 21 changed to end-of-life (EOL) status on 2015-12-01. Fedora 21 is
no longer maintained, which means that it will not receive any further
security or bug fix updates. As a result we are closing this bug.

If you can reproduce this bug against a currently maintained version of
Fedora please feel free to reopen this bug against that version. If you
are unable to reopen this bug, please file a new report against the
current release. If you experience problems, please add a comment to this
bug.

Thank you for reporting this bug and we are sorry it could not be fixed.
Comment 20 Ralf Corsepius 2015-12-02 11:36:52 EST
Reopening - I think this BZ was not reacted appropriately and am inclined to feel the current maintainers are overwhelmed by the package.
Comment 21 Rex Dieter 2015-12-02 11:38:56 EST
it was handled appropriately, feel free to ask fesco to intervene (and override maintainer discretion) if you feel otherwise.

Note You need to log in before you can comment on or make changes to this bug.