List for cgit developers and users
 help / color / mirror / Atom feed
* [PATCH v3 0/4] Makefile improvements
@ 2013-03-06 21:22 john
  2013-03-06 21:22 ` [PATCH v3 1/4] Makefile: re-use Git's Makefile where possible john
                   ` (4 more replies)
  0 siblings, 5 replies; 11+ messages in thread
From: john @ 2013-03-06 21:22 UTC (permalink / raw)


I realised that we already have a global cgit_version so that ui-patch.c
doesn't need to reference the #define'd CGIT_VERSION, meaning that we
can reduce the set of files using that definition to cgit.c.  Patch 2
(new) changes this.

Patch 3 (previously patch 2) is modified to remove ui-patch.c from the
list of files that depend on CGIT_VERSION.

Patch 4 is a new unrelated change to fix the build on systems with
unusual shells in "/bin/sh".

John Keeping (4):
  Makefile: re-use Git's Makefile where possible
  ui-patch: use cgit_version not CGIT_VERSION
  cgit.mk: don't rebuild everything if CGIT_VERSION changes
  cgit.mk: Use SHELL_PATH_SQ to run gen-version.sh

 .gitignore |   1 +
 Makefile   | 124 +++----------------------------------------------------------
 cgit.mk    |  81 ++++++++++++++++++++++++++++++++++++++++
 ui-patch.c |   2 +-
 4 files changed, 88 insertions(+), 120 deletions(-)
 create mode 100644 cgit.mk

-- 
1.8.2.rc2.4.g7799588





^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH v3 1/4] Makefile: re-use Git's Makefile where possible
  2013-03-06 21:22 [PATCH v3 0/4] Makefile improvements john
@ 2013-03-06 21:22 ` john
  2013-03-20 19:33   ` Jason
  2013-03-06 21:22 ` [PATCH v3 2/4] ui-patch: use cgit_version not CGIT_VERSION john
                   ` (3 subsequent siblings)
  4 siblings, 1 reply; 11+ messages in thread
From: john @ 2013-03-06 21:22 UTC (permalink / raw)


Git does quite a lot of platform-specific detection in its Makefile,
which can result in it defining preprocessor variables that are used in
its header files.  If CGit does not define the same variables it can
result in different sizes of some structures in different places in the
same application.

For example, on Solaris Git uses it's "compat" regex library which has a
different sized regex_t structure than that available in the platform
regex.h.  This has a knock-on effect on the size of "struct rev_info"
and leads to hard to diagnose runtime issues.

In order to avoid all of this, introduce a "cgit.mk" file that includes
Git's Makefile and make all of the existing logic apply to CGit's
objects as well.  This is slightly complicated because Git's Makefile
must run in Git's directory, so all references to CGit files need to be
prefixed with "../".

In addition, OBJECTS is a simply expanded variable in Git's Makefile so
we cannot just add our objects to it.  Instead we must copy the two
applicable rules into "cgit.mk".  This has the advantage that we can
split CGit-specific CFLAGS from Git's CFLAGS and hence avoid rebuilding
all of Git whenever a CGit-specific value changes.

Signed-off-by: John Keeping <john at keeping.me.uk>
Acked-by: Jamie Couture <jamie.couture at gmail.com>
---
Unchanged since v2.

 .gitignore |   1 +
 Makefile   | 124 +++----------------------------------------------------------
 cgit.mk    |  74 ++++++++++++++++++++++++++++++++++++
 3 files changed, 80 insertions(+), 119 deletions(-)
 create mode 100644 cgit.mk

diff --git a/.gitignore b/.gitignore
index 487728b..8011b39 100644
--- a/.gitignore
+++ b/.gitignore
@@ -1,6 +1,7 @@
 # Files I don't care to see in git-status/commit
 cgit
 cgit.conf
+CGIT-CFLAGS
 VERSION
 cgitrc.5
 cgitrc.5.fo
diff --git a/Makefile b/Makefile
index 1127961..8c00190 100644
--- a/Makefile
+++ b/Makefile
@@ -23,13 +23,6 @@ DOC_MAN5 = $(patsubst %.txt,%,$(MAN5_TXT))
 DOC_HTML = $(patsubst %.txt,%.html,$(MAN_TXT))
 DOC_PDF  = $(patsubst %.txt,%.pdf,$(MAN_TXT))
 
