Closed fourpastmidnight closed 4 years ago
@robmen Glad to see you back. I have a fork over on my GitHub. I haven't created a pull request yet and it's not current with my local workspace. I've been at work on the UI, but I haven't committed anything yet.
However, it's at a point where I have two more pieces of UI functionality to complete, and then we get to the hard stuff (for me, anyway): Implementing the "setup options" for reinstall/repair/update scenarios. IOW, having the right conditions on the Git For Windows-specific dialogs to behave correctly for various supported installation modes.
So I'm going to pause, get my local repo cleaned up and commit and push what I have completed. Then I'll update my branch with master and issue a pull request. (The latest major change that impacts the installer that I have seen is the inclusion of Git Credential Manager for Windows--and I have no UI support for it yet, but I don't anticipate any complications adding that support.)
Once I issue a pull request, I'll outline where I am and what I believe is still outstanding. Then we can evaluate the current state of affairs and determine how to proceed.
@fourpastmidnight if I promise not to judge you on the quality of a topic branch that is in flight, would you mind committing early and often and opening a [DO NOT MERGE YET]
Pull Request?
For the record: I frequently to the same, committing a lot of fixup!
commits (and sometimes, when I run out of time, I simply commit everything at the end of the day in one huge messy commit with the commit message "wip" and push, just so I have a backup and a known state of affairs).
100% agreed.
Also, smaller issues to tackle would help as well. Right now we're looking at potential convoy problems.
@dscho: Deal. It's very rough and needs a lot of cleanup, but most of the "heavy-lifting" is done.
Question for you: In order to implement allowing the user to choose PuTTY as the default SSH client (#462), I needed to create two MSI custom actions (they only read system state, @robmen <grin />). I wrote them in C++ and they require that at least the Windows SDK for Vista (or above) be installed to compile the custom actions.
So, having said that, I don't really think this fits into the Git for Windows SDK development workflow. It's not an insignificant dependency and I'm not sure we want this dependency. Let me know if you agree. If so, I won't even bother committing this work and we can close #462 (or keep it closed if it already is--I forget it's current status).
Lastly, I won't get to this until later tonight.
@dscho, @robmen I created PR113.
You can read through the commit messages (especially for this commit).
The main takeaways are:
release.sh
and build the MSIThanks!
One more thing I forgot to mention in the commit comments regarding the UI (I can update the pull request notes, too):
Some of the custom dialogs for git options are arranged so that the dialogs will work with accessibility devices (e.g. screen readers). I have to admit, though, that I haven't tested these dialogs with such assistive technology yet, as I had pretty much only yesterday (technically, it's 3am, so it would be two days ago) wrapped up what you see in that Pull Request.
Alright, I'm off to get what little sleep I can get before I need to wake up for work today!
@fourpastmidnight cool to see the UI started. I dislike dealing with MSI UI so I'm glad you've got the bulk of it started there. You might try rebasing off git-for-windows/build-extra#112 since that will get you the minimal MSI that installs Git properly.
@fourpastmidnight I'd like to merge git-for-windows/build-extra#112 because it is so well-contained, any objections?
Well, I guess that sounds good to me; @robmen is pretty much the man when it comes to installers.. I'm just happy to be able to contribute and learn.
Merged it.
@robmen, I could try rebasing off of PR112--though I'm not sure how I would do that on GitHub... I'm a bit new to this platform and haven't contributed much to other open source projects.
Also, because I've effectively "shared" my branch, is it ok for me to do a rebase?
Oh, and @dscho, sorry, I forgot to "sign-off" my PR. But when the final PR is ready, I'll try to remember to do that (or I'm sure you'll slap my wrist... ;) j/k ).
Or, I could try rebasing off of the merge that just happened ;) -- that is, if rebasing is alright.
Also, because I've effectively "shared" my branch, is it ok for me to do a rebase?
Absolutely. Pull Requests are based on branches with the intention of rewriting the branches until they are good to be merged.
Rebasing is alright!
Thanks!! That's why I haven't done any rebasing to fix the "divergence" of my branch--I wasn't sure if that was OK.
With that, then, I could also add the support for the Git Credentials Manager for Windows, too.
It's absolutely okay. As the original author of the interactive rebase, I would also be delighted if you used that tool :-)
@dscho: qq -- after rebasing, when I want to push the rebased branch, do I have to do a forced push?
Yes, you will have to force the push (I usually call git push my-remote +HEAD
) because you want GitHub to forget the exact commits you had before, and GitHub does not like to forget without you asking it explicitly to do so. :smile:
@dscho: Ok, that's what I thought. Thanks for the tips!!
I found a good write-up on git-rebase --interactive
-- I see why you want me to use it :) There are a few commit messages I wouldn't mind rewording/adding to anyway to make it more clear what's in the commit.
I'll work on getting my branch current and repush it up and issue a new pull request(?) later on tonight.
Hopefully I can continue to push this forward over the weekend.
@robmen: let me know if you're taking anything on or if there's anything you see in my work that needs to change so we don't step on each other's toes. I see a light at the end of the tunnel and I'm excited that we have gotten this far.
Instead of a new PR, just force-push the branch; it will update the Pull Request automagically.
I found a good write-up on git-rebase --interactive
That is really a nice write-up. I linked to it in our How to participate wiki page.
@dscho: Not terribly important, but when you have a minute, I'd appreciate any comments/thoughts you have on what I've done towards #462 and whether or not you feel that fits well with Git for Windows long-term.
I'm leaning towards let's not do it due to the dependency on the Windows SDK. Also, as I've already mentioned, there's a lot of information online that's available that shows how to setup Git for Windows to use PuTTY if someone really wants to do that. So I'm not really sure that we need to concern ourselves with that. Git for Windows provides a solution out of the box via OpenSSH. And with the recent inclusion of Git Credentials Manager for Windows, the UX is really nice. (I've been using it on my work laptop and the UX is seamless and completely transparent. I love it!)
Instead of a new PR, just force-push the branch; it will update the Pull Request automagically.
Be sure to leave a comment mentioning it afterward, however, because it was mentioned in another PR that dscho won't get notified automatically when the branch is re-pushed.
@fourpastmidnight I'm not going to take a position on whether or not the use of PuTTY should be retained as an alternative to OpenSSH. For what it's worth, however, ssh-pageant support was recently integrated and should be present in the next release. This lets OpenSSH transparently use Pageant (the PuTTY authentication agent) in place of ssh-agent for key-based authentication.
I suspect that key management was a big driver for people wanting/needing to use PuTTY. If so, this should the switch to using OpenSSH relatively painless.
@landstander668 Even more reason to not bother messing with supporting (per se) PuTTY in the installer itself the way it used to--you already have a solution that will "bridge that gap".
Having said that, though, will there be any installer support needed for your solution in order to configure Git for Windows to use this "bridge", or is this simple enough that a user can configure this post-installation? Perhaps a better question is, has the current InnoSetup installer been modified to support this? If the answer is yes, then the MSI needs to follow suit.
@robmen I was really scratching my head on how to get the user's "personal folder" for the working directory for a per-machine installation! I couldn't get around those ICE errors. This solution is genius! OK, you just happen to know a lot more about MSI than I do, but this is something I've wondered about for a long time over the years. I'm glad I finally know how to do this.
@robmen Reading over the documentation, though, I see a potential problem with this solution. Won't this result in the Start menu shortcuts for a per-machine installation having their Working Directory set to the installing user's home directory (e.g. C:\Users\installing_user
)? If so, doesn't that also imply that for all users on the machine who wish to use Git for Windows, the Working Directory will be pointing to the installing user's home directory, and not the logged on user's home directory? If this is the case, this could be problematic since in most (read almost all) environments, users don't have access to one another's home directories.
In which case, would it then be better to author the shortcuts the way I had authored them over on my branch (since it uses the --cd-to-home
command line switch of git-bash.exe
in the Arguments field of the shortcut)?
@robmen: I just built the installer based off of master and installed it onto my test VM. Setting the shortcut working directory to PersonalFolder
inside of the WiX XML will result in the Start menu shortcuts for Git Bash and Git CMD having a working folder set to the installing user's "My Documents" folder (C:\Users\GfwUser\Documents
is the path Working Directory was set to on my test VM).
Furthermore, when I created a second user on this machine (with non-admin privileges) called GfwUser2
and switched to that user, the Working Directory for the shortcut still remained C:\Users\GfwUser\Documents
as I suspected it would. And when starting Git Bash, because GfwUser2
doesn't have access to GfwUser
's personal folder, Git Bash's working directory becomes C:\Windows\System32
, which is what we don't want.
All in all, unfortunately, the use of PersonalFolder
is not having the desired outcome. However, the shortcuts that I authored do work properly and also present no ICE validation errors upon building the MSI. They don't attempt to set the working directory and rely on setting the Argument property of the shortcuts to use the option --cd-to-home
which makes sure that Git Bash starts in the user's home directory, for example (not their Documents folder, but their home directory, i.e. C:\Users\<username>
).
So, here's my list of what I'd like to accomplish, in priority order:
.git*
files (excluding .gitconfig
) with the system default text editor*.sh
files with git-bash.exe
release.sh
to add @BindPath
attribute to all <File />
elements generated in GitComponents.sh
for the mingw-w64-$ARCH-git
package.[x] Look into using a Feature Tree selection dialog instead of all of these one-off dialogs. This will be a lot easier to maintain in the future.
This doesn't seem like a good idea to me at this time (the "Feature Tree" aspect, that is). Git for Windows doesn't have "features" so much as it has "configurable configurations" that can be chosen at the time of installation.
Lastly, the WixUIAdvanced dialog set may make more sense, but until we have this installer actually working (i.e. using the chosen configurations to actually configure Git for Windows during installation), I'm happy to let this pass for the time being and revisit this once things settle down a bit.
release.sh
to take a localization flag [OPTIONAL]
en-us
if left unspecified.candle.exe
and light.exe
calls in release.sh
Git-2.8.1-64-bit.msi, it should become
Git-2.8.1-en-us-64-bit.msi`I think this list is a fairly comprehensive list of things that need to be completed before we can release this thing into the wild.
My first order of business will be rebasing my branch on top of the latest commits on master and updating pull request 113. From there, I'll start working down this list. If anyone wants to jump in somewhere, please just let me know what you're picking up.
Yes, @fourpastmidnight, you're right about the PersonalFolder
for other users. I should have stopped coding after 2am. The arguments method should work well.
@robmen No worries. Looking over some of the other changes in PR112, I've already learned a lot about MSI--even things I read over at the FireGiant Wix documentation that I glossed over because I didn't know it applied. For instance, on the post-install.bat
and setting @Guid=""
: I get it! I saw the description in the documentation, but didn't understand until associating the behavior with what happens with this file during installation of Git for Windows.
I have one more question regarding the way you authored the shortcuts: I see that you authored them not as child elements of their target <File />
, but rather as siblings within the parent <Component />
element. I'm wondering about the behavior of the installer during repair scenarios. My understanding of components is that they are installed and removed atomically. And the determination for whether or not a component needs repairing or installing depends upon whether or not Windows installer finds the installed KeyPath for the component (whether that be a file, directory, or registry entry). So what happens, say, if some unscrupulous user with the right permissions deletes one of the Start Menu shortcuts (for example's sake, let's say Git Bash shortuct) the way you've authored the MSI?
If my understanding is correct and complete, because the KeyPath of the component still exists on disk (git-bash.exe
), then a repair of the installation of Git for Windows would not result in the Start Menu shortcut being restored. However, (and this is where I'm unsure if my understanding is correct or complete), if the Shortcut is nested within the <File />
element (for git-bash.exe
for example), then a repair install would restore the shortcut, ostensibly because even though the file exists on disk, the shortcut nested witin the <File />
element does not; and since the entire <File />
element is the KeyPath for the component, it must be damaged, and therefore, need reinstallation. Am I correct?
Actually, I think I'll just test it out--seems like a fun learning opportunity. I'll report back with my findings.
@robmen: Ah!! It did put it back upon repair. I guess my current understanding was incomplete--but now it's more complete than it was. Cool!
And with that, the first item on my list above is checked off--my installer is now using the original make-file-list.sh
script.
@dscho I've only ever used git rebase --interactive
once before today. And at that time, I was blindly following what a friend told me to do--I had no idea what I was doing or why. But, after reading that write-up, looking over my commit history and seeing what I wanted to do: WOW, what a tool! I've been wanting to know how to "fix" my commit history for a long time.
Excellent work, sir! This is going into my arsenal of tools; I can't believe I didn't know about this until now.
Anyway, I've fixed up my commit history and the pull request has been updated. I'll now continue on my list above.
Again, if anyone wants to take an item, just ping me and let me know which one you're taking so we don't step on toes.
UPDATE: I see I accidentally reverted a change you made to install.iss
to get rid of the obsolete isreadme
flag. I'll make sure to fix this on my branch.
when you have a minute, I'd appreciate any comments/thoughts you have on what I've done towards #462 and whether or not you feel that fits well with Git for Windows long-term.
I'm leaning towards let's not do it due to the dependency on the Windows SDK.
@fourpastmidnight I agree. Not only does it add a dependency on the Windows SDK, but also on Visual C++ (as WiX is compiled with that compiler, and GCC's C++ ABI is incompatible with Visual C++'s AFAIR).
here's my list of what I'd like to accomplish, in priority order:
@fourpastmidnight Excellent! Thank you so much for communicating in such outstanding a manner!
I'll just butcher the German and Italian that I do know ;)
Please don't. As I am German, and as you know me a little, that would include me :wink:
Seriously again, we do not bundle Git's messages' localizations in the InnoSetup installer, for size reasons. And I am really worrying about size: compared to InnoSetup's 30MB, the MSI seems to weigh in with 44MB already. Most likely there is no way with MSIs to use the real nice LZMA compression? If so, I would be very concerned about increasing the size even further by bundling localizations...
@dscho Unfortunately, I don't think we have a choice in MSI compression scheme. I agree, the LZMA is really nice, resulting in a ~50% smaller payload. I'm disappointed that MS over time hasn't updated the installer to support a compression scheme that results in smaller installers.
BTW: You would build one installer per locale (if you wanted to, yourself, provide localized versions). So you wouldn't be "bundling" all the supported languages into one massive installer. Anyway, it would just be an option, and the default would be en-us
. And one would be able to pass an optional flag to release.sh
to choose/change the localized strings an installer is built with. But, since I really only know English well, the custom Git dialogs will only have English localization to begin with.
You would build one installer per locale
Yeah, I guess I could do that. It would only make the release engineering last several hours more, and I would feel awful about asking GitHub to host so many megabytes of downloads :smile:
@dscho No one says you have to 😉 it's an option if you (or someone else) wants to. It's a community-driven project, so if the option's there and there's enough "demand", then at that time it may make sense. For now, assume business as usual. 😄
The pull request was updated. I fixed the ICE17 warnings due to "missing" banner bitmaps and removed some unnecessary localizations because I switched to use WiX's built-in localizations (e.g. for button caption text).
Just thinking... what if instead of all these remember Properties, we just use Features?
@robmen I tried that out tonight. The UX wasn't great. Don't get me wrong, it worked, but it felt wrong. Tell you what, I'm done for the night--but tomorrow, let me commit what I worked on wrt. a feature dialog rather than all these custom dialogs. Maybe you'll have some ideas for tweaking it. Using a feature dialog did get rid of three of the 6 custom dialogs.
Maybe I'll put it on a separate branch?
Yeah, wasn't thinking we need to show the Feature tree necessarily, just use conditional Features to cut down on the registry keys to remember all the settings since the many of these settings seem to be statements of what to install/not install.
Get your PR in a good place and in. It'll be easier to tweak it once there is stuff to work with directly. :smile_cat:
Hmm, well, actually all of these "features" are asking the user how to configure git. Most of this configuration seems like it would be based on modifications to .gitconfig
(but I haven't looked to see how this is done with the InnoSetup installer). And, from version to version, the user's choices are unlikely to change; so on upgrade, we'd want to present those dialogs with their previous installation options pre-selected. (Or better yet, don't show the dialogs at all at first--ask the user if they want to change their previous configuration; if not, just upgrade using the same settings--two click upgrade, at least, barring new installation options or actual features.) IOW, these registry keys aren't governing whether or not files get installed. It's all about configuration.
One thing I did look into way back at the beginning, which led to my custom make-file-list.sh
script, was to determine what packages are installed and what their dependencies are so that it would be possible to selectively decide which packages to install based on actual feature selection. The problem is, after using GraphVis to draw the dependency graph, there are not many independent packages. And, as more things become bundled with Git for Windows, it becomes harder to manage the dependency graph for the small amount of independent packages there are. So this type of install wasn't worth the effort in maintaining the dependency graph. Another reason I initially decided against the WixAdvancedUI dialog set.
In any event, I'll post what I have in terms of a feature dialog. Using the feature tree would be great because it could potentially allow for a two or three click install. But since it's not all that customizable, it didn't have a great UX. But, it doesn't hurt to explore it further for completeness' sake. If I could easily "roll my own", I would--but it's not easy as I'm sure you know. I read Bob Arnson's blog about that last night.
@robmen My awesome idea:
Given we want as minimal an install as possible (least clicks), then let's present a dialog saying "Here's how Git for Windows will be configured. Do you want to change this configuration?" We could let them select individual configurations to change. Then, only present the necessary dialogs for those configurations. Since these aren't "installed features", per se, we can "roll our own", so to speak, because we don't need the feature tracking normally associated with features. The existing registry entries are good enough.
This would allow two- or three-click installs and upgrades (again, barring new options on upgrade).
@fourpastmidnight It should be pretty easy to automate ssh-pageant startup as part of the installation (says the guy with no MSI knowledge), at least for the user performing the installation, so that it "just works"in all 3 scenarios... Git Bash, Git CMD, and native Windows command prompt. The installer would need to do the following, presumably based upon user selection:
There's a Wiki document covering ssh-pageant configuration, which might help to fill in the details. I need to tweak the details just a bit, but it should be largely complete at this point.
@fourpastmidnight yeah, lots we can do. Let's get the basics in then tweak from there.
@landstander668 that does sound pretty straight forward. A shortcut in the StartupFolder
might be friendliest although Run key is another option.
Again, I'd recommend we get what @fourpastmidnight has right now in then open new issues to track adding additional functionality.
@robmen Agreed. This is a distraction. Let's not change the UI structure as it currently is right now (other than adding the missing functionality, like Explorer Context menu entries and Desktop shortucts). Let's just get the installer functionality completed and on par with the InnoSetup installer. It doesn't have to be perfect right out of the gate--it just needs to work. Once we get it working, we can revisit the UX and flow--hopefully reducing the number of clicks and in the long run.
@landstander668 Thanks for the info. We'll take it under advisement for when we get to integrating that into the installer. If you could get around to updating the Wiki to tweak "the details" that need to be tweaked, that would be great. That way, when we're ready to integrate the configuration into the installer, we won't have to go fumbling around to figure out how it works (since I know next to nothing about ssh-pageant
:wink:). You'll have plenty of time to do that before we're ready to get there. (I figure a 2 - 4 weeks, at least.)
@robmen Updated the pull request with some of the things you pointed out. Thanks for looking things over. And, it was my fault about that one document and the line endings. I kept running across it tonight and so I ran a git config --global --list | grep -e autocrlf
and wouldn't you know? It came back false
. Oops. How did I do that? Anyway, I'm going to have to fix that and make sure the history of this PR looks OK. Sorry about that.
Updated the pull request with some more commits. It's getting messy, but I've completed about 34% of the items on the list above. Next up: trying to make the user's chosen configuration settings actually do something during install.
@fourpastmidnight would it be possible to take a moment and try to find a check-in point? You have a lot of good stuff that if committed others could start helping again.
While installing Git 2.7.2, an error was encountered where
C:\Program Files\Git\mingw64\bin
was unable to be deleted. I was given a dialog with three choices:Abort
,Retry
, orIgnore
. Well, it's fairly safe to say that ignoring this is not a good idea, and retrying didn't work (of course), so really, the only sensible option was toAbort
.When I clicked
Abort
, the status bar changed and saidRolling back...
. However, my previous installation was now corrupted--as not all files were restored/rolled back. This is a pretty huge installer bug. The whole point of MSI's is that they're transactional.If you're not going to support rolling back a failed installation, then remove the rollback steps in the installer.