bugGNUstep - Bugs: bug #52518, [GWorkspace] Unnecessarily complex...

Group
 
 

bug #52518: [GWorkspace] Unnecessarily complex build system

Submitter:  Yavor Doganov <yavor>
Submitted:  Sun 26 Nov 2017 07:11:13 PM UTC
   
 
Category:  Application Severity:  3 - Normal
Item Group:  Change Request Status:  Fixed
Privacy:  Public Assigned to:  rmottola
Open/Closed:  Closed
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Thu 01 Mar 2018 01:10:08 PM UTC, comment #8: 

I didn't observe any problems on the platforms I have access to.

Yavor Doganov <yavor>
Wed 28 Feb 2018 06:59:58 PM UTC, comment #7: 

I tweaked "distclean" a little bit more and fixed a couple of more exotic platforms.
Seems fine on most places where I tested (different Linux, NetBSD, FreeBSD, OpenBSD, Solaris)

Riccardo Mottola <rmottola>
Group Member
Sat 06 Jan 2018 07:15:13 PM UTC, comment #6: 

What makes the patch large is the gazillion configure scripts.  If you filter them out it should be fairly easy to review.  There are also many duplicated configure checks and even macro definitions that the patch cumulates at one place.  I can split it in 56 chunks if you want, but it'll certainly take much more time to review and test them one by one.

You can also test it on all other platforms you have access to, it just takes time.  If anything is broken I'm confident I can fix it quickly.

GNUstep Make doesn't support out of tree builds only if you follow the usual practice for writing GNUstep makefiles.  But if you use autoconf to generate them and the vpath GNU make directive you get VPATH builds out of the box and without too much effort.  It's a matter of good practice to #include <config.h> but it's entirely up to you whether to follow it.

Yavor Doganov <yavor>
Sat 06 Jan 2018 06:17:46 PM UTC, comment #5: 

I have difficulties reviewing and accepting such a large patch and its testing will require lots of regression on different platforms. I spent hours tweaking the existing system  of Enrico for many small issues. A  mega-patch might have unexpected consequences.
Especially inspectors were carefully tuned.

The current build system may be convoluted, but  it is tested on 3 BSDs, Solaris, different Linux flavours and even HURD. Are there open issues?
Only the MDKit

I may add that as far as I know gnustep-make does not support off-tree builds at all.

Also, I am now confused by what I should review, Yavor or Ivan's work.
I would have preferred a more gradual approach.

Riccardo Mottola <rmottola>
Group Member
Tue 02 Jan 2018 11:35:49 PM UTC, comment #4: 

(1) FND_LIBS (and GUI_LIBS) is recommended by the GNUstep Make manual instead of hardcoding -base/-gui as these variables are defined to the appropriate value on Apple and non-Apple systems.

(2) -L flags belong to LIBS and not LDFLAGS.  On some systems (proprietary Unix flavors, I guess), the library will not be found if it's in a non-standard directory and -L <dir> does not appear immediately before -llib.  GNUmakefile.preamble must be deleted as it is a generated file, I'm sorry to have missed that.

(3) The Autoconf manual recommends to use <config.h> with the proper -I option, and that it should be included before any other system headers.  Nearly all GNU packages do this, it is perfectly safe.  The rationale behind this recommendation is for predictable results in out-of-tree builds: If you #include "config.h", the preprocessor will search first the directory where the source file is located, IOW srcdir.  If there's a stale config.h there, it will be used instead of the freshly generated config.h (config.status always writes output files in builddir).

Yavor Doganov <yavor>
Sun 31 Dec 2017 09:36:53 PM UTC, comment #3: 

I created a PR at https://github.com/gnustep/apps-gworkspace/pull/2 if you'd like a diff, but I am fine with reviewing here instead (given that GH is nonfree, you might not like using it).

(1) Side remark: Interesting that you're explicitly stating FND_LIBS. I guess it makes sense for libraries.
(2) I like the change in GWMetadata/gmds/gmds/GNUmakefile.preamble to move to ADDITIONAL_TOOL_LIBS:
  ADDITIONAL_TOOL_LIBS +=  -lsqlite3
but I am not sure moving -L flags in  GWMetadata/gmds/gmds/GNUmakefile.preamble.in is correct:
  ADDITIONAL_TOOL_LIBS += @SQLITE_LIB_DIRS@
Also, are these one and the same? It's confusing that @SQLITE_LIB_DIRS@ has a library and not the -L in it. It's also slightly confusing that both GNUmakefile.preamble.in and GNUmakefile.preamble are and were submitted. I think I'll delete the GNUmakefile.preamble.
(3) In OpenOfficeExtractor.m, the inclusion of <config.h> is almost certainly wrong. Is there a system header called config.h? Did you mean local header "config.h"? I'll update this. Same in Resizer.m.

I'll let Riccardo take a look at this too.

Ivan Vučica <ivucica>
Group Member
Sun 31 Dec 2017 09:09:38 PM UTC, comment #2: 

I am very much in favor of applying this, but I'm insufficiently familiar with GWorkspace to say if there is any reason for why it was done this way.

I'll create a PR and send it for review.

Ivan Vučica <ivucica>
Group Member
Sun 26 Nov 2017 07:15:17 PM UTC, comment #1: 

Thinko, I wanted to say "--no-undefined" instead of "--as-needed".

Yavor Doganov <yavor>
Sun 26 Nov 2017 07:11:13 PM UTC, original submission:  

GWorkspace's build system is incredibly convoluted: there are 56 (!) nested configure scripts, 14 config headers and 59 makefiles that are generated, while only a small number of them have @something@ to be AC_SUBST'ed.  I tried to find out whether there is specific reason for this approach, but couldn't figure out why it was done this way.  Perhaps it remained so from Enrico's days and nobody had the chance to clean it up.

The only legitimate reason for using AC_CONFIG_SUBDIRS is when software in sub-directories is distributed as stand-alone packages (e.g., separate self-contained tarballs for the frameworks, extractors, viewers, ddbd and the rest of the tools)  That is not the case and I doubt it ever will be.

The attached patch, while rather intrusive, reduces the time for running "./configure --enable-gwmetadata" more than 10 times and reduces the size of the tarball with approx 2.5 MB.  It also fixes quotation of Autoconf macros in some places, replaces the obsolete AC_TRY_RUN macro with AC_RUN_IFELSE and fixes the build with LDFLAGS=-Wl,--as-needed.

Please let me know if something doesn't work as expected or if you have some other remarks.  Thanks.

Yavor Doganov <yavor>

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attach Files:
   
   
Comment:
   

Attached Files
file #42493:  0001-Simplify-build-system.patch.xz added by yavor (49KiB - application/x-xz)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by CaS (Updated the item)
  • -email is unavailable- added by rmottola (Posted a comment)
  • -email is unavailable- added by ivucica (Posted a comment)
  • -email is unavailable- added by yavor (Submitted the item)
  •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

    Only logged-in users can vote.

     

    Follow 10 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2020-04-14 rmottola StatusReady For Test Fixed
        Open/ClosedIn Test Closed
    2018-02-28 rmottola StatusIn Progress Ready For Test
        Open/ClosedOpen In Test
    2018-01-24 rmottola StatusNone In Progress
    2018-01-08 CaS CategoryBase/Foundation Application
    2018-01-08 CaS CategoryApplication Base/Foundation
    2017-12-31 ivucica Assigned toivucica rmottola
    2017-12-31 ivucica Assigned toNone ivucica
    2017-11-26 yavor Attached File- Added 0001-Simplify-build-system.patch.xz, #42493

    Back to the top

    Powered by Savane 3.13-3230.
    Corresponding source code