-# Define NO_STRCASESTR if you don't have strcasestr.
-#
-# Define NO_OPENSSL to disable linking with OpenSSL and use bundled SHA1
-# implementation (slower).
-#
-# Define NEEDS_LIBICONV if linking with libc is not enough (eg. Darwin).
-#
 # Define NO_C99_FORMAT if your formatted IO functions (printf/scanf et.al.)
 # do not support the 'size specifiers' introduced by C99, namely ll, hh,
 # j, z, t. (representing long long int, char, intmax_t, size_t, ptrdiff_t).
@@ -39,22 +32,12 @@ DOC_PDF  = $(patsubst %.txt,%.pdf,$(MAN_TXT))
 #-include config.mak
 
 #
-# Platform specific tweaks
-#
-
-VERSION: force-version
-	@./gen-version.sh "$(CGIT_VERSION)"
--include VERSION
-
-uname_S := $(shell sh -c 'uname -s 2>/dev/null || echo not')
-uname_O := $(shell sh -c 'uname -o 2>/dev/null || echo not')
-uname_R := $(shell sh -c 'uname -r 2>/dev/null || echo not')
-
-#
 # Let the user override the above settings.
 #
 -include cgit.conf
 
+export CGIT_SCRIPT_NAME CGIT_SCRIPT_PATH CGIT_DATA_PATH CGIT_CONFIG CACHE_ROOT
+
 #
 # Define a way to invoke make in subdirs quietly, shamelessly ripped
 # from git.git
@@ -69,8 +52,6 @@ NO_SUBDIR = :
 endif
 
 ifndef V
-	QUIET_CC       = @echo '   ' CC $@;
-	QUIET_LINK     = @echo '   ' LINK $@;
 	QUIET_SUBDIR0  = + at subdir=
 	QUIET_SUBDIR1  = ;$(NO_SUBDIR) echo '   ' SUBDIR $$subdir; \
 			 $(MAKE) $(PRINT_DIR) -C $$subdir
@@ -78,107 +59,12 @@ ifndef V
 	export V
 endif
 
-LDFLAGS ?=
-CFLAGS ?= -g -Wall
-CFLAGS += -Igit
-CFLAGS += -DSHA1_HEADER='$(SHA1_HEADER)'
-CFLAGS += -DCGIT_VERSION='"$(CGIT_VERSION)"'
-CFLAGS += -DCGIT_CONFIG='"$(CGIT_CONFIG)"'
-CFLAGS += -DCGIT_SCRIPT_NAME='"$(CGIT_SCRIPT_NAME)"'
-CFLAGS += -DCGIT_CACHE_ROOT='"$(CACHE_ROOT)"'
-
-ifeq ($(uname_O),Cygwin)
-	NO_STRCASESTR = YesPlease
-	NEEDS_LIBICONV = YesPlease
-endif
-
-ifeq ($(uname_S),$(filter $(uname_S),FreeBSD OpenBSD))
-	# Apparantly libiconv is installed in /usr/local on BSD
-	LDFLAGS += -L/usr/local/lib
-	CFLAGS += -I/usr/local/include
-	NEEDS_LIBICONV = yes
-endif
-
-GIT_OPTIONS = prefix=/usr NO_GETTEXT=1
-OBJECTS =
-
-ifdef NO_ICONV
-	CFLAGS += -DNO_ICONV
-endif
-ifdef NO_STRCASESTR
-	CFLAGS += -DNO_STRCASESTR
-endif
-ifdef NO_C99_FORMAT
-	CFLAGS += -DNO_C99_FORMAT
-endif
-ifdef NO_OPENSSL
-	CFLAGS += -DNO_OPENSSL
-	GIT_OPTIONS += NO_OPENSSL=1
-else
-	LDLIBS += -lcrypto
-endif
-
-ifdef NEEDS_LIBICONV
-	LDLIBS += -liconv
-endif
-
-LDLIBS += git/libgit.a git/xdiff/lib.a -lz -lpthread
-
-OBJECTS += cgit.o
-OBJECTS += cache.o
-OBJECTS += cmd.o
-OBJECTS += configfile.o
-OBJECTS += html.o
-OBJECTS += parsing.o
-OBJECTS += scan-tree.o
-OBJECTS += shared.o
-OBJECTS += ui-atom.o
-OBJECTS += ui-blob.o
-OBJECTS += ui-clone.o
-OBJECTS += ui-commit.o
-OBJECTS += ui-diff.o
-OBJECTS += ui-log.o
-OBJECTS += ui-patch.o
-OBJECTS += ui-plain.o
-OBJECTS += ui-refs.o
-OBJECTS += ui-repolist.o
-OBJECTS += ui-shared.o
-OBJECTS += ui-snapshot.o
-OBJECTS += ui-ssdiff.o
-OBJECTS += ui-stats.o
-OBJECTS += ui-summary.o
-OBJECTS += ui-tag.o
-OBJECTS += ui-tree.o
-OBJECTS += vector.o
-
-dep_files := $(foreach f,$(OBJECTS),$(dir $f).deps/$(notdir $f).d)
-dep_dirs := $(addsuffix .deps,$(sort $(dir $OBJECTS)))
-
-$(dep_dirs):
-	@mkdir -p $@
-
-missing_dep_dirs := $(filter-out $(wildcard $(dep_dirs)),$(dep_dirs))
-dep_file = $(dir $@).deps/$(notdir $@).d
-dep_args = -MF $(dep_file) -MMD -MP
-
 .SUFFIXES:
 
