| Message ID | ea23ba5e269305b660a1722254e2a933c14e5b57.1598283480.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Optionally skip linking/copying the built-ins | expand |
"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com> writes: > From: Johannes Schindelin <johannes.schindelin@gmx.de> > > Originally, all of Git's subcommands were implemented in their own > executable/script, using the naming scheme `git-<command-name>`. When > more and more functionality was turned into built-in commands (i.e. the > `git` executable could run them without spawning a separate process), > for backwards-compatibility, we hard-link the `git` executable to > `git-<built-in>` for every built-in. > > This backwards-compatibility was needed to support scripts that called > the dashed form, even if we deprecated that a _long_ time ago. This paragraph is irrelevant. We are keeping the support for it and this topic is not newly deprecating or removing anything. We need to argue for a need to test an installation that lacks these builtin subcommands anywhere on disk under their own names, which you did succinctly below (and there is no need for "For that reason," there). > For that reason, we just introduced a Makefile knob to skip linking > them. TO make sure that this keeps working, teach the CI s/TO/To/ > (and PR) builds to skip generating those hard-links. What is not justified enough is why we no longer test installations with dashed builtins on disk. If this topic is primarily about Windows (as 2/3 said), perhaps we can do this only for Windows tasks before we make a colletive decision to _DROP_ support for the on-disk builtin subcommands? > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> > --- > ci/run-build-and-tests.sh | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/ci/run-build-and-tests.sh b/ci/run-build-and-tests.sh > index 6c27b886b8..1df9402c3b 100755 > --- a/ci/run-build-and-tests.sh > +++ b/ci/run-build-and-tests.sh > @@ -10,7 +10,7 @@ windows*) cmd //c mklink //j t\\.prove "$(cygpath -aw "$cache_dir/.prove")";; > *) ln -s "$cache_dir/.prove" t/.prove;; > esac > > -make > +make SKIP_DASHED_BUILT_INS=YesPlease > case "$jobname" in > linux-gcc) > make test
Hi Junio, On Mon, 24 Aug 2020, Junio C Hamano wrote: > "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com> > writes: > > > From: Johannes Schindelin <johannes.schindelin@gmx.de> > > > > Originally, all of Git's subcommands were implemented in their own > > executable/script, using the naming scheme `git-<command-name>`. When > > more and more functionality was turned into built-in commands (i.e. the > > `git` executable could run them without spawning a separate process), > > for backwards-compatibility, we hard-link the `git` executable to > > `git-<built-in>` for every built-in. > > > > This backwards-compatibility was needed to support scripts that called > > the dashed form, even if we deprecated that a _long_ time ago. > > This paragraph is irrelevant. We are keeping the support for it and > this topic is not newly deprecating or removing anything. We need > to argue for a need to test an installation that lacks these builtin > subcommands anywhere on disk under their own names, which you did > succinctly below (and there is no need for "For that reason," > there). Could we please keep it? It will help me in the future when stumbling over this commit, to remember the context. > > For that reason, we just introduced a Makefile knob to skip linking > > them. TO make sure that this keeps working, teach the CI > > s/TO/To/ Thanks! I guess my keys got sticky or something ;-) > > (and PR) builds to skip generating those hard-links. > > What is not justified enough is why we no longer test installations > with dashed builtins on disk. If this topic is primarily about > Windows (as 2/3 said), perhaps we can do this only for Windows tasks > before we make a colletive decision to _DROP_ support for the on-disk > builtin subcommands? Oh, sorry, I will amend the commit message to clarify that the dashed form is actually not tested at all anymore. Specifically since e4597aae6590 (run test suite without dashed git-commands in PATH, 2009-12-02), in fact. All this change does is to make it an even stronger committment to run the test suite without dashed Git commands. Thanks, Dscho > > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> > > --- > > ci/run-build-and-tests.sh | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/ci/run-build-and-tests.sh b/ci/run-build-and-tests.sh > > index 6c27b886b8..1df9402c3b 100755 > > --- a/ci/run-build-and-tests.sh > > +++ b/ci/run-build-and-tests.sh > > @@ -10,7 +10,7 @@ windows*) cmd //c mklink //j t\\.prove "$(cygpath -aw "$cache_dir/.prove")";; > > *) ln -s "$cache_dir/.prove" t/.prove;; > > esac > > > > -make > > +make SKIP_DASHED_BUILT_INS=YesPlease > > case "$jobname" in > > linux-gcc) > > make test >
On Mon, Aug 24, 2020 at 03:38:00PM +0000, Johannes Schindelin via GitGitGadget wrote: > From: Johannes Schindelin <johannes.schindelin@gmx.de> > > Originally, all of Git's subcommands were implemented in their own > executable/script, using the naming scheme `git-<command-name>`. When > more and more functionality was turned into built-in commands (i.e. the > `git` executable could run them without spawning a separate process), > for backwards-compatibility, we hard-link the `git` executable to > `git-<built-in>` for every built-in. > > This backwards-compatibility was needed to support scripts that called > the dashed form, even if we deprecated that a _long_ time ago. > > For that reason, we just introduced a Makefile knob to skip linking > them. TO make sure that this keeps working, teach the CI > (and PR) builds to skip generating those hard-links. I'm afraid I don't understand this patch or the previous one (or both?). So this new Makefile knob stops hard-linking the dashed builtins _during 'make install'_, but it doesn't affect how Git is built by the default target. And our CI jobs only build Git by the default target, but don't run 'make install', so setting SKIP_DASHED_BUILT_INS wouldn't have any affect anyway. > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> > --- > ci/run-build-and-tests.sh | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/ci/run-build-and-tests.sh b/ci/run-build-and-tests.sh > index 6c27b886b8..1df9402c3b 100755 > --- a/ci/run-build-and-tests.sh > +++ b/ci/run-build-and-tests.sh > @@ -10,7 +10,7 @@ windows*) cmd //c mklink //j t\\.prove "$(cygpath -aw "$cache_dir/.prove")";; > *) ln -s "$cache_dir/.prove" t/.prove;; > esac > > -make > +make SKIP_DASHED_BUILT_INS=YesPlease Note that the CI jobs executed in containers (Linux32 and linux-musl) don't use this 'ci/run-build-and-tests.sh' script, so they won't set SKIP_DASHED_BUILT_INS. I suppose that's unintentional, because it wasn't mentioned in the commit message. > case "$jobname" in > linux-gcc) > make test > -- > gitgitgadget
SZEDER Gábor <szeder.dev@gmail.com> writes: > I'm afraid I don't understand this patch or the previous one (or > both?). So this new Makefile knob stops hard-linking the dashed > builtins _during 'make install'_, but it doesn't affect how Git is > built by the default target. And our CI jobs only build Git by the > default target, but don't run 'make install', so setting > SKIP_DASHED_BUILT_INS wouldn't have any affect anyway. Very very true. Let's drop 3/3 if it is not testing anything new. I do understand the concern 2/3 wants to address, and it would be a real one to you especially if you come from Windows. People on the platform wouldn't be able to use shell scripts written in 12 years ago or written with the promise we made to our users 12 years ago, and unlike hardlink-capable platforms it incurs real cost to install these individual binaries on disk.
Hi, On Tue, 25 Aug 2020, Junio C Hamano wrote: > SZEDER Gábor <szeder.dev@gmail.com> writes: > > > I'm afraid I don't understand this patch or the previous one (or > > both?). So this new Makefile knob stops hard-linking the dashed > > builtins _during 'make install'_, but it doesn't affect how Git is > > built by the default target. And our CI jobs only build Git by the > > default target, but don't run 'make install', so setting > > SKIP_DASHED_BUILT_INS wouldn't have any affect anyway. > > Very very true. Let's drop 3/3 if it is not testing anything new. > > I do understand the concern 2/3 wants to address, and it would be a > real one to you especially if you come from Windows. People on the > platform wouldn't be able to use shell scripts written in 12 years > ago or written with the promise we made to our users 12 years ago, > and unlike hardlink-capable platforms it incurs real cost to install > these individual binaries on disk. Actually, `SKIP_DASHED_BUILT_INS` does not _only_ have an impact on `make install`: $ rm git-add.exe && make BUILTIN git-add.exe BUILTIN all SUBDIR git-gui SUBDIR gitk-git SUBDIR templates $ rm git-add.exe && make SKIP_DASHED_BUILT_INS=1 BUILTIN all SUBDIR git-gui SUBDIR gitk-git SUBDIR templates See how `git-add.exe` is linked in the first, but not in the second run? So the difference 3/3 has is that those hard-linked executables are not even generated. Now, _technically_ this should not result in any change because we run the test suite without `--with-dashes`. Practically, it _does_ make a difference, though, as `bin-wrappers/git` _specifically_ sets `GIT_EXEC_PATH` to the top-level directory, i.e. `git-add.exe` _would_ be found if any core Git command that is still implemented as a script called `git-add`. Therefore, 3/3 makes sure that we really, really, really do not use those dashed invocations ourselves. Ciao, Dscho
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes: > Actually, `SKIP_DASHED_BUILT_INS` does not _only_ have an impact on `make > install`: > ... > See how `git-add.exe` is linked in the first, but not in the second run? OK, that is one more reason why we do want to have 3/3 applied not for all tasks in the CI , but for subset of tasks that includes the Windows task. If we had multiple Windows tasks, it may even be better to have only to some tasks, and allow other tasks build git-add.exe, so that both can be tested for the primary intended platform. Thanks.
Junio C Hamano <gitster@pobox.com> writes: > Johannes Schindelin <Johannes.Schindelin@gmx.de> writes: > >> Actually, `SKIP_DASHED_BUILT_INS` does not _only_ have an impact on `make >> install`: >> ... >> See how `git-add.exe` is linked in the first, but not in the second run? > > OK, that is one more reason why we do want to have 3/3 applied not > for all tasks in the CI , but for subset of tasks that includes the > Windows task. If we had multiple Windows tasks, it may even be > better to have only to some tasks, and allow other tasks build > git-add.exe, so that both can be tested for the primary intended > platform. In other words, something like this squashed in. ci/run-build-and-tests.sh | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/ci/run-build-and-tests.sh b/ci/run-build-and-tests.sh index 1df9402c3b..cfb841d981 100755 --- a/ci/run-build-and-tests.sh +++ b/ci/run-build-and-tests.sh @@ -5,12 +5,16 @@ . ${0%/*}/lib.sh +BUILTINS_HOW= case "$CI_OS_NAME" in -windows*) cmd //c mklink //j t\\.prove "$(cygpath -aw "$cache_dir/.prove")";; -*) ln -s "$cache_dir/.prove" t/.prove;; +windows*) + BUILTINS_HOW=SKIP_DASHED_BUILT_INS=YesPlease + cmd //c mklink //j t\\.prove "$(cygpath -aw "$cache_dir/.prove")";; +*) + ln -s "$cache_dir/.prove" t/.prove;; esac -make SKIP_DASHED_BUILT_INS=YesPlease +make $BUILTINS_HOW case "$jobname" in linux-gcc) make test
On Wed, Aug 26, 2020 at 06:19:52AM +0200, Johannes Schindelin wrote: > Hi, > > On Tue, 25 Aug 2020, Junio C Hamano wrote: > > > SZEDER Gábor <szeder.dev@gmail.com> writes: > > > > > I'm afraid I don't understand this patch or the previous one (or > > > both?). So this new Makefile knob stops hard-linking the dashed > > > builtins _during 'make install'_, but it doesn't affect how Git is > > > built by the default target. And our CI jobs only build Git by the > > > default target, but don't run 'make install', so setting > > > SKIP_DASHED_BUILT_INS wouldn't have any affect anyway. > > > > Very very true. Let's drop 3/3 if it is not testing anything new. > > > > I do understand the concern 2/3 wants to address, and it would be a > > real one to you especially if you come from Windows. People on the > > platform wouldn't be able to use shell scripts written in 12 years > > ago or written with the promise we made to our users 12 years ago, > > and unlike hardlink-capable platforms it incurs real cost to install > > these individual binaries on disk. > > Actually, `SKIP_DASHED_BUILT_INS` does not _only_ have an impact on `make > install`: > > $ rm git-add.exe && make > BUILTIN git-add.exe > BUILTIN all > SUBDIR git-gui > SUBDIR gitk-git > SUBDIR templates > > $ rm git-add.exe && make SKIP_DASHED_BUILT_INS=1 > BUILTIN all > SUBDIR git-gui > SUBDIR gitk-git > SUBDIR templates > > See how `git-add.exe` is linked in the first, but not in the second run? Ah, ok, so I did indeed misunderstand the previous patch. Thanks.
Hi Junio, On Wed, 26 Aug 2020, Junio C Hamano wrote: > Johannes Schindelin <Johannes.Schindelin@gmx.de> writes: > > > Actually, `SKIP_DASHED_BUILT_INS` does not _only_ have an impact on `make > > install`: > > ... > > See how `git-add.exe` is linked in the first, but not in the second run? > > OK, that is one more reason why we do want to have 3/3 applied not > for all tasks in the CI , but for subset of tasks that includes the > Windows task. If we had multiple Windows tasks, it may even be > better to have only to some tasks, and allow other tasks build > git-add.exe, so that both can be tested for the primary intended > platform. If you want to skip this patch, that's fine with me. But I would like to clarify what I perceive as a misunderstanding: this patch is not about testing whether it would install the necessary files or not. What this patch does is simply to complete the mission of e4597aae6590 (run test suite without dashed git-commands in PATH, 2009-12-02): to make sure that our very own scripts do not use dashed invocations of built-in commands. In that respect, I find it to make more sense to either do it, or not do it (even if I don't quite understand why we wouldn't do it), instead of doing it only for one platform. Ciao, Dscho
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes: > What this patch does is simply to complete the mission of e4597aae6590 > (run test suite without dashed git-commands in PATH, 2009-12-02): to make > sure that our very own scripts do not use dashed invocations of built-in > commands. OK, then I totally misread the proposed log message, or the proposed log message didn't adequately describe what it was trying to solve. With e4597aae (run test suite without dashed git-commands in PATH, 2009-12-02), we should not need git-foo in bin-wrappers/ for most of git subcommands, as long as our tests and scripted porcelains the test invokes never use the dashed form. And with this step, we come closer to that goal? "git checkout next && make test" does not seem to populate anything extra in bin-wrappers but bare minimum that needs due to the protocol requirements, though, without this patch, so the above may not be the whole story. Apparently, the patch does not achieve this goal. For that reason, we just introduced a Makefile knob to skip linking them. TO make sure that this keeps working, teach the CI (and PR) builds to skip generating those hard-links. because what matters to tests, when run without with-dash, which is the sensible modern default, is what is in bin-wrappers/ and we do not populate it with builtin git-add... at all even before this step. In other words, this change has no way to make sure "skip linking them" will keep working more than what we already have. Rather, Since e4597aae6590, we stopped running our tests with "git-foo" binaries found at the top-level of a freshly built source tree; instead we have placed only "git" and selected "git-foo" commands that must be on $PATH in bin-wrappers/ and they were what used by the tests. This is to catch the tests and scripted Porcelains that are tested will get caught if they try to use the dashed form. Since CI jobs will not install the built Git to anywhere, and the links we make at the top-level of the source tree for "git-add" and friends are not even used during tests, they are pure waste of resources these days. Thanks to the newly invented SKIP_DASHED_BUILT_INS knob, we can now skip creating these links in the source tree. Do so. or something along the line, perhaps. Thanks.
diff --git a/ci/run-build-and-tests.sh b/ci/run-build-and-tests.sh index 6c27b886b8..1df9402c3b 100755 --- a/ci/run-build-and-tests.sh +++ b/ci/run-build-and-tests.sh @@ -10,7 +10,7 @@ windows*) cmd //c mklink //j t\\.prove "$(cygpath -aw "$cache_dir/.prove")";; *) ln -s "$cache_dir/.prove" t/.prove;; esac -make +make SKIP_DASHED_BUILT_INS=YesPlease case "$jobname" in linux-gcc) make test