Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

git-for-windows docs #60

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

r2evans
Copy link

@r2evans r2evans commented Feb 3, 2020

Documentation and script to create an installation script for git-bash (within git-for-windows), as a suggestion for #57.

This leverages on the variables and install: section of the existing Makefile, so it should update if/when the package itself changes.

I've not included the install_gfw.sh file for now, since I'm assuming you'd prefer it to not be the default for all users. However, it's rather small and can otherwise be ignored (or used for that matter). The script it creates is currently:

PREFIX=${PREFIX:-/usr/local}
BINPREFIX=${BINPREFIX:-"${PREFIX}/bin"}
LIBPREFIX=${LIBPREFIX:-"${PREFIX}/lib"}
MANPREFIX=${MANPREFIX:-"${PREFIX}/share/man/man1"}
SYSCONFDIR=${SYSCONFDIR:-${PREFIX}/etc}
mkdir -p ${DESTDIR}${MANPREFIX}
mkdir -p ${DESTDIR}${BINPREFIX}
mkdir -p ${DESTDIR}${LIBPREFIX}/git-issue
install git-issue.sh ${DESTDIR}${BINPREFIX}/git-issue
install lib/git-issue/import-export.sh ${DESTDIR}${LIBPREFIX}/git-issue/import-export.sh
install -m 644 git-issue.1 ${DESTDIR}${MANPREFIX}/
mkdir -p ${DESTDIR}${SYSCONFDIR}/bash_completion.d
install -m 644 gi-completion.sh ${DESTDIR}${SYSCONFDIR}/bash_completion.d/git-issue

This script has a few caveats/disclaimers:

  • even though it does not start with the hash-bang header line, git-bash will still work, so the following (on GfW) both work:

    PREFIX=/some/path sh install_gfw.sh
    PREFIX=/some/path ./install-gfw.sh
  • in GfW, the default prefix of /usr/local will not work, due to directory virtual mounts; I mention this in the docs, but it might be a commonly (perhaps frequently) asked question to the maintainer(s); we can easily define an alternate location to override the current default just in this installation file

  • in GfW, SYSCONFDIR is already defined (or at least it is on mine) to /etc; as long as the user installs with ./install_gfw.sh or sh install_gfw.sh, then this envvar is not inherited and the bash-completion script will be installed in ${PREFIX}/etc; if, however, they decide to go with . install_gfw.sh (using . for source, not suggested in the docs), then they will receive an error

    mkdir: cannot create directory ‘/etc/bash_completion.d’: Permission denied
    

    The fix to this error is to define SYSCONFDIR (and/or DESTDIR). Or to not use source or ..

  • this script can be run repeatedly, it does not fail or behave differently if the target directories and/or files already exist

General notes:

  • I think the logic for converting make's conditional variable assignment (?=) is applied generally-enough to bash's (:-), but I'm sure it's possible to engineer incompatible make recipes. If you complicate your Makefile preamble, this script might no longer work.

  • Similarly, it finds all installation-related code by finding the literal install: block and stops parsing when it finds the next line with a non-space in the first character. This means an inadvertent comment will be problematic:

    install:
    	@mkdir -p $(DESTDIR)$(MANPREFIX)
    # something here
    	@mkdir -p $(DESTDIR)$(BINPREFIX)

    This would cause the second mkdir to be missed. (I don't know that this is legal for make, to be honest, just thought I'd state the parsing assumption.) This current method appears to handle blank lines and indented comments.

  • On my system, this passes all tests except test 1, which fails due to CRLF issues. Also, tests 1 and 77 warn about the same line-ending problem, which I believe is safe to ignore:

    warning: LF will be replaced by CRLF in ...
    The file will have its original line endings in your working directory
    

    (I believe that the preferable config in windows is core.autocrlf = true, I might be wrong.)

@dspinellis
Copy link
Owner

Thank you! The sed code is very clever! I'm not familiar with Git for Windows. I'm a bit hesitant to massage the Makefile with code almost as long as the script it creates. Wouldn't it be easier to ask users either to install make (and tweak the Makefile as required) or to provide directly the installation script? In the latter case we would need a comment in the Makefile to remind maintainers to also update the Git for Windows script. Note that the make dependency is wider (for example it's not installed by default under Termux), it just hasn't cropped up yet for other users.

@r2evans
Copy link
Author

r2evans commented Feb 3, 2020

GfW is really just meant to do one thing: provide git.exe and all of the tools required to support it. That means unzip (but not zip ...), tar, etc, and enough unixy utilities so that user/group/passdb stuff works as needed. In reality, though, it's really just for git. It also provides bash so that if the user does not have a GUI, they can happily use git on a command-line that git-developers suggest is natural ... though it can be used within a Windows cmd window and PowerShell without change, I think. GfW is not intended to be anything near a development platform, unlike cygwin (which is significantly larger) nor a light VM (WSL).

Asking users (or downstream package maintainers) to replace the GfW installation with cygwin is analogous to replacing an installation of vi with Microsoft Office: sure, the basic premise of editing a text file is covered, but it adds dozens and dozens of otherwise unnecessary dependencies, complexity, and learning-curve for maintenance. And cygwin requires significantly more maintenance than GfW (which requires just simple upgrades).

Asking users (...) to replace GfW with a virtual machine (WSL) is similarly exceedingly asymmetric.

I haven't suggested any changes to the Makefile, so there's no massaging it (I must be misunderstanding your point). As long as its structure remains relatively unperturbed, you're good. While I had assumed that you were explicitly maintaining it, as long as its structure does not change then it does not matter if it is a manual-edit or generated via something else (though I do not see reference to a ./configure script or similar mechanism for makefile-generation).