-$(OBJECTS): %.o: %.c $(missing_dep_dirs)
-	$(QUIET_CC)$(CC) -o $*.o -c $(dep_args) $(CFLAGS) $<
-
-dep_files_present := $(wildcard $(dep_files))
-ifneq ($(dep_files_present),)
-include $(dep_files_present)
-endif
-
 all:: cgit
 
-cgit: VERSION $(OBJECTS) libgit
-	$(QUIET_LINK)$(CC) $(CFLAGS) $(LDFLAGS) -o $@ $(OBJECTS) $(LDLIBS)
-
-libgit:
-	$(QUIET_SUBDIR0)git $(QUIET_SUBDIR1) NO_CURL=1 $(GIT_OPTIONS) libgit.a
-	$(QUIET_SUBDIR0)git $(QUIET_SUBDIR1) NO_CURL=1 $(GIT_OPTIONS) xdiff/lib.a
+cgit:
+	$(QUIET_SUBDIR0)git $(QUIET_SUBDIR1) -f ../cgit.mk ../cgit NO_CURL=1
 
 test: all
 	$(QUIET_SUBDIR0)tests $(QUIET_SUBDIR1) all
@@ -259,7 +145,7 @@ get-git:
 tags:
 	$(QUIET_TAGS)find . -name '*.[ch]' | xargs ctags
 
-.PHONY: all cgit get-git libgit force-version
+.PHONY: all cgit get-git
 .PHONY: clean clean-doc cleanall
 .PHONY: doc doc-html doc-man doc-pdf
 .PHONY: install install-doc install-html install-man install-pdf
