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

installer: provide Windows Terminal Fragment Extension #339

Merged
merged 1 commit into from
May 18, 2021
Merged

installer: provide Windows Terminal Fragment Extension #339

merged 1 commit into from
May 18, 2021

Conversation

Okeanos
Copy link
Contributor

@Okeanos Okeanos commented May 2, 2021

Provides initial support for installing basic Git Bash fragments for Windows Terminal (see git-for-windows/git/issues/3183 for details).
Known Issues:

  • Final location of git-bash-global.json and git-bash-local.json in installer repository TBD
  • Mix and match of globally installed Terminal and locally installed Git (Bash) and vice versa does not work
  • Custom Git installation directories are not supported in the fragments
  • Should probably include a pre-generated UUIDv5 according to the Terminal docs for later modification

I am raising this WIP pull request to initiate the discussion on how to fix the aforementioned issues.

rimrul
rimrul previously requested changes May 2, 2021
Copy link
Member

@rimrul rimrul left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Final location of git-bash-global.json and git-bash-local.json in installer repository TBD

I think they should go into git-extra, but I'm not completely sure.

Mix and match of globally installed Terminal and locally installed Git (Bash) and vice versa does not work

I'm don't think we actually need to care how Windows terminal was installed.

Custom Git installation directories are not supported in the fragments

We might be able to disable the component for those unsupported cases with a check wether the value of {app} is what we expect.

Should probably include a pre-generated UUIDv5 according to the Terminal docs for later modification

Unless I'm missreading the documentation, that's only for modifying profiles that Windows Terminal comes with.

The only profiles that can be modified through fragments are the default profiles, Command Prompt and PowerShell, as well as dynamic profiles.

and "dynamic profiles" refers to these:

Windows Terminal will automatically create Windows Subsystem for Linux (WSL) and PowerShell profiles for you if you have these shells installed on your machine.

So I think the way we're supposed to update our profile is by updating the json file.

installer/install.iss Outdated Show resolved Hide resolved
installer/git-bash-local.json Outdated Show resolved Hide resolved
installer/install.iss Outdated Show resolved Hide resolved
installer/install.iss Outdated Show resolved Hide resolved
installer/install.iss Outdated Show resolved Hide resolved
@Okeanos
Copy link
Contributor Author

Okeanos commented May 2, 2021

General side question: I presume the merge request should be a single commit in the end, i.e. all fixes for the issues squashed into a single commit with a proper description and signed-off-by line?

@rimrul
Copy link
Member

rimrul commented May 2, 2021

We might also be able to avoid the install directory issues if we dynamically generate the json file like we do with the xml file for the auto updater in InstallAutoUpdater. So something like

procedure InstallWindowsTerminalFragment;
var
    Res:Longint;
    AppPath,XMLPath:String;
begin
    if IsAdminInstallMode() then
        JSONPath:=ExpandConstant('{commonappdata}\Microsoft\Windows Terminal\Fragments\Git\git-bash.json')
    else
        JSONPath:=ExpandConstant('{localappdata}\Microsoft\Windows Terminal\Fragments\Git\git-bash.json');
    AppPath:=ExpandConstant('{app}');
    SaveStringToFile(JSONPath,
        '{'+
        '    "profiles": ['+
        '      {'+
        '        "name": "Git Bash",'+
        '        "commandline": "'+AppPath+'\usr\bin\bash.exe -i -l",'+
        '        "icon": "'+AppPath+'\{#MINGW_BITNESS}\share\git\git-for-windows.ico",'+
        '        "startingDirectory": "%USERPROFILE%"'+
        '      }'+
        '    ]'+
        '  }',False);
end;

might work.

@rimrul
Copy link
Member

rimrul commented May 2, 2021

I presume the merge request should be a single commit in the end, i.e. all fixes for the issues squashed into a single commit with a proper description and signed-off-by line?

Yes, exactly. just force push your updated commit and this PR will be updated accordingly.

@Okeanos
Copy link
Contributor Author

Okeanos commented May 2, 2021

We might also be able to avoid the install directory issues if we dynamically generate the json file like we do with the xml file for the auto updater in InstallAutoUpdater. So something like

