Fix a pair of bugs for parallel (-jX) make install

[samueldr]

Hi!

See the commit bodies themselves for details. No changes to vim itself, only its build/install tooling.

In both instances, make -j24 in the build farm for Nixpkgs surfaced those spurious errors.

For the runtime install, I'm not entirely sure what the consequences of this failing at $(MAKE) VIMEXE=$(DEST_BIN)/$(VIMTARGET) vimtags would be. How broken would vim have been?

For the other issue, the consequences would be that spuriously (once to this date AFAICT) some of the additional tools that are symlinks to vim might be missing. For example ex is currently missing in one of our successful builds. (NixOS/nixpkgs#227677)

Don't hesitate to recommend alternatives to my exact changes.

There may be further parallelism issues, I don't have the full log to investigate on that one, and the few lines available may point to a red herring: NixOS/nixpkgs#224962 (comment).

[brammool]

See the commit bodies themselves for details. No changes to vim itself, only its build/install tooling. In both instances, `make -j24` in the build farm for Nixpkgs surfaced those spurious errors. For the runtime install, I'm not entirely sure what the consequences of this failing at `$(MAKE) VIMEXE=$(DEST_BIN)/$(VIMTARGET) vimtags` would be. How broken would `vim` have been? For the other issue, the consequences would be that spuriously (once to this date AFAICT) some of the additional tools that are symlinks to `vim` might be missing. For example `ex` is currently missing in one of our successful builds. (NixOS/nixpkgs#227677) Don't hesitate to recommend alternatives to my exact changes. There may be further parallelism issues, I don't have the full log to investigate on that one, and the few lines available may point to a red herring: NixOS/nixpkgs#224962 (comment). * Ensure vim is installed before installing runtime

I suppose you did this by adding "installvimbin" as a dependency to "installrtbase". That's not right, it should be possible to install Vim in some other way. And "installrtbase" should not have the side effect of installing more, see the explanation around line 128.

* Ensure DEST_BIN is present before installing tool symlinks

That should be OK.

…

-- Adam and Eve: First ones to ignore Apple terms & conditions. /// Bram Moolenaar -- ***@***.*** -- http://www.Moolenaar.net \\\ /// \\\ \\\ sponsor Vim, vote for features -- http://www.Vim.org/sponsor/ /// \\\ help me help AIDS victims -- http://ICCF-Holland.org ///

[samueldr]

I suppose you did this by adding "installvimbin" as a dependency to
"installrtbase". That's not right, it should be possible to install Vim
in some other way. And "installrtbase" should not have the side effect
of installing more, see the explanation around line 128.

This happens when only using parallelism by adding -j24 on make install in an isolated build. Since it's isolated, there is no "previous" vim already installed.

The current installation steps refer to the DEST_BIN vim, which even outside entirely isolated builds, may not have been installed yet, or is being written to (VIMEXE=$(DEST_BIN)/$(VIMTARGET)). Since the target refers to that path, it must depend on it to satisfy dependencies.

I'll note that it's explained in lines 353-354 of runtime/doc/Makefile that the target “Can only be used when Vim has been compiled and installed.”

• vim/runtime/doc/Makefile

  Lines 353 to 354 in 7b4164e

  	# Use Vim to generate the tags file. Can only be used when Vim has been
  	# compiled and installed. Supports multiple languages.

I'm not that familiar with arcane Makefile features, but I don't think there's a way to prevent or block a target from being used before another one is ran, but not depending on the target, right? (Anyway it wouldn't make sense for the dependency graph.)

Without depending on installvimbin, we can't make the build depend on the $(DEST_BIN)/$(VIMTARGET) path:

make[1]: *** No rule to make target '.../bin/vim', needed by 'installrtbase'.  Stop.

(Anyway it would technically amount to doing part of installvimbin if it worked...)

The alternative I can see would be to refer to the vim executable in the build directory, which would be presumed to exist at that point. It's a bit kludgey since the build cds into another directory; we need to remember where we were so the relative VIMTARGET becomes absolute.

The change on top of my current changes:

diff --git a/src/Makefile b/src/Makefile
index a82a46e49..131188a59 100644
--- a/src/Makefile
+++ b/src/Makefile
@@ -2329,7 +2329,7 @@ installruntime: installrtbase installmacros installpack installtutor installspel
 
 # Install the help files; first adjust the contents for the final location.
 # Also install most of the other runtime files.
-installrtbase: $(HELPSOURCE)/vim.1 $(DEST_VIM) installvimbin $(DEST_RT) \
+installrtbase: $(HELPSOURCE)/vim.1 $(DEST_VIM) $(VIMTARGET) $(DEST_RT) \
                $(DEST_HELP) $(DEST_PRINT) $(DEST_COL) \
                $(DEST_SYN) $(DEST_SYN)/shared $(DEST_IND) \
                $(DEST_FTP) $(DEST_AUTO) $(DEST_AUTO)/dist $(DEST_AUTO)/xml \
@@ -2342,8 +2342,8 @@ installrtbase: $(HELPSOURCE)/vim.1 $(DEST_VIM) installvimbin $(DEST_RT) \
        cd $(HELPSOURCE); if test -z "$(CROSS_COMPILING)" -a -f tags; then \
                mv -f tags tags.dist; fi
        @echo generating help tags