diff --git a/cgit.mk b/cgit.mk
new file mode 100644
index 0000000..4869c55
--- /dev/null
+++ b/cgit.mk
@@ -0,0 +1,74 @@
+# This Makefile is run in the "git" directory in order to re-use Git's
+# build variables and operating system detection.  Hence all files in
+# CGit's directory must be prefixed with "../".
+include Makefile
+
+CGIT_PREFIX = ../
+
+# The CGIT_* variables are inherited when this file is called from the
+# main Makefile - they are defined there.
+
+$(CGIT_PREFIX)VERSION: force-version
+	@cd $(CGIT_PREFIX) && ./gen-version.sh "$(CGIT_VERSION)"
+-include $(CGIT_PREFIX)VERSION
+.PHONY: force-version
+
+# CGIT_CFLAGS is a separate variable so that we can track it separately
+# and avoid rebuilding all of Git when these variables change.
+CGIT_CFLAGS += -DCGIT_VERSION='"$(CGIT_VERSION)"'
+CGIT_CFLAGS += -DCGIT_CONFIG='"$(CGIT_CONFIG)"'
+CGIT_CFLAGS += -DCGIT_SCRIPT_NAME='"$(CGIT_SCRIPT_NAME)"'
+CGIT_CFLAGS += -DCGIT_CACHE_ROOT='"$(CACHE_ROOT)"'
+
+ifdef NO_C99_FORMAT
+	CFLAGS += -DNO_C99_FORMAT
+endif
+
+CGIT_OBJ_NAMES += cgit.o
+CGIT_OBJ_NAMES += cache.o
+CGIT_OBJ_NAMES += cmd.o
+CGIT_OBJ_NAMES += configfile.o
+CGIT_OBJ_NAMES += html.o
+CGIT_OBJ_NAMES += parsing.o
+CGIT_OBJ_NAMES += scan-tree.o
+CGIT_OBJ_NAMES += shared.o
+CGIT_OBJ_NAMES += ui-atom.o
+CGIT_OBJ_NAMES += ui-blob.o
+CGIT_OBJ_NAMES += ui-clone.o
+CGIT_OBJ_NAMES += ui-commit.o
+CGIT_OBJ_NAMES += ui-diff.o
+CGIT_OBJ_NAMES += ui-log.o
+CGIT_OBJ_NAMES += ui-patch.o
+CGIT_OBJ_NAMES += ui-plain.o
+CGIT_OBJ_NAMES += ui-refs.o
+CGIT_OBJ_NAMES += ui-repolist.o
+CGIT_OBJ_NAMES += ui-shared.o
+CGIT_OBJ_NAMES += ui-snapshot.o
+CGIT_OBJ_NAMES += ui-ssdiff.o
+CGIT_OBJ_NAMES += ui-stats.o
+CGIT_OBJ_NAMES += ui-summary.o
+CGIT_OBJ_NAMES += ui-tag.o
+CGIT_OBJ_NAMES += ui-tree.o
+CGIT_OBJ_NAMES += vector.o
+
+CGIT_OBJS := $(addprefix $(CGIT_PREFIX),$(CGIT_OBJ_NAMES))
+
+ifeq ($(wildcard $(CGIT_PREFIX).depend),)
+missing_dep_dirs += $(CGIT_PREFIX).depend
+endif
+
+$(CGIT_PREFIX).depend:
+	@mkdir -p $@
+
+$(CGIT_PREFIX)CGIT-CFLAGS: FORCE
+	@FLAGS='$(subst ','\'',$(CGIT_CFLAGS))'; \
+	    if test x"$$FLAGS" != x"`cat ../CGIT-CFLAGS 2>/dev/null`" ; then \
+		echo 1>&2 "    * new CGit build flags"; \
+		echo "$$FLAGS" >$(CGIT_PREFIX)CGIT-CFLAGS; \
+            fi
+
+$(CGIT_OBJS): %.o: %.c GIT-CFLAGS $(CGIT_PREFIX)CGIT-CFLAGS $(missing_dep_dirs)
+	$(QUIET_CC)$(CC) -o $*.o -c $(dep_args) $(ALL_CFLAGS) $(EXTRA_CPPFLAGS) $(CGIT_CFLAGS) $<
+
+$(CGIT_PREFIX)cgit: $(CGIT_OBJS) GIT-LDFLAGS $(GITLIBS)
+	$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) $(LIBS)
-- 
1.8.2.rc2.4.g7799588





^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH v3 2/4] ui-patch: use cgit_version not CGIT_VERSION
  2013-03-06 21:22 [PATCH v3 0/4] Makefile improvements john
  2013-03-06 21:22 ` [PATCH v3 1/4] Makefile: re-use Git's Makefile where possible john
@ 2013-03-06 21:22 ` john
  2013-03-06 21:22 ` [PATCH v3 3/4] cgit.mk: don't rebuild everything if CGIT_VERSION changes john
                   ` (2 subsequent siblings)
  4 siblings, 0 replies; 11+ messages in thread
From: john @ 2013-03-06 21:22 UTC (permalink / raw)


We already have a global cgit_version which is set from the #define'd
CGIT_VERSION in cgit.c.  Change ui-patch.c to use this so that we only
need to rebuild cgit.o when the version changes.

Signed-off-by: John Keeping <john at keeping.me.uk>
---
 ui-patch.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ui-patch.c b/ui-patch.c
index 79bc509..12abe10 100644
--- a/ui-patch.c
+++ b/ui-patch.c
@@ -131,6 +131,6 @@ void cgit_print_patch(char *hex, const char *prefix)
 		htmlf("(limited to '%s')\n\n", prefix);
 	cgit_diff_tree(old_sha1, sha1, filepair_cb, prefix, 0);
 	html("--\n");
-	htmlf("cgit %s\n", CGIT_VERSION);
+	htmlf("cgit %s\n", cgit_version);
 	cgit_free_commitinfo(info);
 }
-- 
1.8.2.rc2.4.g7799588





