{"thread":{"id":"54764","subject":"[PATCH] cmake: determine list of extra built-ins dynamically","startedAt":"2020-12-04T19:34:40Z","lastAt":"2020-12-04T22:46:13Z","messageCount":5,"participants":["Johannes Schindelin via GitGitGadget","Junio C Hamano","Jeff King","Johannes Schindelin"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"411359","messageId":"pull.807.git.1607110436367.gitgitgadget@gmail.com","threadId":"54764","inReplyTo":null,"subject":"[PATCH] cmake: determine list of extra built-ins dynamically","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-12-04T19:33:56Z","receivedAt":"2020-12-04T19:34:40Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin <johannes.schindelin@gmx.de>\n\nIn 0a21d0e08902 (Makefile: mark git-maintenance as a builtin,\n2020-12-01), we marked git-maintenance as a builtin in the Makefile, but\nforgot to do the same in `CMakeLists.txt`.\n\nRather than always play catch-up and adjust `git_builtin_extra`\nmanually, use the `BUILT_INS` definitions in the Makefile as\nauthoritative source and generate `git_builtin_extra` dynamically.\n\nSigned-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n---\n    ci: fix the vs-build job after adding git maintenance\n    \n    Together with https://github.com/gitgitgadget/git/pull/805, this should\n    fix the vs-build job in the CI build of seen.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-807%2Fdscho%2Fcmake-and-extra-builtins-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-807/dscho/cmake-and-extra-builtins-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/807\n\n contrib/buildsystems/CMakeLists.txt | 15 +++++++++++----\n 1 file changed, 11 insertions(+), 4 deletions(-)\n\ndiff --git a/contrib/buildsystems/CMakeLists.txt b/contrib/buildsystems/CMakeLists.txt\nindex df539a44fa..c151dd7257 100644\n--- a/contrib/buildsystems/CMakeLists.txt\n+++ b/contrib/buildsystems/CMakeLists.txt\n@@ -114,6 +114,16 @@ macro(parse_makefile_for_scripts list_var regex lang)\n \tendif()\n endmacro()\n \n+macro(parse_makefile_for_executables list_var regex)\n+\tfile(STRINGS ${CMAKE_SOURCE_DIR}/Makefile ${list_var} REGEX \"^${regex} \\\\+= git-(.*)\")\n+\tstring(REPLACE \"${regex} +=\" \"\" ${list_var} ${${list_var}})\n+\tstring(STRIP ${${list_var}} ${list_var}) #remove trailing/leading whitespaces\n+\tstring(REPLACE \"git-\" \"\" ${list_var} ${${list_var}}) #strip `git-` prefix\n+\tstring(REPLACE \"\\$X\" \";\" ${list_var} ${${list_var}}) #strip $X, ; is for converting the string into a list\n+\tlist(TRANSFORM ${list_var} STRIP) #remove trailing/leading whitespaces for each element in list\n+\tlist(REMOVE_ITEM ${list_var} \"\") #remove empty list elements\n+endmacro()\n+\n include(CheckTypeSize)\n include(CheckCSourceRuns)\n include(CheckCSourceCompiles)\n@@ -673,10 +683,7 @@ if(CURL_FOUND)\n \tendif()\n endif()\n \n-set(git_builtin_extra\n-\tcherry cherry-pick format-patch fsck-objects\n-\tinit merge-subtree restore show\n-\tstage status switch whatchanged)\n+parse_makefile_for_executables(git_builtin_extra \"BUILT_INS\")\n \n #Creating hardlinks\n foreach(s ${git_SOURCES} ${git_builtin_extra})\n\nbase-commit: 898f80736c75878acc02dc55672317fcc0e0a5a6\n-- \ngitgitgadget\n"},{"id":"411373","messageId":"xmqqczzpe3i6.fsf@gitster.c.googlers.com","threadId":"54764","inReplyTo":"pull.807.git.1607110436367.gitgitgadget@gmail.com","subject":"Re: [PATCH] cmake: determine list of extra built-ins dynamically","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-12-04T20:10:41Z","receivedAt":"2020-12-04T20:11:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Johannes Schindelin via GitGitGadget\" <gitgitgadget@gmail.com>\nwrites:\n\n> From: Johannes Schindelin <johannes.schindelin@gmx.de>\n>\n> In 0a21d0e08902 (Makefile: mark git-maintenance as a builtin,\n> 2020-12-01), we marked git-maintenance as a builtin in the Makefile, but\n> forgot to do the same in `CMakeLists.txt`.\n>\n> Rather than always play catch-up and adjust `git_builtin_extra`\n> manually, use the `BUILT_INS` definitions in the Makefile as\n> authoritative source and generate `git_builtin_extra` dynamically.\n>\n> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n> ---\n>     ci: fix the vs-build job after adding git maintenance\n>     \n>     Together with https://github.com/gitgitgadget/git/pull/805, this should\n>     fix the vs-build job in the CI build of seen.\n\nI suspect that the breakage 805 addresses affects even 'master'; is\nthe breakage this patch fixes limited to 'seen', or affect everybody?\n\n> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-807%2Fdscho%2Fcmake-and-extra-builtins-v1\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-807/dscho/cmake-and-extra-builtins-v1\n> Pull-Request: https://github.com/gitgitgadget/git/pull/807\n>\n>  contrib/buildsystems/CMakeLists.txt | 15 +++++++++++----\n>  1 file changed, 11 insertions(+), 4 deletions(-)\n>\n> diff --git a/contrib/buildsystems/CMakeLists.txt b/contrib/buildsystems/CMakeLists.txt\n> index df539a44fa..c151dd7257 100644\n> --- a/contrib/buildsystems/CMakeLists.txt\n> +++ b/contrib/buildsystems/CMakeLists.txt\n> @@ -114,6 +114,16 @@ macro(parse_makefile_for_scripts list_var regex lang)\n>  \tendif()\n>  endmacro()\n>  \n> +macro(parse_makefile_for_executables list_var regex)\n> +\tfile(STRINGS ${CMAKE_SOURCE_DIR}/Makefile ${list_var} REGEX \"^${regex} \\\\+= git-(.*)\")\n> +\tstring(REPLACE \"${regex} +=\" \"\" ${list_var} ${${list_var}})\n> +\tstring(STRIP ${${list_var}} ${list_var}) #remove trailing/leading whitespaces\n> +\tstring(REPLACE \"git-\" \"\" ${list_var} ${${list_var}}) #strip `git-` prefix\n> +\tstring(REPLACE \"\\$X\" \";\" ${list_var} ${${list_var}}) #strip $X, ; is for converting the string into a list\n> +\tlist(TRANSFORM ${list_var} STRIP) #remove trailing/leading whitespaces for each element in list\n> +\tlist(REMOVE_ITEM ${list_var} \"\") #remove empty list elements\n> +endmacro()\n> +\n>  include(CheckTypeSize)\n>  include(CheckCSourceRuns)\n>  include(CheckCSourceCompiles)\n> @@ -673,10 +683,7 @@ if(CURL_FOUND)\n>  \tendif()\n>  endif()\n>  \n> -set(git_builtin_extra\n> -\tcherry cherry-pick format-patch fsck-objects\n> -\tinit merge-subtree restore show\n> -\tstage status switch whatchanged)\n> +parse_makefile_for_executables(git_builtin_extra \"BUILT_INS\")\n>  \n>  #Creating hardlinks\n>  foreach(s ${git_SOURCES} ${git_builtin_extra})\n>\n> base-commit: 898f80736c75878acc02dc55672317fcc0e0a5a6\n"},{"id":"411405","messageId":"X8qmClPjwYWIoExP@coredump.intra.peff.net","threadId":"54764","inReplyTo":"pull.807.git.1607110436367.gitgitgadget@gmail.com","subject":"Re: [PATCH] cmake: determine list of extra built-ins dynamically","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-12-04T21:11:38Z","receivedAt":"2020-12-04T21:12:36Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Dec 04, 2020 at 07:33:56PM +0000, Johannes Schindelin via GitGitGadget wrote:\n\n> In 0a21d0e08902 (Makefile: mark git-maintenance as a builtin,\n> 2020-12-01), we marked git-maintenance as a builtin in the Makefile, but\n> forgot to do the same in `CMakeLists.txt`.\n\nOof, right.\n\n> Rather than always play catch-up and adjust `git_builtin_extra`\n> manually, use the `BUILT_INS` definitions in the Makefile as\n> authoritative source and generate `git_builtin_extra` dynamically.\n\nYay. This is exactly how I'd hoped things would work or the cmake file\nin general. I don't mind following micro-formats within our Makefile to\nkeep things easier for the cmake parsing side.\n\n>  contrib/buildsystems/CMakeLists.txt | 15 +++++++++++----\n>  1 file changed, 11 insertions(+), 4 deletions(-)\n\nThe implementation looks plausibly correct to me (bearing in mind that\nI've never written cmake).\n\n-Peff\n"},{"id":"411413","messageId":"nycvar.QRO.7.76.6.2012042338580.25979@tvgsbejvaqbjf.bet","threadId":"54764","inReplyTo":"xmqqczzpe3i6.fsf@gitster.c.googlers.com","subject":"Re: [PATCH] cmake: determine list of extra built-ins dynamically","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-12-04T22:41:00Z","receivedAt":"2020-12-04T22:42:41Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Fri, 4 Dec 2020, Junio C Hamano wrote:\n\n> \"Johannes Schindelin via GitGitGadget\" <gitgitgadget@gmail.com>\n> writes:\n>\n> > From: Johannes Schindelin <johannes.schindelin@gmx.de>\n> >\n> > In 0a21d0e08902 (Makefile: mark git-maintenance as a builtin,\n> > 2020-12-01), we marked git-maintenance as a builtin in the Makefile, but\n> > forgot to do the same in `CMakeLists.txt`.\n> >\n> > Rather than always play catch-up and adjust `git_builtin_extra`\n> > manually, use the `BUILT_INS` definitions in the Makefile as\n> > authoritative source and generate `git_builtin_extra` dynamically.\n> >\n> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n> > ---\n> >     ci: fix the vs-build job after adding git maintenance\n> >\n> >     Together with https://github.com/gitgitgadget/git/pull/805, this should\n> >     fix the vs-build job in the CI build of seen.\n>\n> I suspect that the breakage 805 addresses affects even 'master'; is\n> the breakage this patch fixes limited to 'seen', or affect everybody?\n\nI haven't checked `master` yet, but `next` definitely was affected. I\n_think_ the problem was introduced when Peff's patch added the\n`git-maintenance` tool to the `BUILT_INS` list in the `Makefile` (and\nhence we try to `tar` it up with all the other build artifacts in the\n`vs-build` job) but wasn't added to `CMakeLists.txt` (and therefore\n`vs-build` would not generate that hard-link).\n\nCiao,\nDscho\n\n>\n> > Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-807%2Fdscho%2Fcmake-and-extra-builtins-v1\n> > Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-807/dscho/cmake-and-extra-builtins-v1\n> > Pull-Request: https://github.com/gitgitgadget/git/pull/807\n> >\n> >  contrib/buildsystems/CMakeLists.txt | 15 +++++++++++----\n> >  1 file changed, 11 insertions(+), 4 deletions(-)\n> >\n> > diff --git a/contrib/buildsystems/CMakeLists.txt b/contrib/buildsystems/CMakeLists.txt\n> > index df539a44fa..c151dd7257 100644\n> > --- a/contrib/buildsystems/CMakeLists.txt\n> > +++ b/contrib/buildsystems/CMakeLists.txt\n> > @@ -114,6 +114,16 @@ macro(parse_makefile_for_scripts list_var regex lang)\n> >  \tendif()\n> >  endmacro()\n> >\n> > +macro(parse_makefile_for_executables list_var regex)\n> > +\tfile(STRINGS ${CMAKE_SOURCE_DIR}/Makefile ${list_var} REGEX \"^${regex} \\\\+= git-(.*)\")\n> > +\tstring(REPLACE \"${regex} +=\" \"\" ${list_var} ${${list_var}})\n> > +\tstring(STRIP ${${list_var}} ${list_var}) #remove trailing/leading whitespaces\n> > +\tstring(REPLACE \"git-\" \"\" ${list_var} ${${list_var}}) #strip `git-` prefix\n> > +\tstring(REPLACE \"\\$X\" \";\" ${list_var} ${${list_var}}) #strip $X, ; is for converting the string into a list\n> > +\tlist(TRANSFORM ${list_var} STRIP) #remove trailing/leading whitespaces for each element in list\n> > +\tlist(REMOVE_ITEM ${list_var} \"\") #remove empty list elements\n> > +endmacro()\n> > +\n> >  include(CheckTypeSize)\n> >  include(CheckCSourceRuns)\n> >  include(CheckCSourceCompiles)\n> > @@ -673,10 +683,7 @@ if(CURL_FOUND)\n> >  \tendif()\n> >  endif()\n> >\n> > -set(git_builtin_extra\n> > -\tcherry cherry-pick format-patch fsck-objects\n> > -\tinit merge-subtree restore show\n> > -\tstage status switch whatchanged)\n> > +parse_makefile_for_executables(git_builtin_extra \"BUILT_INS\")\n> >\n> >  #Creating hardlinks\n> >  foreach(s ${git_SOURCES} ${git_builtin_extra})\n> >\n> > base-commit: 898f80736c75878acc02dc55672317fcc0e0a5a6\n>\n"},{"id":"411415","messageId":"nycvar.QRO.7.76.6.2012042341270.25979@tvgsbejvaqbjf.bet","threadId":"54764","inReplyTo":"X8qmClPjwYWIoExP@coredump.intra.peff.net","subject":"Re: [PATCH] cmake: determine list of extra built-ins dynamically","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-12-04T22:44:17Z","receivedAt":"2020-12-04T22:46:13Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Peff,\n\nOn Fri, 4 Dec 2020, Jeff King wrote:\n\n> On Fri, Dec 04, 2020 at 07:33:56PM +0000, Johannes Schindelin via GitGitGadget wrote:\n>\n> > Rather than always play catch-up and adjust `git_builtin_extra`\n> > manually, use the `BUILT_INS` definitions in the Makefile as\n> > authoritative source and generate `git_builtin_extra` dynamically.\n>\n> Yay. This is exactly how I'd hoped things would work or the cmake file\n> in general. I don't mind following micro-formats within our Makefile to\n> keep things easier for the cmake parsing side.\n\nMe, too. I am somewhat embarrassed that I missed the `git_builtin_extra`\nlist in my review of the CMake patch series...\n\n> >  contrib/buildsystems/CMakeLists.txt | 15 +++++++++++----\n> >  1 file changed, 11 insertions(+), 4 deletions(-)\n>\n> The implementation looks plausibly correct to me (bearing in mind that\n> I've never written cmake).\n\nIt is a close copy of the two macros we already use to parse the\n`Makefile` for lists of `.o` files and for scripts.\n\nTogether with the fact that this patch fixes the CI build of Git for\nWindows' `shears/seen` branch (which is a continuously-rebased version of\nGit for Windows' `main` branch onto `seen`, plus fixups), I am fairly\nconfident that it is correct.\n\nCiao,\nDscho\n"}]}