{"thread":{"id":"55898","subject":"Re: Build errors when building git on MacOS 11 (x86-64) and for M1 macs","startedAt":"2021-06-10T14:02:04Z","lastAt":"2021-06-10T14:51:41Z","messageCount":4,"participants":["Ævar Arnfjörð Bjarmason","Felipe Contreras","Jeff King"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"427008","messageId":"87sg1p24zc.fsf@evledraar.gmail.com","threadId":"55898","inReplyTo":"f5487adb-b7dc-4f97-bca1-749701337759n@googlegroups.com","subject":"Re: Build errors when building git on MacOS 11 (x86-64) and for M1 macs","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-06-10T13:52:09Z","receivedAt":"2021-06-10T14:02:04Z","isPatch":false,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\n[CC-ing the main git@ list for the \"bug in git\" aspect of this]\n\nOn Thu, Jun 10 2021, Nikhil Gupta wrote:\n\n>   | The following shell command exited with status 2:\n>   |  \n>   | $ CFLAGS=-I/opt/chef-workstation/embedded/include -O3 -D_FORTIFY_SOURCE=2 -fstack-protector CPPFLAGS=-I/opt/chef-workstation/embedded/include -O3\n> -D_FORTIFY_SOURCE=2 -fstack-protector CXXFLAGS=-I/opt/chef-workstation/embedded/include -O3 -D_FORTIFY_SOURCE=2 -fstack-protector\n> LDFLAGS=-Wl,-rpath,/opt/chef-workstation/embedded/lib -L/opt/chef-workstation/embedded/lib LD_RUN_PATH=/opt/chef-workstation/embedded/lib MAKE=gmake\n> OMNIBUS_INSTALL_DIR=/opt/chef-workstation\n> PATH=/opt/chef-workstation/bin:/opt/chef-workstation/embedded/bin:/Users/administrator/.buildkite-agent/builds/MM009-local-1/chef/chef-chef-workstation-master-omnibus-adhoc/omnibus/vendor/bundle/ruby/2.7.0/bin:/opt/angry-omnibus-toolchain/bin:/usr/local/bin:/usr/bin:/bin:/usr/sbin:/sbin\n> PKG_CONFIG_PATH=/opt/chef-workstation/embedded/lib/pkgconfig gmake prefix=/opt/chef-workstation/embedded bindir=/opt/chef-workstation/gitbin -j 10\n> [...]\n>   | * new script parameters\n>   | builtin/archive.c:48:24: error: implicit declaration of function 'archive_format_from_filename' is invalid in C99\n> [-Werror,-Wimplicit-function-declaration]\n>   | const char *format = archive_format_from_filename(name_hint);\n>   | ^\n>   | builtin/archive.c:48:24: note: did you mean 'archive_read_open_filename'?\n>   | /opt/chef-workstation/embedded/include/archive.h:527:15: note: 'archive_read_open_filename' declared here\n>   | __LA_DECL int archive_read_open_filename(struct archive *,\n>   | ^\n>   | builtin/archive.c:48:15: warning: incompatible integer to pointer conversion initializing 'const char *' with an expression of type 'int'\n> [-Wint-conversion]\n>   | const char *format = archive_format_from_filename(name_hint);\n>   | ^        ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~\n>   | builtin/archive.c:101:2: error: implicit declaration of function 'init_archivers' is invalid in C99 [-Werror,-Wimplicit-function-declaration]\n>   | init_archivers();\n>   | ^\n>   | builtin/archive.c:111:9: error: implicit declaration of function 'write_archive' is invalid in C99 [-Werror,-Wimplicit-function-declaration]\n>   | return write_archive(argc, argv, prefix, the_repository, output, 0);\n>   | ^\n>   | builtin/archive.c:111:9: note: did you mean 'write_or_die'?\n>   | ./cache.h:1737:6: note: 'write_or_die' declared here\n>   | void write_or_die(int fd, const void *buf, size_t count);\n>   | ^\n>   | 1 warning and 3 errors generated.\n>   | gmake: *** [Makefile:2431: builtin/archive.o] Error 1\n>   | gmake: *** Waiting for unfinished jobs....\n>   |  \n>   |  \n\nI have no access to such a system, but I think think I see the problem\nfrom what you've supplied here.\n\nYou supplied a CFLAGS=-I/opt/chef-workstation/embedded/include which has\nan archive.h file that's unrelated to the archive.h file git expects.\n\nThus we include that and find things unrelated to git, and error when we\nencounter a function we expected to have declared in our own archive.h.\n\nThe solution is something like defining a config.mak file where you add\nflags with BASIC_CFLAGS +=, not =. See config.mak.uname for an\nexample. You'll then add new directories after our own -I.\n\nThis is arguably a bug in git's Makefile in that we should have that\n\"-I.\" in an ESSENTIAL_CFLAGS variable or something, I can't think of a\nscenario where git would compile without it. That or things in builtin/\nshould include e.g. ../archive.h, perhaps such a thing isn't portable.\n\nI think (but have not confirmed) that you probably got this far because\nyour compiler will stick -I. at the /end/ of the flags implicitly (or is\nthat standardized C behavior? I can't remember).\n\nSo it worked until we had a filename conflict, i.e. we'd find strbuf.h\nin our own sources, but have an issue with a common name like archive.h.\n"},{"id":"427012","messageId":"60c22083b4131_b25b1208f6@natae.notmuch","threadId":"55898","inReplyTo":"87sg1p24zc.fsf@evledraar.gmail.com","subject":"Re: Build errors when building git on MacOS 11 (x86-64) and for M1 macs","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2021-06-10T14:24:03Z","receivedAt":"2021-06-10T14:24:21Z","isPatch":false,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Ævar Arnfjörð Bjarmason wrote:\n\n> This is arguably a bug in git's Makefile in that we should have that\n> \"-I.\" in an ESSENTIAL_CFLAGS variable or something, I can't think of a\n> scenario where git would compile without it.\n\nIf they are so essential we probably don't even need a variable, just\nput it directly into the $(CC) command.\n\n-- \nFelipe Contreras"},{"id":"427013","messageId":"YMIjaytMYslKjK6z@coredump.intra.peff.net","threadId":"55898","inReplyTo":"87sg1p24zc.fsf@evledraar.gmail.com","subject":"Re: Build errors when building git on MacOS 11 (x86-64) and for M1 macs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-06-10T14:36:27Z","receivedAt":"2021-06-10T14:36:29Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jun 10, 2021 at 03:52:09PM +0200, Ævar Arnfjörð Bjarmason wrote:\n\n> I have no access to such a system, but I think think I see the problem\n> from what you've supplied here.\n> \n> You supplied a CFLAGS=-I/opt/chef-workstation/embedded/include which has\n> an archive.h file that's unrelated to the archive.h file git expects.\n> \n> Thus we include that and find things unrelated to git, and error when we\n> encounter a function we expected to have declared in our own archive.h.\n\nYeah, I agree that is likely the problem.\n\n> The solution is something like defining a config.mak file where you add\n> flags with BASIC_CFLAGS +=, not =. See config.mak.uname for an\n> example. You'll then add new directories after our own -I.\n\nThat would work, though I think in general we should consider\nBASIC_CFLAGS to be an internal thing, and encourage people to use the\nstandard CFLAGS, etc.\n\n> This is arguably a bug in git's Makefile in that we should have that\n> \"-I.\" in an ESSENTIAL_CFLAGS variable or something, I can't think of a\n> scenario where git would compile without it. That or things in builtin/\n> should include e.g. ../archive.h, perhaps such a thing isn't portable.\n\nYeah, we do say \"-I.\" explicitly and unconditionally. I think we'd just\nwant to make sure it's before any user-provided flags. We could do\nsomething like this:\n\ndiff --git a/Makefile b/Makefile\nindex 7d719ece46..b5794560e9 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -1241,7 +1241,7 @@ ALL_COMMANDS_TO_INSTALL += git-upload-archive$(X)\n ALL_COMMANDS_TO_INSTALL += git-upload-pack$(X)\n endif\n \n-ALL_CFLAGS = $(DEVELOPER_CFLAGS) $(CPPFLAGS) $(CFLAGS)\n+ALL_CFLAGS = $(DEVELOPER_CFLAGS) $(CPPFLAGS) $(BASIC_CFLAGS) $(CFLAGS)\n ALL_LDFLAGS = $(LDFLAGS)\n \n comma := ,\n@@ -2106,7 +2106,6 @@ PAGER_ENV_CQ = \"$(subst \",\\\",$(subst \\,\\\\,$(PAGER_ENV)))\"\n PAGER_ENV_CQ_SQ = $(subst ','\\'',$(PAGER_ENV_CQ))\n BASIC_CFLAGS += -DPAGER_ENV='$(PAGER_ENV_CQ_SQ)'\n \n-ALL_CFLAGS += $(BASIC_CFLAGS)\n ALL_LDFLAGS += $(BASIC_LDFLAGS)\n \n export DIFF TAR INSTALL DESTDIR SHELL_PATH\n\nbut I imagine that it is helpful for other BASIC_CFLAGS to be\noverrideable by the user's CFLAGS (especially other system-level -I\ndirectives we may include!). So yes, having an $(ESSENTIAL_CFLAGS) would\nwork. Or perhaps even just:\n\ndiff --git a/Makefile b/Makefile\nindex 7d719ece46..72517db31e 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -1202,7 +1202,7 @@ endif\n CFLAGS = -g -O2 -Wall\n LDFLAGS =\n CC_LD_DYNPATH = -Wl,-rpath,\n-BASIC_CFLAGS = -I.\n+BASIC_CFLAGS =\n BASIC_LDFLAGS =\n \n # library flags\n@@ -1241,7 +1241,7 @@ ALL_COMMANDS_TO_INSTALL += git-upload-archive$(X)\n ALL_COMMANDS_TO_INSTALL += git-upload-pack$(X)\n endif\n \n-ALL_CFLAGS = $(DEVELOPER_CFLAGS) $(CPPFLAGS) $(CFLAGS)\n+ALL_CFLAGS = -I. $(DEVELOPER_CFLAGS) $(CPPFLAGS) $(CFLAGS)\n ALL_LDFLAGS = $(LDFLAGS)\n \n comma := ,\n\nsince this \"-I.\" really is special.\n\n> I think (but have not confirmed) that you probably got this far because\n> your compiler will stick -I. at the /end/ of the flags implicitly (or is\n> that standardized C behavior? I can't remember).\n\nThe standard just says (for quoted #include lines):\n\n  The named source file is searched for in an implementation-defined\n  manner.\n\nMost compilers put the directory of the source file at the start of the\nsearch path. E.g., gcc says under -I:\n\n  1. For the quote form of the include directive, the directory of the\n     current file is searched first.\n\nWhich is why we need \"-I.\" at all; we are finding \"archive.h\" from\n\"builtin/*.c\". I'm not sure what \"current file\" there means, or how\nportable it is. Is it the source file being filed, or the file\ncontaining the #include directive?\n\nI'd hope it's consistently the latter. Otherwise \"foo.h\" which includes\n\"bar.h\" cannot be included as \"../foo.h\" (from builtin/foo.c) and as\n\"foo.h\" (from top-level foo.c).\n\nIf so, then yeah, using \"../archive.h\" (and dropping -I. entirely) would\nbe an option. Which is nice, because it makes things less magical and\nmore predictable (think what confusion we'd see if we introduced\n\"archive.h\" into builtin/ ourselves).\n\nWe do use \"../foo.h\" in lots of places already. But the fact that we\n_also_ have \"-I.\" means I don't think we have any data on the \"what does\ncurrent file mean\" question above. My gut feeling is that it probably\ndoes consistently work as we'd want it to, though.\n\n-Peff\n"},{"id":"427017","messageId":"YMIm+hqqeY2IjEW3@coredump.intra.peff.net","threadId":"55898","inReplyTo":"YMIjaytMYslKjK6z@coredump.intra.peff.net","subject":"Re: Build errors when building git on MacOS 11 (x86-64) and for M1 macs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-06-10T14:51:38Z","receivedAt":"2021-06-10T14:51:41Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jun 10, 2021 at 10:36:27AM -0400, Jeff King wrote:\n\n> Most compilers put the directory of the source file at the start of the\n> search path. E.g., gcc says under -I:\n> \n>   1. For the quote form of the include directive, the directory of the\n>      current file is searched first.\n> \n> Which is why we need \"-I.\" at all; we are finding \"archive.h\" from\n> \"builtin/*.c\". I'm not sure what \"current file\" there means, or how\n> portable it is. Is it the source file being filed, or the file\n> containing the #include directive?\n> \n> I'd hope it's consistently the latter. Otherwise \"foo.h\" which includes\n> \"bar.h\" cannot be included as \"../foo.h\" (from builtin/foo.c) and as\n> \"foo.h\" (from top-level foo.c).\n> \n> If so, then yeah, using \"../archive.h\" (and dropping -I. entirely) would\n> be an option. Which is nice, because it makes things less magical and\n> more predictable (think what confusion we'd see if we introduced\n> \"archive.h\" into builtin/ ourselves).\n\nThere's a related case, which is that within trace2/foo.c, we'd include\ntrace2/foo.h. Without \"-I.\", the correct include there is just \"foo.h\".\n\nSo what was explicit actually becomes implicit. Here's a real example to\nmake this file work without -I:\n\n  diff --git a/trace2/tr2_tgt_perf.c b/trace2/tr2_tgt_perf.c\n  index a8018f18cc..592c194ab4 100644\n  --- a/trace2/tr2_tgt_perf.c\n  +++ b/trace2/tr2_tgt_perf.c\n  @@ -1,15 +1,15 @@\n  -#include \"cache.h\"\n  -#include \"config.h\"\n  -#include \"run-command.h\"\n  -#include \"quote.h\"\n  -#include \"version.h\"\n  -#include \"json-writer.h\"\n  -#include \"trace2/tr2_dst.h\"\n  -#include \"trace2/tr2_sid.h\"\n  -#include \"trace2/tr2_sysenv.h\"\n  -#include \"trace2/tr2_tbuf.h\"\n  -#include \"trace2/tr2_tgt.h\"\n  -#include \"trace2/tr2_tls.h\"\n  +#include \"../cache.h\"\n  +#include \"../config.h\"\n  +#include \"../run-command.h\"\n  +#include \"../quote.h\"\n  +#include \"../version.h\"\n  +#include \"../json-writer.h\"\n  +#include \"tr2_dst.h\"\n  +#include \"tr2_sid.h\"\n  +#include \"tr2_sysenv.h\"\n  +#include \"tr2_tbuf.h\"\n  +#include \"tr2_tgt.h\"\n  +#include \"tr2_tls.h\"\n\nSo I dunno. I had hoped it would be a strict readability improvement,\nbut now I'm not so sure.\n\n-Peff\n"}]}