^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH v3 3/4] cgit.mk: don't rebuild everything if CGIT_VERSION changes
  2013-03-06 21:22 [PATCH v3 0/4] Makefile improvements john
  2013-03-06 21:22 ` [PATCH v3 1/4] Makefile: re-use Git's Makefile where possible john
  2013-03-06 21:22 ` [PATCH v3 2/4] ui-patch: use cgit_version not CGIT_VERSION john
@ 2013-03-06 21:22 ` john
  2013-03-07  8:03   ` 
  2013-03-06 21:22 ` [PATCH v3 4/4] cgit.mk: Use SHELL_PATH_SQ to run gen-version.sh john
  2013-03-20 19:34 ` [PATCH v3 0/4] Makefile improvements Jason
  4 siblings, 1 reply; 11+ messages in thread
From: john @ 2013-03-06 21:22 UTC (permalink / raw)


If CGIT_VERSION is in CGIT_CFLAGS then a change in version (for example
because you have committed your changes) causes all of the CGit objects
to be rebuilt.  Avoid this by using EXTRA_CPPFLAGS to add the version
for only those files that are affected and make them depend on VERSION.

Signed-off-by: John Keeping <john at keeping.me.uk>
---
Changes since v2:

- With the previous patch, ui-patch.c no longer references CGIT_VERSION
  so ui-patch.o is removed from CGIT_VERSION_OBJS.

 cgit.mk | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/cgit.mk b/cgit.mk
index 4869c55..c8ecd3a 100644
--- a/cgit.mk
+++ b/cgit.mk
@@ -15,7 +15,6 @@ $(CGIT_PREFIX)VERSION: force-version
 
 # CGIT_CFLAGS is a separate variable so that we can track it separately
 # and avoid rebuilding all of Git when these variables change.
-CGIT_CFLAGS += -DCGIT_VERSION='"$(CGIT_VERSION)"'
 CGIT_CFLAGS += -DCGIT_CONFIG='"$(CGIT_CONFIG)"'
 CGIT_CFLAGS += -DCGIT_SCRIPT_NAME='"$(CGIT_SCRIPT_NAME)"'
 CGIT_CFLAGS += -DCGIT_CACHE_ROOT='"$(CACHE_ROOT)"'
@@ -53,6 +52,14 @@ CGIT_OBJ_NAMES += vector.o
 
 CGIT_OBJS := $(addprefix $(CGIT_PREFIX),$(CGIT_OBJ_NAMES))
 
+# Only cgit.c reference CGIT_VERSION so we only rebuild its objects when the
+# version changes.
+CGIT_VERSION_OBJS := $(addprefix $(CGIT_PREFIX),cgit.o)
+$(CGIT_VERSION_OBJS): $(CGIT_PREFIX)VERSION
+$(CGIT_VERSION_OBJS): EXTRA_CPPFLAGS = \
+	-DCGIT_VERSION='"$(CGIT_VERSION)"'
+
+
 ifeq ($(wildcard $(CGIT_PREFIX).depend),)
 missing_dep_dirs += $(CGIT_PREFIX).depend
 endif
-- 
1.8.2.rc2.4.g7799588





^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH v3 4/4] cgit.mk: Use SHELL_PATH_SQ to run gen-version.sh
  2013-03-06 21:22 [PATCH v3 0/4] Makefile improvements john
                   ` (2 preceding siblings ...)
  2013-03-06 21:22 ` [PATCH v3 3/4] cgit.mk: don't rebuild everything if CGIT_VERSION changes john
@ 2013-03-06 21:22 ` john
  2013-03-20 19:34 ` [PATCH v3 0/4] Makefile improvements Jason
  4 siblings, 0 replies; 11+ messages in thread
From: john @ 2013-03-06 21:22 UTC (permalink / raw)


On some platforms (notably Solaris) /bin/sh doesn't support enough of
POSIX for gen-version.sh to run.  Git's Makefile provides SHELL_PATH_SQ
to address this issue so we just have to use it.

Signed-off-by: John Keeping <john at keeping.me.uk>
---
 cgit.mk | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/cgit.mk b/cgit.mk
index c8ecd3a..e1aed63 100644
--- a/cgit.mk
+++ b/cgit.mk
@@ -9,7 +9,7 @@ CGIT_PREFIX = ../
 # main Makefile - they are defined there.
 
 $(CGIT_PREFIX)VERSION: force-version
