Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(123)

Issue 51730044: data/dconf: Don't run "dconf update" if $(DESTDIR) is set (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
6 years, 10 months ago by fujiwara
Modified:
6 years, 10 months ago
Reviewers:
shawn.p.huang, Peng, cgwalters
CC:
shawn.p.huang_gmail.com, fujiwara, walters_verbum.org
Base URL:
git://github.com/ibus/ibus.git@master
Visibility:
Public.

Description

data/dconf: Don't run "dconf update" if $(DESTDIR) is set dconf changed as of https://git.gnome.org/browse/dconf/commit/?id=c211fc46496597c7ddabd73d623bae4037754916 to actually emit an error if /etc/dconf/db is empty. When building ibus in a system such as dpkg/rpm or gnome-continuous, there may actually be nothing in that directory in the buildroot. This will now cause "dconf update" as executed by this Makefile to fail. The fix is to just check $(DESTDIR), like we should do for all triggers (e.g. gtk-update-icon-cache too). It's never useful to execute these from per-component Makefiles if $(DESTDIR) is set. Instead, these meta-build systems (dpkg/rpm/jhbuild/Continuous) all take care of execution of triggers on their own.

Patch Set 1 #

Total comments: 1
Unified diffs Side-by-side diffs Delta from patch set Stats (+3 lines, -1 line) Patch
M data/dconf/Makefile.am View 1 chunk +3 lines, -1 line 1 comment Download

Messages

Total messages: 4
fujiwara
6 years, 10 months ago (2014-01-15 02:19:06 UTC) #1
Peng
https://codereview.appspot.com/51730044/diff/1/data/dconf/Makefile.am File data/dconf/Makefile.am (right): https://codereview.appspot.com/51730044/diff/1/data/dconf/Makefile.am#newcode64 data/dconf/Makefile.am:64: if test -z "$(DESTDIR)"; then \ What about if ...
6 years, 10 months ago (2014-01-15 13:04:48 UTC) #2
cgwalters
On 2014/01/15 13:04:48, Peng wrote: > https://codereview.appspot.com/51730044/diff/1/data/dconf/Makefile.am > File data/dconf/Makefile.am (right): > > https://codereview.appspot.com/51730044/diff/1/data/dconf/Makefile.am#newcode64 > ...
6 years, 10 months ago (2014-01-15 14:13:56 UTC) #3
Peng
6 years, 10 months ago (2014-01-16 02:26:38 UTC) #4
On 2014/01/15 14:13:56, cgwalters wrote:
> On 2014/01/15 13:04:48, Peng wrote:
> > https://codereview.appspot.com/51730044/diff/1/data/dconf/Makefile.am
> > File data/dconf/Makefile.am (right):
> > 
> >
>
https://codereview.appspot.com/51730044/diff/1/data/dconf/Makefile.am#newcode64
> > data/dconf/Makefile.am:64: if test -z "$(DESTDIR)"; then \
> > What about if the db exists in destdir?
> 
> No one does that.  DESTDIR is for package systems to gather the files
> for an individual component.  This patch works exactly how
> many other real-world Makefiles do. 
> 
> For example:
> https://git.gnome.org/browse/librsvg/tree/gdk-pixbuf-loader/Makefile.am#n52

I see. lgtm.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b