procedure InstallWindowsTerminalFragment;
var
    Res:Longint;
    AppPath,XMLPath:String;
begin
    if IsAdminInstallMode() then
        JSONPath:=ExpandConstant('{commonappdata}\Microsoft\Windows Terminal\Fragments\Git\git-bash.json')
    else
        JSONPath:=ExpandConstant('{localappdata}\Microsoft\Windows Terminal\Fragments\Git\git-bash.json');
    AppPath:=ExpandConstant('{app}');
    SaveStringToFile(JSONPath,
        '{'+
        '    "profiles": ['+
        '      {'+
        '        "name": "Git Bash",'+
        '        "commandline": "'+AppPath+'\usr\bin\bash.exe -i -l",'+
        '        "icon": "'+AppPath+'\{#MINGW_BITNESS}\share\git\git-for-windows.ico",'+
        '        "startingDirectory": "%USERPROFILE%"'+
        '      }'+
        '    ]'+
        '  }',False);
end;

might work.

The good news: it works. The bad news: AppPath expands using \ as separator which breaks JSON compatibility and renders the fragment unusable by Terminal.

@rimrul
Copy link
Member

rimrul commented May 2, 2021

Right. This should work around that:

procedure InstallWindowsTerminalFragment;
var
    Res:Longint;
    AppPath,XMLPath:String;