-	@cd $(CGIT_PREFIX) && ./gen-version.sh "$(CGIT_VERSION)"
+	@cd $(CGIT_PREFIX) && '$(SHELL_PATH_SQ)' ./gen-version.sh "$(CGIT_VERSION)"
 -include $(CGIT_PREFIX)VERSION
 .PHONY: force-version
 
-- 
1.8.2.rc2.4.g7799588





^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH v3 3/4] cgit.mk: don't rebuild everything if CGIT_VERSION changes
  2013-03-06 21:22 ` [PATCH v3 3/4] cgit.mk: don't rebuild everything if CGIT_VERSION changes john
@ 2013-03-07  8:03   ` 
  2013-03-07  8:16     ` cgit
  0 siblings, 1 reply; 11+ messages in thread
From:  @ 2013-03-07  8:03 UTC (permalink / raw)


> +# Only cgit.c reference CGIT_VERSION so we only rebuild its objects when the
> +# version changes.
> +CGIT_VERSION_OBJS := $(addprefix $(CGIT_PREFIX),cgit.o)
> +$(CGIT_VERSION_OBJS): $(CGIT_PREFIX)VERSION
> +$(CGIT_VERSION_OBJS): EXTRA_CPPFLAGS = \
> +	-DCGIT_VERSION='"$(CGIT_VERSION)"'

Is the small gain (not rebuilding a couple of small C-files) worth this
hassle? Because it sounds a bit like a maintenance nightmare: One more
place to remember to put a file.

If cgit were some template-C++-monster, this might be useful, but cgit
is not...

- Ren?





^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH v3 3/4] cgit.mk: don't rebuild everything if CGIT_VERSION changes
  2013-03-07  8:03   ` 
@ 2013-03-07  8:16     ` cgit
  2013-03-07  9:07       ` john
  0 siblings, 1 reply; 11+ messages in thread
From: cgit @ 2013-03-07  8:16 UTC (permalink / raw)


On Thu, Mar 07, 2013 at 09:03:11AM +0100, Ren? Neumann wrote:
> > +# Only cgit.c reference CGIT_VERSION so we only rebuild its objects when the
> > +# version changes.
> > +CGIT_VERSION_OBJS := $(addprefix $(CGIT_PREFIX),cgit.o)
> > +$(CGIT_VERSION_OBJS): $(CGIT_PREFIX)VERSION
> > +$(CGIT_VERSION_OBJS): EXTRA_CPPFLAGS = \
> > +	-DCGIT_VERSION='"$(CGIT_VERSION)"'
> 
> Is the small gain (not rebuilding a couple of small C-files) worth this
> hassle? Because it sounds a bit like a maintenance nightmare: One more
> place to remember to put a file.
> 
> If cgit were some template-C++-monster, this might be useful, but cgit
> is not...

I don't think any other file should reference CGIT_VERSION in the
future, see patch 2/4 in this patch set.

> 
> - Ren?
> 
> 
> _______________________________________________
> cgit mailing list
> cgit at hjemli.net
> http://hjemli.net/mailman/listinfo/cgit




^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH v3 3/4] cgit.mk: don't rebuild everything if CGIT_VERSION changes
  2013-03-07  8:16     ` cgit
@ 2013-03-07  9:07       ` john
  2013-03-07  9:49         ` 
  0 siblings, 1 reply; 11+ messages in thread
From: john @ 2013-03-07  9:07 UTC (permalink / raw)


On Thu, Mar 07, 2013 at 09:16:50AM +0100, Lukas Fleischer wrote:
> On Thu, Mar 07, 2013 at 09:03:11AM +0100, Ren? Neumann wrote:
> > > +# Only cgit.c reference CGIT_VERSION so we only rebuild its objects when the
> > > +# version changes.
> > > +CGIT_VERSION_OBJS := $(addprefix $(CGIT_PREFIX),cgit.o)
> > > +$(CGIT_VERSION_OBJS): $(CGIT_PREFIX)VERSION
> > > +$(CGIT_VERSION_OBJS): EXTRA_CPPFLAGS = \
> > > +	-DCGIT_VERSION='"$(CGIT_VERSION)"'
> > 
> > Is the small gain (not rebuilding a couple of small C-files) worth this
> > hassle? Because it sounds a bit like a maintenance nightmare: One more
> > place to remember to put a file.

It's not a couple of C files, it's *all* of the CGit C files.

> > If cgit were some template-C++-monster, this might be useful, but cgit
> > is not...
> 
> I don't think any other file should reference CGIT_VERSION in the
> future, see patch 2/4 in this patch set.

Exactly.  We shouldn't ever need to use the #define'd CGIT_VERSION in
more than one place.


John




^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH v3 3/4] cgit.mk: don't rebuild everything if CGIT_VERSION changes
  2013-03-07  9:07       ` john
