Skip to content

fix(Makefile): use correct git shell path and add semicolons#1346

Merged
L3MON4D3 merged 2 commits intoL3MON4D3:masterfrom
xudyang1:fix/win-shell
Jul 6, 2025
Merged

fix(Makefile): use correct git shell path and add semicolons#1346
L3MON4D3 merged 2 commits intoL3MON4D3:masterfrom
xudyang1:fix/win-shell

Conversation

@xudyang1
Copy link
Copy Markdown
Contributor

Problem:

PR #1343 did not use the correct git shell path as the space is not escaped.

Solution:

  1. Adding a backslash to escape the space does not fix the issue as SHELL still expands to sh.exe even after assignment. Instead of directly copying $(WIN_GIT_SHELL), use $(wildcard $(WIN_GIT_SHELL)) returned from the Makefile function to overwrite SHELL.

  2. Similar to PR feat(Makefile): enable auto detect compiler #1282 on $(shell %COMMAND%), semicolons are added after each command to make sure make can call CreateProcess correctly:

  • Without semicolon: calls CreateProcess(NULL,%COMMAND%,...)
  • With semicolon: calls CreateProcess(NULL,$(SHELL) $(.SHELLFLAGS) "%COMMAND%;",...)

@L3MON4D3
Copy link
Copy Markdown
Owner

Hi!
Would you add a comment which explains why (or mentions that) the CreateProcess fails with multiple lines? It might be forgotten when creating new build steps.

@xudyang1
Copy link
Copy Markdown
Contributor Author

@L3MON4D3 I would like to postpone merging this PR to master. Just need more time to fix the :checkhealth luasnip bug (#1341). Thank you for your consideration.

@L3MON4D3
Copy link
Copy Markdown
Owner

L3MON4D3 commented Jun 2, 2025

Of course :)

@xudyang1
Copy link
Copy Markdown
Contributor Author

xudyang1 commented Jul 3, 2025

@L3MON4D3 I believe this PR is ready to be merged, as another user has provided positive feedback: #1220 (comment).

Also, what do you think about switching from Makefile's $(shell ....) function to the shell's native command substitution in the following section? I have not verified this change, so I'm leaving it as-is for now.

diff --git a/Makefile b/Makefile
index fff60df..9205201 100644
--- a/Makefile
+++ b/Makefile
@@ -157,8 +157,8 @@ test_nix: nvim install_jsregexp
 spellcheck:
 # grabbed from word-warden.
 	# Misspelled words will appear inside the following tests:
-	if [ -n "$(shell aspell --home-dir . --encoding=utf-8 --mode markdown --lang en_US --personal ./data/project-dictionary.txt list < README.md)" ]; then exit 1; fi;
-	if [ -n "$(shell aspell --home-dir . --encoding=utf-8 --mode markdown --lang en_US --personal ./data/project-dictionary.txt list < DOC.md)" ]; then exit 1; fi;
+	if [ -n "$$(aspell --home-dir . --encoding=utf-8 --mode markdown --lang en_US --personal ./data/project-dictionary.txt list < README.md)" ]; then exit 1; fi;
+	if [ -n "$$(aspell --home-dir . --encoding=utf-8 --mode markdown --lang en_US --personal ./data/project-dictionary.txt list < DOC.md)" ]; then exit 1; fi;
 
 spellcheck_interactive:
 	aspell --home-dir . --encoding=utf-8 --mode markdown --lang en_US --personal ./data/project-dictionary.txt check DOC.md;

@L3MON4D3
Copy link
Copy Markdown
Owner

L3MON4D3 commented Jul 6, 2025

@L3MON4D3 I believe this PR is ready to be merged, as another user has provided positive feedback: #1220 (comment).

Wonderful, thank you for managing that ❤️

Also, what do you think about switching from Makefile's $(shell ....) function to the shell's native command substitution in the following section? I have not verified this change, so I'm leaving it as-is for now.

I think I actually considered using $$(aspell ) when writing that: aspell prints misspelled words in this instance, and if we use the makefile's substitution at this place, the words will appear in the output, not the command that was used to get them:

screen

This shows the output when there is a misspelled word, top is $$(aspell ...), bottom is $(shell ...). IMO bottom is better because there is more info (if you know how to read it).

I'll merge this for now, Thank You for continuing your support-work for windows!
If you'd like to discuss $$(aspell ...) further, feel free to open another PR :)

@L3MON4D3 L3MON4D3 merged commit a6afde6 into L3MON4D3:master Jul 6, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants