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

Remove getent calls from /etc/bash.bashrc #146

Merged
merged 1 commit into from
Jul 12, 2017
Merged

Remove getent calls from /etc/bash.bashrc #146

merged 1 commit into from
Jul 12, 2017

Conversation

landstander668
Copy link
Contributor

This is my initial attempt to add getent to the Git for Windows package, in order to address issue #1226. It seems to work exactly as expected for the 64-bit release on Windows 10, but I want to do some additional validation (portable and SDK installers, as well as the 32-bit variants) before considering it ready to merge... I probably won't have an opportunity to do this until Sunday.

The only downside I see thus far is that it increased the size of the installer binary by approximately 10 MB, which is roughly 20 percent. This might be enough of a jump to be problematic for folks with less-than-ideal connectivity speeds.

An alternate, potential fix would be to include just /usr/bin/getent (along with any hard dependencies) instead of the full getent package. That will of course require more effort to implement and validate, and I don't know if GfW already has a precedent for that. So I'm currently unsure if this would be a good idea to pursue.

A simpler alternate fix would be to remove the getent calls from /etc/bash.bashrc. That would remove the ability to customize the prompt based on admin/non-admin user status (see this commit), when that status can actually be determined... it apparently requires group: db to be enabled, which is disabled by default in GfW. I assume this would require in-place editing of bash.bashrc during either the installer build or installation, however, since it's part of MSYS2-packages rather than being directly owned by GfW.

@dscho Do you have any preference as to the possible approaches?


Add getent to the list of pacman MSYS2 packages to be included in the
build. This utility is used, if available, by /etc/bash.bashrc in order
to customize the shell prompt to distinguish between admin and non-admin
users. It is not currently included in Git for Windows, however.

Normally its omission is not a problem, but when Cygwin is installed
and also in the PATH -- even if Git appears first -- then Git Bash will
encounter the following error during startup (from issue #1226).

1 [main] getent (14752) C:\cygwin\bin\getent.exe: *** fatal error - cygheap base mismatch detected - 0x1802FF408/0x180304408.

During test builds, this increased the size of the 64-bit installer by
approximately 10 MB (47,154 vs 37,848 MB).

Signed-off-by: Adric Norris [email protected]

@dscho
Copy link
Member

dscho commented Jul 7, 2017

A simpler alternate fix would be to remove the getent calls from /etc/bash.bashrc.

Yes, I would like that better. Something similar to b333b6a.

@landstander668
Copy link
Contributor Author

landstander668 commented Jul 7, 2017

@dscho Before I go down that route, can you tell me if there are any expected differences (code signing excluded) when manually building an installer? It turns out that the getent package is actually pretty small (around 100 KB, including dependencies), so I don't think there's any way it can explain the observed size increase by itself. I'm thinking I might have generated a build with additional debugging options, without realizing it.

If we can get the massive size increase resolved, I'm thinking that integrating getent might be the more easily maintained option. Primarily because it should be less fragile when upstream modifies /etc/bash.bashrc in the future.

That being said, I'll be happy to pursue the removal of getent from /etc/bash.bashrc if that's still your preferred approach.

@landstander668
Copy link
Contributor Author

In case there's any confusion, my build process was:

  1. Fresh install of the 64-bit SDK on a Windows 10 VM
  2. $ cd /usr/src/build-extra
  3. $ git fetch
  4. $ git checkout master
  5. $ git checkout -b getent
  6. Apply the single commit from this PR
  7. $ ./installer/release.sh 2.13.2-getent

@dscho
Copy link
Member

dscho commented Jul 9, 2017

I occasionally see a CI build resulting in a Portable git of 45MB (instead of the expected 35MB). I thought that I had resolved this issue by upgrading p7zip, but apparently not...

My preference is still to remove the getent call, and actually to find the time somewhere (there must be a rock under which I did not yet look, and where there is more time for me) to audit the entire profile/bash_profile for calls that simply make little sense in Git for Windows' context. I recently removed code to figure out the default printer, for crying out loud.

@landstander668
Copy link
Contributor Author

OK, I'll go ahead and proceed with removing the getent callse from /etc/bash.bashrc. Should have something usable for you in the next day or two, depending upon how the free time falls.

Incidentally, my build of the 64-bit portable installer ended up being a whopping 89 MB. No idea what's going on with that, especially since it was a fresh install of the SDK with no changes. I'll see if I can figure it out later... once the current PR is out of the way.

@landstander668 landstander668 changed the title WIP: Add getent to package list Remove getent calls from /etc/bash.bashrc Jul 11, 2017
@landstander668
Copy link
Contributor Author

@dscho I've pushed a new commit which removes the getent calls from /etc/bash.bashrc. I tried to be fairly strict with the pattern matching, to ensure that it won't inadvertently delete the wrong lines if upstream changes that section of the file. It appears to be be working as expected, so assuming that you're OK with the changes made I believe this PR is now in mergeable shape.

I'm still getting surprisingly large installers (48 MB for the standard 64-bit installer, and a whopping 89 MB for 64-bit portable) for some reason, but that's clearly unrelated to the commit in question.

end="$(($start+${line%%:*}+1))"
sed -ie "${start},${end}d" /etc/bash.bashrc
fi
fi

This comment was marked as off-topic.

@landstander668
Copy link
Contributor Author

You're clearly more familiar with sed than I am. :) I'll give it a try tonight, and then update the PR accordingly.

Remove calls to the "getent" utility, which is not currently included in
Git for Windows, from the system /etc/bash.bashrc file. Its presence is
not usually a problem, but when Cygwin is installed and also in the PATH
then Git Bash will encounter the following error during startup (see
issue #1226).

   1 [main] getent (14752) C:\cygwin\bin\getent.exe: *** fatal error - cygheap base mismatch detected - 0x1802FF408/0x180304408.

Signed-off-by: Adric Norris <[email protected]>
@landstander668
Copy link
Contributor Author

I'm getting closer. The current version (based upon your suggestion) seems to work perfectly if I run the commands manually from within the SDK. For some reason, however, the lines are still uncommented after building and installing a new installer.

I'm currently trying to figure out if git-extra.install is actually being used, or if some condition is precluding that.

@dscho dscho merged commit 6898266 into git-for-windows:master Jul 12, 2017
@dscho
Copy link
Member

dscho commented Jul 12, 2017

The current version (based upon your suggestion) seems to work perfectly if I run the commands manually from within the SDK. For some reason, however, the lines are still uncommented after building and installing a new installer.

I tested this locally and it worked...

I had to build & install a new git-extra package for that to work, which revealed a slight problem with this commit: typically, I prefer to edit only the .install.in files up until the time when a new git-extra package is generated (including the .install file), and there had been a previous change in the .in file that had not made it into a generated .install file yet.

In any case, this fix is part of git-extra 1.1.145.7d1d6ea.

Thank you so much!

@dscho
Copy link
Member

dscho commented Jul 12, 2017

FWIW the huge size may be due to unstripped binaries: by default, Git for Windows' SDK builds Git with debug information and installs those executables unstripped. If you do not reinstall the mingw-w64-x86_64-git package, those executables will increase the size of the archive/installer.

@landstander668
Copy link
Contributor Author

I had to build & install a new git-extra package for that to work, which revealed a slight problem with this commit: typically, I prefer to edit only the .install.in files up until the time when a new git-extra package is generated (including the .install file), and there had been a previous change in the .in file that had not made it into a generated .install file yet.

Ah, that explains it.

On a related note, thank you @dscho for taking the time to explain the recommended sed syntax. I believe that I have a better understanding of the tool now (past usage had always been rather rudimentary), which will hopefully benefit both this and future projects. :)

@landstander668
Copy link
Contributor Author

FWIW the huge size may be due to unstripped binaries: by default, Git for Windows' SDK builds Git with debug information and installs those executables unstripped. If you do not reinstall the mingw-w64-x86_64-git package, those executables will increase the size of the archive/installer.

I figured it was something like that, but hadn't tracked it all the way down yet. Thanx for the info!

@dscho
Copy link
Member

dscho commented Jul 13, 2017

@landstander668 you're welcome, and thank you for this lovely Pull Request!

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