begin
    if IsAdminInstallMode() then
        JSONPath:=ExpandConstant('{commonappdata}\Microsoft\Windows Terminal\Fragments\Git\git-bash.json')
    else
        JSONPath:=ExpandConstant('{localappdata}\Microsoft\Windows Terminal\Fragments\Git\git-bash.json');
    AppPath:=StringChangeEx(ExpandConstant('{app}'), '\', '/', True);
    SaveStringToFile(JSONPath,
        '{'+
        '    "profiles": ['+
        '      {'+
        '        "name": "Git Bash",'+
        '        "commandline": "'+AppPath+'/usr/bin/bash.exe -i -l",'+
        '        "icon": "'+AppPath+'/{#MINGW_BITNESS}/share/git/git-for-windows.ico",'+
        '        "startingDirectory": "%USERPROFILE%"'+
        '      }'+
        '    ]'+
        '  }',False);
end;

@Okeanos
Copy link
Contributor Author

Okeanos commented May 2, 2021

So, I just pushed the changes you suggested:

  • Single option install
  • Additional Checks
  • Custom Procedure with embedded JSON

I tested this with local admin user by installing Git with the local installer (from scratch). Verified installation by opening Terminal and starting the newly registered Git Bash. To finish I also verified the fragment was deleted from disk when Git was uninstalled.

I could set up a local user (non-admin) in my VM as well if desired to test that part of the installation but not today anymore.

@rimrul
Copy link
Member

rimrul commented May 3, 2021

I could set up a local user (non-admin) in my VM as well if desired to test that part of the installation but not today anymore.

That would be great. Thank you.

Provides support for installing a basic Git Bash fragment for Windows Terminal (see git-for-windows/git/issues/3183 for details).

Signed-off-by: Nikolas Grottendieck <git@nikolasgrottendieck.com>
@Okeanos
Copy link
Contributor Author

Okeanos commented May 3, 2021

I updated the procedure a little to log errors in case the file could not be saved. That aside the tests (clean installs for admin and non-admin) users as described before worked perfectly fine and the Fragment was successfully detected by Windows Terminal.

@Okeanos Okeanos requested a review from rimrul May 10, 2021 05:53
Copy link
Member

@dscho dscho left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Most excellent!

@dscho dscho dismissed rimrul’s stale review May 18, 2021 11:40

@Okeanos addressed the concerns

@dscho dscho changed the title WIP: Provide Windows Terminal Fragment Extension installer: provide Windows Terminal Fragment Extension May 18, 2021
@dscho dscho merged commit 94620cd into git-for-windows:main May 18, 2021
dscho added a commit that referenced this pull request May 18, 2021
The installer now offers to install [a Windows Terminal
profile](#339).

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
@jeremyd2019
Copy link
Contributor

Should probably include a pre-generated UUIDv5 according to the Terminal docs for later modification

Unless I'm missreading the documentation, that's only for modifying profiles that Windows Terminal comes with.

I think providing a pre-generated UUIDv5 would allow the user to modify/override/disable the profile shipped here in their own settings json. That was my reading of the docs anyway

@DHowett
Copy link

DHowett commented Jun 8, 2021

You're correct! Providing a stable guid of any sort will allow the user to customize git-for-windows once and have their customizations stick around even if you change the name of the profile. It will also make sure that you collide with yourself (which may or may not be good, depending on your view.)

@jeremyd2019
Copy link
Contributor

So in this case should it be in the "guid" property like in the normal settings, or is the uuid supposed to be called "updates" in the fragment extensions schema?

@315mohsen

This comment has been minimized.

@dscho
Copy link
Member

dscho commented Jun 9, 2021

So how would such a GUID be provided? Could you open a PR to do that?

@jeremyd2019
Copy link
Contributor

From microsoft/terminal#10374 (comment)

UGH. Our documentation here is not correct¹.

The namespace GUID is actually based on the name of the folder/fragment name you're using, and it will be used to automatically generate a GUID if you haven't specified one.

Okay -- we can't change this because now we'll break compatibility with all user customizations on top of Fragments (grrr).

Here is the code that will generate the same GUID as Git Bash is already getting. You'll want to use this guid specifically, because doing anything else will separate the user's settings from your upstream document.

import uuid

# CALCULATE NAMESPACE BASED ON FRAGMENT FOLDER NAME
u = uuid.uuid5(uuid.UUID("{f65ddb7e-706b-4499-8a50-40313caf510a}"), "Git".encode("UTF-16LE").decode("ASCII"))

# CALCULATE FINAL BASED ON NAMESPACE
uuid.uuid5(u, "Git Bash".encode("UTF-16LE").decode("ASCII"))

# For Git/Git Bash: UUID('2ece5bfe-50ed-5f3a-ab87-5cd4baafed2b')

¹ The namespace GUID it provides is actually for the internal "Windows.Terminal.Wsl" namespace... which due to legacy reasons doesn't follow the same rules. It was authored in v0.5, and we didn't want to break user customizations back then either. 😬

So we at least now know what the GUID should be. I at least am not sure what "property" it should be on, "guid" or "updates", for a fragment.

@DHowett
Copy link

DHowett commented Jun 9, 2021

So we at least now know what the GUID should be. I at least am not sure what "property" it should be on, "guid" or "updates", for a fragment.

I'll also make sure the docs clear this up 😄

When the fragment is creating a totally new profile, it should have a guid or a name (the guid will be automatically generated based on the namespace rules above if guid is not provided.). Providing a guid is a way to guarantee that no matter what you do -- whether you move the fragment to another place or rename it -- it will be stable with regards to the user's customizations.

When the fragment is modifying an existing Terminal profile that came from another source (came with Terminal, detected via the Windows Subsystem for Linux detector, etc.) that was not the user¹, it should contain an updates guid. If the profile it is "updating" does not exist, the fragment will be ignored.

¹ This is so that a fragment cannot manipulate the user's personal profiles, no matter how hard it tries.

@DHowett
Copy link

DHowett commented Jun 9, 2021

In a diagram (which I'll formalize), it's like this:

-
|+ 0 - THE BEGINNING OF TIME
|
|+ 1 - The oceans form
|
|+ 2 - Built-in Terminal profiles (PowerShell, Command Prompt) are loaded
|+ 3 - Dynamic Terminal profiles (PowerShell 6+, WSL) are loaded
|
|+ 4 - Fragments are loaded
|      Fragments with a `guid` become full-fledged profiles
|      Fragments with an `updates` are applied to profiles in [1], [2], [3]
|         If they do not match, they are deleted.
|
|+ 5 - User profiles are loaded
|      Profiles that have the same GUID as ones in 1,2,3,4 are overlaid on
|      top as user-specific customizations.
|      If the user has a profile that came from [3] or [4] that can no longer
|      be found, it is deleted (So, if they uninstall Git for Windows the
|      profile doesn't linger)
v

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.

8 participants