-       -@cd $(HELPSOURCE); if test -z "$(CROSS_COMPILING)"; then \
-               $(MAKE) VIMEXE=$(DEST_BIN)/$(VIMTARGET) vimtags; fi
+       -@BUILD_DIR="$$PWD"; cd $(HELPSOURCE); if test -z "$(CROSS_COMPILING)"; then \
+               $(MAKE) VIMEXE=$$BUILD_DIR/$(VIMTARGET) vimtags; fi
        cd $(HELPSOURCE); \
                files=`ls *.txt tags`; \
                files="$$files `ls *.??x tags-?? 2>/dev/null || true`"; \

[brammool]

> I suppose you did this by adding "installvimbin" as a dependency to > "installrtbase". That's not right, it should be possible to install Vim > in some other way. And "installrtbase" should not have the side effect > of installing more, see the explanation around line 128. This happens when only using parallelism by adding `-j24` on `make install` in an isolated build. Since it's isolated, there is no "previous" vim already installed.

I understand that. The solution to add "installvimbin" is wrong though.

The current installation steps refer to the `DEST_BIN` vim, which even outside entirely isolated builds, may not have been installed yet, or is being written to (`VIMEXE=$(DEST_BIN)/$(VIMTARGET)`). Since the target refers to that path, it *must* depend on it to satisfy dependencies. I'll note that it's explained in lines 353-354 of `runtime/doc/Makefile` that the target “Can only be used when Vim has been compiled and installed.” - https://github.com/vim/vim/blob/7b4164ee14c546edecdcfcb3b4cdfaad0dd9d0f3/runtime/doc/Makefile#L353-L354 I'm not that familiar with arcane Makefile features, but I don't think there's a way to prevent or block a target from being used before another one is ran, but not depending on the target, right? (Anyway it wouldn't make sense for the dependency graph.)

Right. The Makefile was originally written to be used with non-parallel build. Support was added later to make a parallel build possible, but the Makefile syntax/semantics is/are not sufficient to make this work in corner cases.

Without depending on `installvimbin`, we can't make the build depend on the `$(DEST_BIN)/$(VIMTARGET)` path: ``` make[1]: *** No rule to make target '.../bin/vim', needed by 'installrtbase'. Stop. ```

That actually sounds correct, it tells the user that this target won't work without installing Vim first. But it's inconvenient.

(Anyway it would technically amount to doing part of `installvimbin` if it worked...)

Or whatever other way the user wants to use to install Vim, which might be something completely different.

The alternative I can see would be to refer to the vim executable in the build directory, which would be presumed to exist at that point. It's a bit kludgey since the build `cd`s into another directory; we need to remember where we were so the relative `VIMTARGET` becomes absolute. The change on top of my current changes:

This uses ***@***.***_DIR="$$PWD"; cd $(HELPSOURCE); ... Does this work everywhere? Note that the Makefile isn't written for any specific version of make, it should work with any version. We especially don't want to depend on GNU-make specific features (e.g. it should work with BSD make).

…

-- A year spent in artificial intelligence is enough to make one believe in God. /// Bram Moolenaar -- ***@***.*** -- http://www.Moolenaar.net \\\ /// \\\ \\\ sponsor Vim, vote for features -- http://www.Vim.org/sponsor/ /// \\\ help me help AIDS victims -- http://ICCF-Holland.org ///

[samueldr]

Does this work everywhere? Note that the Makefile isn't written for any
specific version of make [...]

Most of the load-bearing change here is handled by the shell used by make.

The POSIX make documentation states:

The macro $$ shall be replaced by the single character '$'

— https://pubs.opengroup.org/onlinepubs/009695299/utilities/make.html

Anyway $$ is used elsewhere in the same target. So this part is a non-issue.

Then as long as the shell provides $PWD (as POSIX shell states it should) it would work as authored. Since I can't know what other obscure platforms would do here, "pwd" (literal with backticks) can be used instead if you prefer.

[brammool]

> Does this work everywhere? Note that the Makefile isn't written for any > specific version of make [...] Most of the load-bearing change here is handled by the shell used by make. The POSIX make documentation states: > The macro $$ shall be replaced by the single character '$' — https://pubs.opengroup.org/onlinepubs/009695299/utilities/make.html Anyway `$$` is used elsewhere in the same target. So this part is a non-issue. Then as long as the shell provides $PWD (as POSIX shell states it should) it would work as authored. Since I can't know what other obscure platforms would do here, "`pwd`" (literal with backticks) can be used instead if you prefer.

I'm not 100% sure $PWD works in every shell, while `pwd` should work everywhere. I'm a little bit worried about having spaces in the path. I guess adding quotes like this should help (haven't tried it yet): $(MAKE) VIMEXE="$$BUILD_DIR/$(VIMTARGET)" vimtags; fi

…

-- A fool must search for a greater fool to find admiration. /// Bram Moolenaar -- ***@***.*** -- http://www.Moolenaar.net \\\ /// \\\ \\\ sponsor Vim, vote for features -- http://www.Vim.org/sponsor/ /// \\\ help me help AIDS victims -- http://ICCF-Holland.org ///

[samueldr]

I'm not 100% sure $PWD works in every shell, while pwd should work everywhere. I'm a little bit worried about having spaces in the path. I guess adding quotes like this should help (haven't tried it yet): $(MAKE) VIMEXE="$$BUILD_DIR/$(VIMTARGET)" vimtags; fi

Yes, adding quotes is a good catch. Added that while force-pushing the requested changes.