If your make dependency is a wider problem than just GfW, perhaps it would be justified to include install_gfw.sh, renamed to remove the implied reliance on GfW. This does suggest that there needs to be another default for PREFIX that is either (1) more universally standard, or (2) at least will fail less. Since outside of windows I find /usr/local a perfectly acceptable default, maybe GfW is the sole exception to that rule with a simple heuristic for determining a sane alternative for PREFIX.

@dspinellis
Copy link
Owner

Sorry, I wasn't clear regarding massaging. I meant processing the Makefile. I understand how GfW is used,, and I wasn't proposing to ask users to switch. I (probably mistakenly) thought it had a package manager that would allow the easy installation of make.

I like your suggestion for a universal install.sh script with a suitable GfW heuristic. Let's have that. If you want to generate it from the Makefile (via make -n install | sed or via your original sed script) I'm also fine, but let's keep that generation code in a separate file, not in sync-docs.sh. Feel free to add demarcation comments in the Makefile to make your job easier if needed.

@r2evans
Copy link
Author

r2evans commented Feb 3, 2020

That's a very relevant point: GfW has no package management, perhaps that gives you some perspective to understand why it is both "good" (simple) and not extensible. (I apologize if my points came across as pedantic or patronizing. I often forget that not everybody is cursed as I am to use GfW as my best-available shell.)

Your suggestion to use make -n install is good in that it uses make to parse the Makefile and the effective values of the variables. For instance, it's relatively straight-forward to use this:

$ PREFIX=QUUX make -n install
mkdir -p "QUUX/share/man/man1"
mkdir -p "QUUX/bin"
mkdir -p "QUUX/lib"/git-issue
install git-issue.sh "QUUX/bin"/git-issue
install lib/git-issue/import-export.sh "QUUX/lib"/git-issue/import-export.sh
install -m 644 git-issue.1 "QUUX/share/man/man1"/
mkdir -p QUUX/etc/bash_completion.d
install -m 644 gi-completion.sh QUUX/etc/bash_completion.d/git-issue

to generate an install script that regains the ability to set a prefix dir, though we do lose the ability to control separate bin, lib, sysconfdir directories. (I don't see that as a huge problem.) This needs to be run on the dev system, though, not the user's system.

I think the assumptions for Makefile-parsing are good-enough that demarcation comments won't add much value/safety. (Rules: (1) preamble contains variables; (2) blocks are demarcated by first-column-labels; (3) indented code in a block is what we want, removing the leading @.)

I can see adding some logic to determine a reasonable PREFIX, though, perhaps based on some quick trial-and-error: if it is /usr/local and that fails, then try a GfW-friendly approach (tbd); iterate through a list of other OSes. This logic could really go in the Makefile itself (and therefore used by all installations, conditionals notwithstanding).

I can't work on it now, I'll look at it tomorrow. I have some thoughts.

@dspinellis
Copy link
Owner

Excellent, thank you for the clarifications! Given the simplicity goal of GfW, bundling a gfw-install.sh script that simply works could probably be the best approach. Based on your suggestion, consider a Makefile rule (for dev execution / refresh), such as the following.

gfw-install.sh: Makefile
        make -n PREFIX=QUUX >$@

@r2evans
Copy link
Author

r2evans commented Feb 4, 2020

@dspinellis, the guidance is clear, and ... GfW is messing with things a little.

Using the simple make -n from above, I can get it and all of the companion files installed just fine. As what I think is a reasonable default, I chose $HOME/AppData/Roaming/git-issue as a good default for installation.

Unfortunately, windows is thwarting things a little. If you ignore the /c/Rtools/bin/make I have to use to test this locally (it is not adversely affecting the install-script contents), then it's the last failure I'm looking at:

r2@d2sb2 MINGW64 ~/Projects/git-issue/_57_gfw_install (fix/57_gfw_install)
$ /c/Rtools/bin/make gfw-install.sh
c:/Rtools/bin/make -s -n PREFIX=DOLLARHOME/AppData/Roaming/git-issue > gfw-install.sh
echo "git config --global alias.issue '!'\"DOLLARHOME/AppData/Roaming/git-issue/bin/git-issue\"" >> gfw-install.sh
sed -i -e 's/DOLLARHOME/\$HOME/g' gfw-install.sh

r2@d2sb2 MINGW64 ~/Projects/git-issue/_57_gfw_install (fix/57_gfw_install)
$ sh gfw-install.sh

r2@d2sb2 MINGW64 ~/Projects/git-issue/_57_gfw_install (fix/57_gfw_install)
$ git issue
No git-issue directory in path .
/Users/r2/AppData/Roaming/git-issue/bin/../lib::/usr/lib:/usr/local/lib

In a non-windows system, the /Users/r2/.../bin/../lib/ would resolve just fine. Unfortunately, the way GfW (and cygwin, I believe) provides the c:/ drive mounted as /c/, this fails.

So while the make -n works just fine in creating it, the script itself is not working perfectly. I think this PR should probably be put on hold until that is fixed.

@dspinellis
Copy link
Owner

The error comes from git-issue.sh line 68.

  • It looks like the LIB_PATH is set to .\n/Users/r2/AppData/Roaming/git-issue/bin/../lib::/usr/lib:/usr/local/lib. Is this the case? How could the newline have crept int?
  • Try adding echo executable is [$0] in line 67. What is reported?
  • What is the output of ls /Users/r2/AppData/Roaming/git-issue/bin/../lib?

@r2evans r2evans mentioned this pull request Feb 4, 2020
@r2evans
Copy link
Author

r2evans commented Feb 4, 2020

See my comments in #64, as I believe fixing that makes the rest of this PR easy.

If you'd like, I can include the fix from that issue in this PR. Or a separate PR. Up to you :-)

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