git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [RFC PATCH v2 0/7] Introduce Git Standard Library

From
PWPhillip Wood <phillip.wood123@gmail.com>
Date
Aug 15, 2023, 09:41 UTC
Message-ID
<a0f04bd7-3a1e-b303-fd52-eee2af4d38b3@gmail.com>
In-Reply-To
<20230810163346.274132-1-calvinwan@google.com>
Hi Calvin
On 10/08/2023 17:33, Calvin Wan wrote:
Show 16 quoted lines
> Original cover letter:
> https://lore.kernel.org/git/20230627195251.1973421-1-calvinwan@google.com/
> 
> In the initial RFC, I had a patch that removed the trace2 dependency
> from usage.c so that git-std-lib.a would not have dependencies outside
> of git-std-lib.a files. Consequently this meant that tracing would not
> be possible in git-std-lib.a files for other developers of Git, and it
> is not a good idea for the libification effort to close the door on
> tracing in certain files for future development (thanks Victoria for
> pointing this out). That patch has been removed and instead I introduce
> stubbed out versions of repository.[ch] and trace2.[ch] that are swapped
> in during compilation time (I'm no Makefile expert so any advice on how
> on I could do this better would be much appreciated). These stubbed out
> files contain no implementations and therefore do not have any
> additional dependencies, allowing git-std-lib.a to compile with only the
> stubs as additional dependencies.

I think stubbing out trace2 is a sensible approach. I don't think we need separate headers when using the stub though, or a stub for repository.c as we don't call any of the functions declared in that header. I've appended a patch that shows a simplified stub. It also removes the recursive make call as it no-longer needs to juggle the header files.

Show 5 quoted lines
> This also has the added benefit of
> removing `#ifdef GIT_STD_LIB` macros in C files for specific library
> compilation rules. Libification shouldn't pollute C files with these
> macros. The boundaries for git-std-lib.a have also been updated to
> contain these stubbed out files.
Do you have any plans to support building with gettext support so we
can use git-std-lib.a as a dependency of libgit.a?
Show 7 quoted lines
> I have also made some additional changes to the Makefile to piggy back
> off of our existing build rules for .c/.o targets and their
> dependencies. As I learn more about Makefiles, I am continuing to look
> for ways to improve these rules. Eventually I would like to be able to
> have a set of rules that future libraries can emulate and is scalable
> in the sense of not creating additional toil for developers that are not
> interested in libification.

I'm not sure reusing LIB_OBJS for different targets is a good idea. Once libgit.a starts to depend on git-std-lib.a we'll want to build them both with a single make invocation without resorting to recursive make calls. I think we could perhaps make a template function to create the compilation rules for each library - see the end of https://wingolog.org/archives/2023/08/08/a-negative-result

Best Wishes
Phillip
---- >8 -----
 From 194403e42f116cc3c6ed8eb8b03d6933b24067e4 Mon Sep 17 00:00:00 2001
From: Phillip Wood <phillip.wood@dunelm.org.uk>
Date: Sat, 12 Aug 2023 17:27:23 +0100
Subject: [PATCH] git-std-lib: simplify sub implementation

The code in std-lib does not depend directly on the functions declared in repository.h and so it does not need to provide stub implementations of the functions declared in repository.h. There is a transitive dependency on `struct repository` from the functions declared in trace2.h but the stub implementation of those functions can simply define its own stub for struct repository. There is also no need to use different headers when compiling against the stub implementation of trace2.

This means we can simplify the stub implementation by removing stubs/{repository.[ch],trace2.h} and simplify the Makefile by removing the code that replaces header files when compiling against the trace2 stub. git-std-lib.a can now be built by running

   make git-std-lib.a GIT_STD_LIB=YesPlease STUB_TRACE2=YesPlease
There is one other small fixup in this commit:
  - `wrapper.c` includes `repository.h` but does not use any of the
    declarations.
Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
  Makefile           | 29 +-------------------
  stubs/repository.c |  4 ---
  stubs/repository.h |  8 ------
  stubs/trace2.c     |  5 ++++
  stubs/trace2.h     | 68 ----------------------------------------------
  wrapper.c          |  1 -
  6 files changed, 6 insertions(+), 109 deletions(-)
  delete mode 100644 stubs/repository.c
  delete mode 100644 stubs/repository.h
  delete mode 100644 stubs/trace2.h
diff --git a/Makefile b/Makefile
index a821d73c9d0..8eff4021025 100644
--- a/Makefile
+++ b/Makefile
@@ -1209,10 +1209,6 @@ LIB_OBJS += usage.o
  LIB_OBJS += utf8.o
  LIB_OBJS += wrapper.o
  
-ifdef STUB_REPOSITORY
-STUB_OBJS += stubs/repository.o
-endif
-
  ifdef STUB_TRACE2
  STUB_OBJS += stubs/trace2.o
  endif
@@ -3866,31 +3862,8 @@ fuzz-all: $(FUZZ_PROGRAMS)
  ### Libified Git rules
  
  # git-std-lib
-# `make git-std-lib GIT_STD_LIB=YesPlease STUB_REPOSITORY=YesPlease STUB_TRACE2=YesPlease`
+# `make git-std-lib.a GIT_STD_LIB=YesPlease STUB_TRACE2=YesPlease`
  STD_LIB = git-std-lib.a
  
  $(STD_LIB): $(LIB_OBJS) $(COMPAT_OBJS) $(STUB_OBJS)
  	$(QUIET_AR)$(RM) $@ && $(AR) $(ARFLAGS) $@ $^
-
-TEMP_HEADERS = temp_headers/
-
-git-std-lib:
-# Move headers to temporary folder and replace them with stubbed headers.
-# After building, move headers and stubbed headers back.
-ifneq ($(STUB_OBJS),)
-	mkdir -p $(TEMP_HEADERS); \
-	for d in $(STUB_OBJS); do \
-		BASE=$${d%.*}; \
-		mv $${BASE##*/}.h $(TEMP_HEADERS)$${BASE##*/}.h; \
-		mv $${BASE}.h $${BASE##*/}.h; \
-	done; \
-	$(MAKE) $(STD_LIB); \
-	for d in $(STUB_OBJS); do \
-		BASE=$${d%.*}; \
-		mv $${BASE##*/}.h $${BASE}.h; \
-		mv $(TEMP_HEADERS)$${BASE##*/}.h $${BASE##*/}.h; \
-	done; \
-	rm -rf temp_headers
-else
-	$(MAKE) $(STD_LIB)
-endif
diff --git a/stubs/repository.c b/stubs/repository.c
deleted file mode 100644
index f81520d083a..00000000000
--- a/stubs/repository.c
+++ /dev/null
@@ -1,4 +0,0 @@
-#include "git-compat-util.h"
-#include "repository.h"
-
-struct repository *the_repository;
diff --git a/stubs/repository.h b/stubs/repository.h
deleted file mode 100644
index 18262d748e5..00000000000
--- a/stubs/repository.h
+++ /dev/null
@@ -1,8 +0,0 @@
-#ifndef REPOSITORY_H
-#define REPOSITORY_H
-
-struct repository { int stub; };
-
-extern struct repository *the_repository;
-
-#endif /* REPOSITORY_H */
diff --git a/stubs/trace2.c b/stubs/trace2.c
index efc3f9c1f39..7d894822288 100644
--- a/stubs/trace2.c
+++ b/stubs/trace2.c
@@ -1,6 +1,10 @@
  #include "git-compat-util.h"
  #include "trace2.h"
  
+struct child_process { int stub; };
+struct repository { int stub; };
+struct json_writer { int stub; };
+
  void trace2_region_enter_fl(const char *file, int line, const char *category,
  			    const char *label, const struct repository *repo, ...) { }
  void trace2_region_leave_fl(const char *file, int line, const char *category,
@@ -19,4 +23,5 @@ void trace2_data_intmax_fl(const char *file, int line, const char *category,
  			   const struct repository *repo, const char *key,
  			   intmax_t value) { }
  int trace2_is_enabled(void) { return 0; }
+void trace2_counter_add(enum trace2_counter_id cid, uint64_t value) { }
  void trace2_collect_process_info(enum trace2_process_info_reason reason) { }
diff --git a/stubs/trace2.h b/stubs/trace2.h
deleted file mode 100644
index 836a14797cc..00000000000
--- a/stubs/trace2.h
+++ /dev/null
@@ -1,68 +0,0 @@
-#ifndef TRACE2_H
-#define TRACE2_H
-
-struct child_process { int stub; };
-struct repository;
-struct json_writer { int stub; };
-
-void trace2_region_enter_fl(const char *file, int line, const char *category,
-			    const char *label, const struct repository *repo, ...);
-
-#define trace2_region_enter(category, label, repo) \
-	trace2_region_enter_fl(__FILE__, __LINE__, (category), (label), (repo))
-
-void trace2_region_leave_fl(const char *file, int line, const char *category,
-			    const char *label, const struct repository *repo, ...);
-
-#define trace2_region_leave(category, label, repo) \
-	trace2_region_leave_fl(__FILE__, __LINE__, (category), (label), (repo))
-
-void trace2_data_string_fl(const char *file, int line, const char *category,
-			   const struct repository *repo, const char *key,
-			   const char *value);
-
-#define trace2_data_string(category, repo, key, value)                       \
-	trace2_data_string_fl(__FILE__, __LINE__, (category), (repo), (key), \
-			      (value))
-
-void trace2_cmd_ancestry_fl(const char *file, int line, const char **parent_names);
-
-#define trace2_cmd_ancestry(v) trace2_cmd_ancestry_fl(__FILE__, __LINE__, (v))
-
-void trace2_cmd_error_va_fl(const char *file, int line, const char *fmt,
-			    va_list ap);
-
-#define trace2_cmd_error_va(fmt, ap) \
-	trace2_cmd_error_va_fl(__FILE__, __LINE__, (fmt), (ap))
-
-
-void trace2_cmd_name_fl(const char *file, int line, const char *name);
-
-#define trace2_cmd_name(v) trace2_cmd_name_fl(__FILE__, __LINE__, (v))
-
-void trace2_thread_start_fl(const char *file, int line,
-			    const char *thread_base_name);
-
-#define trace2_thread_start(thread_base_name) \
-	trace2_thread_start_fl(__FILE__, __LINE__, (thread_base_name))
-
-void trace2_thread_exit_fl(const char *file, int line);
-
-#define trace2_thread_exit() trace2_thread_exit_fl(__FILE__, __LINE__)
-
-void trace2_data_intmax_fl(const char *file, int line, const char *category,
-			   const struct repository *repo, const char *key,
-			   intmax_t value);
-
-#define trace2_data_intmax(category, repo, key, value)                       \
-	trace2_data_intmax_fl(__FILE__, __LINE__, (category), (repo), (key), \
-			      (value))
-
-enum trace2_process_info_reason {
-	TRACE2_PROCESS_INFO_STARTUP,
-	TRACE2_PROCESS_INFO_EXIT,
-};
-int trace2_is_enabled(void);
-void trace2_collect_process_info(enum trace2_process_info_reason reason);
-
-#endif /* TRACE2_H */
diff --git a/wrapper.c b/wrapper.c
index 9eae4a8b3a0..e6facc5ff0c 100644
--- a/wrapper.c
+++ b/wrapper.c
@@ -5,7 +5,6 @@
  #include "abspath.h"
  #include "parse.h"
  #include "gettext.h"
-#include "repository.h"
  #include "strbuf.h"
  #include "trace2.h"
  

-- 
2.40.1.850.ge5e148ffb7d
Previous: Junio C HamanoNext: Calvin Wan
Message 55 of 111 in “Introduce Git Standard Library”
  1. 0/8 Introduce Git Standard LibraryCalvin Wan, Jun 27, 2023
  2. 1/8 trace2: log fsync stats in trace2 rather than wrapperCalvin Wan, Jun 27, 2023
  3. Victoria DyeJun 28, 2023
  4. Calvin WanJul 5, 2023
  5. Victoria DyeJul 5, 2023
  6. Jeff HostetlerJul 11, 2023
  7. 2/8 hex-ll: split out functionality from hexCalvin Wan, Jun 27, 2023
  8. Phillip WoodJun 28, 2023
  9. Calvin WanJun 28, 2023
  10. 3/8 object: move function to object.cCalvin Wan, Jun 27, 2023
  11. 4/8 config: correct bad boolean env value error messageCalvin Wan, Jun 27, 2023
  12. 5/8 parse: create new library for parsing strings and env valuesCalvin Wan, Jun 27, 2023
  13. Junio C HamanoJun 27, 2023
  14. 6/8 pager: remove pager_in_use()Calvin Wan, Jun 27, 2023
  15. Junio C HamanoJun 27, 2023
  16. Calvin WanJun 27, 2023
  17. Glen ChooJun 28, 2023
  18. Glen ChooJun 28, 2023
  19. Calvin WanJun 28, 2023
  20. Junio C HamanoJun 28, 2023
  21. Junio C HamanoJun 28, 2023
  22. 8/8 git-std-lib: add test file to call git-std-lib.a functionsCalvin Wan, Jun 27, 2023
  23. 7/8 git-std-lib: introduce git standard libraryCalvin Wan, Jun 27, 2023
  24. Phillip WoodJun 28, 2023
  25. Calvin WanJun 28, 2023
  26. Phillip WoodJun 30, 2023
  27. Glen ChooJun 28, 2023
  28. Calvin WanJun 28, 2023
  29. Linus ArverJun 30, 2023
  30. 0/7 Introduce Git Standard LibraryCalvin Wan, Aug 10, 2023
  31. 2/7 object: move function to object.cCalvin Wan, Aug 10, 2023
  32. Junio C HamanoAug 10, 2023
  33. Glen ChooAug 10, 2023
  34. Junio C HamanoAug 10, 2023
  35. 1/7 hex-ll: split out functionality from hexCalvin Wan, Aug 10, 2023
  36. 3/7 config: correct bad boolean env value error messageCalvin Wan, Aug 10, 2023
  37. Junio C HamanoAug 10, 2023
  38. 5/7 date: push pager.h dependency upCalvin Wan, Aug 10, 2023
  39. Glen ChooAug 10, 2023
  40. Jonathan TanAug 14, 2023
  41. 4/7 parse: create new library for parsing strings and env valuesCalvin Wan, Aug 10, 2023
  42. Glen ChooAug 10, 2023
  43. Junio C HamanoAug 10, 2023
  44. Jonathan TanAug 14, 2023
  45. Jonathan TanAug 14, 2023
  46. Junio C HamanoAug 14, 2023
  47. 7/7 git-std-lib: add test file to call git-std-lib.a functionsCalvin Wan, Aug 10, 2023
  48. Jonathan TanAug 14, 2023
  49. 6/7 git-std-lib: introduce git standard libraryCalvin Wan, Aug 10, 2023
  50. Jonathan TanAug 14, 2023
  51. Glen ChooAug 10, 2023
  52. Phillip WoodAug 15, 2023
  53. Calvin WanAug 16, 2023
  54. Junio C HamanoAug 16, 2023
  55. Phillip WoodAug 15, 2023
  56. 0/6 Introduce Git Standard LibraryCalvin Wan, Sep 8, 2023
  57. 2/6 wrapper: remove dependency to Git-specific internal fileCalvin Wan, Sep 8, 2023
  58. Jonathan TanSep 15, 2023
  59. 1/6 hex-ll: split out functionality from hexCalvin Wan, Sep 8, 2023
  60. 3/6 config: correct bad boolean env value error messageCalvin Wan, Sep 8, 2023
  61. 4/6 parse: create new library for parsing strings and env valuesCalvin Wan, Sep 8, 2023
  62. 5/6 git-std-lib: introduce git standard libraryCalvin Wan, Sep 8, 2023
  63. Phillip WoodSep 11, 2023
  64. Phillip WoodSep 27, 2023
  65. Jonathan TanSep 15, 2023
  66. phillip.wood123@gmail.comSep 26, 2023
  67. 6/6 git-std-lib: add test file to call git-std-lib.a functionsCalvin Wan, Sep 8, 2023
  68. Junio C HamanoSep 9, 2023
  69. Jonathan TanSep 15, 2023
  70. Junio C HamanoSep 15, 2023
  71. Junio C HamanoSep 8, 2023
  72. Junio C HamanoSep 8, 2023
  73. 0/4 Preliminary patches before git-std-libJonathan Tan, Sep 29, 2023
  74. 1/4 hex-ll: separate out non-hash-algo functionsJonathan Tan, Sep 29, 2023
  75. Linus ArverOct 21, 2023
  76. 2/4 wrapper: reduce scope of remove_or_warn()Jonathan Tan, Sep 29, 2023
  77. phillip.wood123@gmail.comOct 10, 2023
  78. Junio C HamanoOct 10, 2023
  79. Jonathan TanOct 10, 2023
  80. 3/4 config: correct bad boolean env value error messageJonathan Tan, Sep 29, 2023
  81. Junio C HamanoSep 29, 2023
  82. 4/4 parse: separate out parsing functions from config.hJonathan Tan, Sep 29, 2023
  83. phillip.wood123@gmail.comOct 10, 2023
  84. Jonathan TanOct 10, 2023
  85. Phillip WoodOct 10, 2023
  86. Junio C HamanoOct 10, 2023
  87. phillip.wood123@gmail.comOct 10, 2023
  88. Jonathan TanOct 10, 2023
  89. 0/3 Introduce Git Standard LibraryCalvin Wan, Feb 22, 2024
  90. 1/3 pager: include stdint.h because uintmax_t is usedCalvin Wan, Feb 22, 2024
  91. Junio C HamanoFeb 22, 2024
  92. Kyle LippincottFeb 26, 2024
  93. Junio C HamanoFeb 27, 2024
  94. Kyle LippincottFeb 27, 2024
  95. Junio C HamanoFeb 27, 2024
  96. Kyle LippincottFeb 27, 2024
  97. Junio C HamanoFeb 27, 2024
  98. Jeff KingFeb 27, 2024
  99. Jeff KingFeb 27, 2024
  100. Kyle LippincottFeb 27, 2024
  101. Kyle LippincottFeb 24, 2024
  102. Junio C HamanoFeb 24, 2024
  103. 2/3 git-std-lib: introduce Git Standard LibraryCalvin Wan, Feb 22, 2024
  104. Phillip WoodFeb 29, 2024
  105. Junio C HamanoFeb 29, 2024
  106. Linus ArverFeb 29, 2024
  107. Junio C HamanoFeb 29, 2024
  108. Linus ArverFeb 29, 2024
  109. 3/3 test-stdlib: show that git-std-lib is independentCalvin Wan, Feb 22, 2024
  110. Junio C HamanoFeb 22, 2024
  111. Junio C HamanoMar 7, 2024

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.