| Message ID | patch-3.7-bde9de756b4-20210329T162327Z-avarab@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Makefile: make non-symlink & non-hardlink install sane | expand |
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > Refactor out the hard-to-read and maintain "ln || ln -s || cp" > pattern. > > This was initially added in 3e073dc5611 (Makefile: always provide a > fallback when hardlinks fail, 2008-08-25), but since then it's become > a lot more complex as we've added: > > * 3426e34fedd (Add NO_CROSS_DIRECTORY_HARDLINKS support to the > Makefile, 2009-05-11) > > * NO_INSTALL_HARDLINKS in 70de5e65e8c (Makefile: > NO_INSTALL_HARDLINKS, 2012-05-02) > > * INSTALL_SYMLINKS in ad874608d8c (Makefile: optionally symlink > libexec/git-core binaries to bin/git, 2018-03-13) > > * SKIP_DASHED_BUILT_INS 179227d6e21 (Optionally skip linking/copying > the built-ins, 2020-09-21) > > Each of those commits had to add a new special-case to this code, > resulting in quite an unmaintainable mess for adding any sort of new > option. > > Let's use the newly introduced ln-or-cp.sh script instead, note that > we only sometimes pass the --no-cross-directory-hardlinks option, per > the previous behavior. The target of the "ln -s" is also another > special snowflake, but we're careful to carry that forward. Nice. These explicit command-line options to the helper may be harder to initially write and maintain than just exporting the relevant $(MAKE) macros and using it from the helper, but once it works correctly, it is much easier to see what is going on. And obviously, if we want to fix the "I do not ever want to see any symlinks", it is very clear that main_with_fallbacks is where the change needs to go. Raelly nice.
On Mon, Mar 29, 2021 at 03:24:33PM -0700, Junio C Hamano wrote: > Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > > > Refactor out the hard-to-read and maintain "ln || ln -s || cp" > > pattern. > > > > This was initially added in 3e073dc5611 (Makefile: always provide a > > fallback when hardlinks fail, 2008-08-25), but since then it's become > > a lot more complex as we've added: > > > > * 3426e34fedd (Add NO_CROSS_DIRECTORY_HARDLINKS support to the > > Makefile, 2009-05-11) > > > > * NO_INSTALL_HARDLINKS in 70de5e65e8c (Makefile: > > NO_INSTALL_HARDLINKS, 2012-05-02) > > > > * INSTALL_SYMLINKS in ad874608d8c (Makefile: optionally symlink > > libexec/git-core binaries to bin/git, 2018-03-13) > > > > * SKIP_DASHED_BUILT_INS 179227d6e21 (Optionally skip linking/copying > > the built-ins, 2020-09-21) > > > > Each of those commits had to add a new special-case to this code, > > resulting in quite an unmaintainable mess for adding any sort of new > > option. > > > > Let's use the newly introduced ln-or-cp.sh script instead, note that > > we only sometimes pass the --no-cross-directory-hardlinks option, per > > the previous behavior. The target of the "ln -s" is also another > > special snowflake, but we're careful to carry that forward. > > Nice. These explicit command-line options to the helper may be > harder to initially write and maintain than just exporting the > relevant $(MAKE) macros and using it from the helper, but once > it works correctly, it is much easier to see what is going on. Another option is to make "ln-or-cp" itself a target that "make" knows how to build, and bake the values into its "built" version. Besides making the invocations a bit shorter, it also means that the dependency graph is more correct. If a rule invokes ln-or-cp, its behavior will change if $NO_INSTALL_HARDLINKS changes, for example. Telling that to make requires depending on a sentinel file in each such caller (like GIT-BUILD-OPTIONS). Whereas we could do that once for the "ln-or-cp" target, and then everyone who uses it just depends on it. I had a series a long time ago that moved the whole Makefile in that direction, but I got nervous that it was too disruptive and too non-idiomatic to be worth pursuing. So I offer the alternative here mostly as food for thought. It may not be a good direction (and we already have good-enough solutions, like depending on GIT-BUILD-OPTIONS). -Peff
Jeff King <peff@peff.net> writes: >> Nice. These explicit command-line options to the helper may be >> harder to initially write and maintain than just exporting the >> relevant $(MAKE) macros and using it from the helper, but once >> it works correctly, it is much easier to see what is going on. > > Another option is to make "ln-or-cp" itself a target that "make" knows > how to build, and bake the values into its "built" version. Aha. That is even nicer ;-).
diff --git a/Makefile b/Makefile index cfc87d7734d..a4784f28f5b 100644 --- a/Makefile +++ b/Makefile @@ -3007,40 +3007,41 @@ endif { test "$$bindir/" = "$$execdir/" || \ for p in git$X $(filter $(install_bindir_programs),$(ALL_PROGRAMS)); do \ $(RM) "$$execdir/$$p" && \ - test -n "$(INSTALL_SYMLINKS)" && \ - ln -s "$$destdir_from_execdir_SQ/$(bindir_relative_SQ)/$$p" "$$execdir/$$p" || \ - { test -z "$(NO_INSTALL_HARDLINKS)$(NO_CROSS_DIRECTORY_HARDLINKS)" && \ - ln "$$bindir/$$p" "$$execdir/$$p" 2>/dev/null || \ - cp "$$bindir/$$p" "$$execdir/$$p" || exit; } \ + ./ln-or-cp.sh \ + --install-symlinks "$(INSTALL_SYMLINKS)" \ + --no-install-hardlinks "$(NO_INSTALL_HARDLINKS)" \ + --no-cross-directory-hardlinks "$(NO_CROSS_DIRECTORY_HARDLINKS)" \ + --symlink-target "$$destdir_from_execdir_SQ/$(bindir_relative_SQ)/$$p" \ + "$$bindir/$$p" "$$execdir/$$p"; \ done; \ } && \ for p in $(filter $(install_bindir_programs),$(BUILT_INS)); do \ $(RM) "$$bindir/$$p" && \ - test -n "$(INSTALL_SYMLINKS)" -o "$(NO_INSTALL_HARDLINKS)" && \ - ln -s "git$X" "$$bindir/$$p" || \ - { test -z "$(NO_INSTALL_HARDLINKS)" && \ - ln "$$bindir/git$X" "$$bindir/$$p" 2>/dev/null || \ - cp "$$bindir/git$X" "$$bindir/$$p" || exit; }; \ + ./ln-or-cp.sh \ + --install-symlinks "$(INSTALL_SYMLINKS)" \ + --no-install-hardlinks "$(NO_INSTALL_HARDLINKS)" \ + --symlink-target "git$X" \ + "$$bindir/git$X" "$$bindir/$$p"; \ done && \ for p in $(BUILT_INS); do \ $(RM) "$$execdir/$$p" && \ if test -z "$(SKIP_DASHED_BUILT_INS)"; \ then \ - test -n "$(INSTALL_SYMLINKS)" -o "$(NO_INSTALL_HARDLINKS)" && \ - ln -s "$$destdir_from_execdir_SQ/$(bindir_relative_SQ)/git$X" "$$execdir/$$p" || \ - { test -z "$(NO_INSTALL_HARDLINKS)" && \ - ln "$$execdir/git$X" "$$execdir/$$p" 2>/dev/null || \ - cp "$$execdir/git$X" "$$execdir/$$p" || exit; }; \ + ./ln-or-cp.sh \ + --install-symlinks "$(INSTALL_SYMLINKS)" \ + --no-install-hardlinks "$(NO_INSTALL_HARDLINKS)" \ + --symlink-target "$$destdir_from_execdir_SQ/$(bindir_relative_SQ)/git$X" \ + "$$execdir/git$X" "$$execdir/$$p"; \ fi \ done && \ remote_curl_aliases="$(REMOTE_CURL_ALIASES)" && \ for p in $$remote_curl_aliases; do \ $(RM) "$$execdir/$$p" && \ - test -n "$(INSTALL_SYMLINKS)" -o "$(NO_INSTALL_HARDLINKS)" && \ - ln -s "git-remote-http$X" "$$execdir/$$p" || \ - { test -z "$(NO_INSTALL_HARDLINKS)" && \ - ln "$$execdir/git-remote-http$X" "$$execdir/$$p" 2>/dev/null || \ - cp "$$execdir/git-remote-http$X" "$$execdir/$$p" || exit; } \ + ./ln-or-cp.sh \ + --install-symlinks "$(INSTALL_SYMLINKS)" \ + --no-install-hardlinks "$(NO_INSTALL_HARDLINKS)" \ + --symlink-target "git-remote-http$X" \ + "$$execdir/git-remote-http$X" "$$execdir/$$p"; \ done && \ ./check_bindir "z$$bindir" "z$$execdir" "$$bindir/git-add$X" diff --git a/ln-or-cp.sh b/ln-or-cp.sh index de79cd85a81..663ffd0489d 100755 --- a/ln-or-cp.sh +++ b/ln-or-cp.sh @@ -1,8 +1,65 @@ #!/bin/sh +install_symlinks= +no_install_hardlinks= +no_cross_directory_hardlinks= +symlink_target= +while test $# != 0 +do + case "$1" in + --install-symlinks) + install_symlinks="$2" + shift + ;; + --no-install-hardlinks) + no_install_hardlinks="$2" + shift + ;; + --no-cross-directory-hardlinks) + no_cross_directory_hardlinks="$2" + shift + ;; + --symlink-target) + symlink_target="$2" + shift + ;; + *) + break + ;; + esac + shift +done + target="$1" +if test -z "$symlink_target" +then + symlink_target="$target" +fi link="$2" -ln "$target" "$link" 2>/dev/null || -ln -s "$target" "$link" 2>/dev/null || -cp "$target" "$link" +hardlink_or_cp () { + if test -z "$no_install_hardlinks" -a -z "$no_cross_directory_hardlinks" + then + if ! ln "$target" "$link" + then + cp "$target" "$link" + fi + + else + cp "$target" "$link" + fi +} + +main_with_fallbacks () { + if test -n "$install_symlinks" -o -n "$no_install_hardlinks" + then + if ! ln -s "$symlink_target" "$link" + then + hardlink_or_cp + fi + else + hardlink_or_cp + fi +} + +main_with_fallbacks
Refactor out the hard-to-read and maintain "ln || ln -s || cp" pattern. This was initially added in 3e073dc5611 (Makefile: always provide a fallback when hardlinks fail, 2008-08-25), but since then it's become a lot more complex as we've added: * 3426e34fedd (Add NO_CROSS_DIRECTORY_HARDLINKS support to the Makefile, 2009-05-11) * NO_INSTALL_HARDLINKS in 70de5e65e8c (Makefile: NO_INSTALL_HARDLINKS, 2012-05-02) * INSTALL_SYMLINKS in ad874608d8c (Makefile: optionally symlink libexec/git-core binaries to bin/git, 2018-03-13) * SKIP_DASHED_BUILT_INS 179227d6e21 (Optionally skip linking/copying the built-ins, 2020-09-21) Each of those commits had to add a new special-case to this code, resulting in quite an unmaintainable mess for adding any sort of new option. Let's use the newly introduced ln-or-cp.sh script instead, note that we only sometimes pass the --no-cross-directory-hardlinks option, per the previous behavior. The target of the "ln -s" is also another special snowflake, but we're careful to carry that forward. As in an earlier commit this also changes the behavior to emit any errors to stdout. In that earlier case that was done to simplify the script so that it can use one "ln -s" instead of the two, likewise we're now unconditionally emitting to stderr if ln (without -s, to create the hardlink) fails. We always emitted to stderr if "cp" failed. As in that earlier commit let's let that pass for now, yes it might be very verbose in some scenarios, but we're working our way towards a simpler end-state here. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- Makefile | 41 +++++++++++++++++----------------- ln-or-cp.sh | 63 ++++++++++++++++++++++++++++++++++++++++++++++++++--- 2 files changed, 81 insertions(+), 23 deletions(-)