After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 747699 - some ide-builder patches
some ide-builder patches
Status: RESOLVED FIXED
Product: gnome-builder
Classification: Other
Component: general
unspecified
Other Linux
: Normal normal
: ---
Assigned To: GNOME Builder Maintainers
GNOME Builder Maintainers
Depends on:
Blocks:
 
 
Reported: 2015-04-11 13:15 UTC by Paolo Borelli
Modified: 2015-04-11 23:07 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
patch (848 bytes, patch)
2015-04-11 13:16 UTC, Paolo Borelli
accepted-commit_now Details | Review
patch 2 (5.60 KB, patch)
2015-04-11 13:22 UTC, Paolo Borelli
accepted-commit_now Details | Review
patch 3 (2.30 KB, patch)
2015-04-11 13:25 UTC, Paolo Borelli
accepted-commit_now Details | Review
patch 4 (665 bytes, patch)
2015-04-11 18:39 UTC, sébastien lafargue
committed Details | Review

Description Paolo Borelli 2015-04-11 13:15:24 UTC
I started looking at this code and poked around a bit. Feel free to apply or discard the following patches.
Comment 1 Paolo Borelli 2015-04-11 13:16:00 UTC
Created attachment 301361 [details] [review]
patch

trivial typo
Comment 2 Paolo Borelli 2015-04-11 13:22:13 UTC
Created attachment 301362 [details] [review]
patch 2

This fixes the todo in the workbench about it generic and pushing down autotools specific implementation details to the concrete builder.

I am not sure this goes in the direction you have in mind since it adds a flags argument while your approach was to pass down a free form "config" object.

Clearly the generic config object is much more flexible and an alternative would be to set the "force-rebuild" in the config object but in a section more generic than "autotools", however since "force rebuild" is something that will be shared among all kind of builders and even reflected in the UI, I think making it explicit in "flags" could work.
Comment 3 Paolo Borelli 2015-04-11 13:25:31 UTC
Created attachment 301363 [details] [review]
patch 3

Remove boiler plate from ide-builder.c and make abstract. Just removing the empty set/get_prop, finalize etc that looks like generated from a snippet. Not sure if it is worth it, since I guess you have plenty of this stuff around, but seems unlikely that this base class need then any time soon.

Maybe slightly more interesting, declare the type abstract since there is not even a default implementation of build_async
Comment 4 Christian Hergert 2015-04-11 17:20:45 UTC
Review of attachment 301361 [details] [review]:

Perfect example of why not to use a generic container for everything :)
Comment 5 Christian Hergert 2015-04-11 17:22:22 UTC
Review of attachment 301362 [details] [review]:

LGTM. ide-build.c was such a hack. If we are going to end up installing it with bin_PROGRAMS someday, it really should be cleaned up.
Comment 6 Christian Hergert 2015-04-11 17:22:45 UTC
Review of attachment 301363 [details] [review]:

LGTM
Comment 7 sébastien lafargue 2015-04-11 18:39:03 UTC
Created attachment 301377 [details] [review]
patch 4
Comment 8 sébastien lafargue 2015-04-11 19:24:46 UTC
Review of attachment 301377 [details] [review]:

fine, seen on irc with Christian.
Comment 9 sébastien lafargue 2015-04-11 19:30:30 UTC
Review of attachment 301377 [details] [review]:

commited as b65749b
Comment 10 Paolo Borelli 2015-04-11 23:07:09 UTC
pushed the patches