@ 2013-03-07  9:49         ` 
  0 siblings, 0 replies; 11+ messages in thread
From:  @ 2013-03-07  9:49 UTC (permalink / raw)


Am 07.03.2013 10:07, schrieb John Keeping:
> On Thu, Mar 07, 2013 at 09:16:50AM +0100, Lukas Fleischer wrote:
>> On Thu, Mar 07, 2013 at 09:03:11AM +0100, Ren? Neumann wrote:
>>>> +# Only cgit.c reference CGIT_VERSION so we only rebuild its objects when the
>>>> +# version changes.
>>>> +CGIT_VERSION_OBJS := $(addprefix $(CGIT_PREFIX),cgit.o)
>>>> +$(CGIT_VERSION_OBJS): $(CGIT_PREFIX)VERSION
>>>> +$(CGIT_VERSION_OBJS): EXTRA_CPPFLAGS = \
>>>> +	-DCGIT_VERSION='"$(CGIT_VERSION)"'
>>>
>>> Is the small gain (not rebuilding a couple of small C-files) worth this
>>> hassle? Because it sounds a bit like a maintenance nightmare: One more
>>> place to remember to put a file.
> 
> It's not a couple of C files, it's *all* of the CGit C files.

Well, in my perspective, *all* of CGit C files _is_ 'a couple'.

> Exactly.  We shouldn't ever need to use the #define'd CGIT_VERSION in
> more than one place.

That's a fair point.

- Ren?




^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH v3 1/4] Makefile: re-use Git's Makefile where possible
  2013-03-06 21:22 ` [PATCH v3 1/4] Makefile: re-use Git's Makefile where possible john
@ 2013-03-20 19:33   ` Jason
  0 siblings, 0 replies; 11+ messages in thread
From: Jason @ 2013-03-20 19:33 UTC (permalink / raw)


On Wed, Mar 6, 2013 at 10:22 PM, John Keeping <john at keeping.me.uk> wrote:
> +$(CGIT_PREFIX)CGIT-CFLAGS: FORCE
> +       @FLAGS='$(subst ','\'',$(CGIT_CFLAGS))'; \
> +           if test x"$$FLAGS" != x"`cat ../CGIT-CFLAGS 2>/dev/null`" ; then \
> +               echo 1>&2 "    * new CGit build flags"; \
> +               echo "$$FLAGS" >$(CGIT_PREFIX)CGIT-CFLAGS; \
> +            fi
> +

I understand the desire to track cflags changes and only rebuild
certain parts, etc, but the notion of creating an extra file for this
isn't too nice. I suppose this is the only way to do it though.




^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH v3 0/4] Makefile improvements
  2013-03-06 21:22 [PATCH v3 0/4] Makefile improvements john
                   ` (3 preceding siblings ...)
  2013-03-06 21:22 ` [PATCH v3 4/4] cgit.mk: Use SHELL_PATH_SQ to run gen-version.sh john
@ 2013-03-20 19:34 ` Jason
  4 siblings, 0 replies; 11+ messages in thread
From: Jason @ 2013-03-20 19:34 UTC (permalink / raw)


Merged to WIP for testing.




^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2013-03-20 19:34 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-03-06 21:22 [PATCH v3 0/4] Makefile improvements john
2013-03-06 21:22 ` [PATCH v3 1/4] Makefile: re-use Git's Makefile where possible john
2013-03-20 19:33   ` Jason
2013-03-06 21:22 ` [PATCH v3 2/4] ui-patch: use cgit_version not CGIT_VERSION john
2013-03-06 21:22 ` [PATCH v3 3/4] cgit.mk: don't rebuild everything if CGIT_VERSION changes john
2013-03-07  8:03   ` 
2013-03-07  8:16     ` cgit
2013-03-07  9:07       ` john
2013-03-07  9:49         ` 
2013-03-06 21:22 ` [PATCH v3 4/4] cgit.mk: Use SHELL_PATH_SQ to run gen-version.sh john
2013-03-20 19:34 ` [PATCH v3 0/4] Makefile improvements Jason

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).