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

Create GCM 2.1.0 release #1235

Merged
merged 157 commits into from
May 2, 2023
Merged

Create GCM 2.1.0 release #1235

merged 157 commits into from
May 2, 2023

Conversation

mjcheetham
Copy link
Collaborator

@mjcheetham mjcheetham commented May 2, 2023

Changes:

dependabot bot and others added 30 commits February 23, 2023 21:02
Bumps [lycheeverse/lychee-action](https://github.com/lycheeverse/lychee-action) from 1.5.4 to 1.6.1.
- [Release notes](https://github.com/lycheeverse/lychee-action/releases)
- [Commits](lycheeverse/lychee-action@4dcb8be...9ace499)

---
updated-dependencies:
- dependency-name: lycheeverse/lychee-action
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <support@github.com>
Because we use dotnet publish (which requires a specific framework) to
create the payload of our .NET tool package, the package is tied to the
framework specified. This means that it will only work with the
corresponding .NET SDK for that framework.

In light of this, update the .NET tool install instructions to specify
that users must download the latest .NET 6.0 SDK as a prerequisite for
installing the package, since that is the framework for which we
currently publish. This will also need to be updated whenever we upgrade
to a new Target Framework.

Additionally, remove Windows as a supported .NET tool platform. Since we
currently target .NET Framework rather than .NET, the package cannot
actually be installed on Windows.
Because we use dotnet publish (which requires a specific framework) to
create the payload of our .NET tool package, the package is tied to the
framework specified. This means that it will only work with the
corresponding .NET SDK for that framework.

In light of this, update the .NET tool install instructions to specify
that users must download the latest .NET 6.0 SDK as a prerequisite for
installing the package, since that is the framework for which we
currently publish. This will also need to be updated whenever we upgrade
to a new Target Framework.

Additionally, remove Windows as a supported .NET tool platform. Since we
currently target .NET Framework rather than .NET, the package cannot
actually be installed on Windows.
Ensure that we don't try to dispose of the pipe client in the
`Trace2CollectorWriter` when it hasn't been created yet.
We should always try and drain standard output before waiting for the
Git process to exit. We do this for all other calls that expect output,
but missed the `TryGet` method.
Ensure that we don't try to dispose of the pipe client in the
`Trace2CollectorWriter` when it hasn't been created yet.
We should always try and drain standard output before waiting for the
Git process to exit. We do this for all other calls that expect output,
but missed the `TryGet` method.

This is important if Git ever writes out so much data to stdout that we
buffer the output, and then Git will not exit until it can finish
writing (and we'd be waiting for Git to exit).
Add the ability to include extra query parameters in the authorization
code grant request. This allows consumers of the OAuth2Client to
customise the login experience that may include vendor specific args.
If a username is specified in the remote URL then include this as a
login hint when performing OAuth authentication for GitHub.
Add a unit test to ensure that we do not allow overriding or replacing
standard OAuth2 query parameters.
GCM currently ignores ports in URIs. This means attempting to authenticate
to a URI with a port can lead to some unexpected behavior (e.g.
shortcutting the provider auto-detect process won't work, even if the
provider is set in config). This change adds support for ports by updating
GetGitConfigurationScopes() to recognize them.

This change also updates GetRemoteUri() to recognize paths with queries
and fragments, as that issue was uncovered during the implementation of
the GetGitConfigurationScopes() fix. Without it, input paths containing
queries and/or fragments get saved as part of Uri.Path, which converts the
query '?' and the fragment '#' to url encoding.
GCM currently ignores ports in URIs. This means attempting to
authenticate to a URI with a port can lead to some unexpected behavior
(e.g. shortcutting the provider auto-detect process won't work, even if
the provider is set in config). This change adds support for ports by
updating GetGitConfigurationScopes() to recognize them.

This change also updates GetRemoteUri() to recognize paths with queries
and fragments, as that issue was uncovered during the implementation of
the GetGitConfigurationScopes() fix. Without it, input paths containing
queries and/or fragments get saved as part of Uri.Path, which converts
the query '?' and the fragment '#' to url encoding.

I validated these changes with unit tests for applicable scenarios and by
running a locally-compiled version of GCM with tracing enabled and the 
following config set:

`credential.http://localhost:7990/bitbucket.provider bitbucket`

Trace logs showed that the override was recognized and auto-detection
was skipped.
…1137)

If a username is specified in the remote URL then include this as a
login hint when performing OAuth authentication for GitHub. This can
help users of multiple accounts select the correct account before the
OAuth flow completes and returns a token for the wrong account.

GitHub shows this nice prompt when a login hint is provided that does
_not match the currently logged-in user_:


![image](https://user-images.githubusercontent.com/5658207/222230310-25c0e53e-ffab-4d42-bd4a-19d652e59835.png)

At the same time, if there is no logged-in user, then the login page's
username box is already filled in:


![image](https://user-images.githubusercontent.com/5658207/222231085-21c10d7d-bd38-4374-8ed8-da2e6f1429ee.png)

The login hint is specified to GitHub via the extra `login` query
parameter to the authorization endpoint.
The GitCredentialManager org is being re-named to git-ecosystem. In light
of this, update references to GitCredentialManager to instead reference
git-ecosystem.
Bumps [mjcheetham/update-homebrew](https://github.com/mjcheetham/update-homebrew) from 1.2 to 1.3.
- [Release notes](https://github.com/mjcheetham/update-homebrew/releases)
- [Commits](mjcheetham/update-homebrew@v1.2...v1.3)

---
updated-dependencies:
- dependency-name: mjcheetham/update-homebrew
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <support@github.com>
The GitCredentialManager org is being re-named to git-ecosystem. In
light of this, update references to GitCredentialManager to instead
reference git-ecosystem.
Create static class to manage session ids (sids). This allows us to
immediately set up GCM's sid in Program.cs to ensure Git processes that
are executed before Trace2 tracing is set up have the correct parent sid.
We also use it to give helper exes unique child sids.

Although this will also execute when TRACE2 tracing is not enabled, it
should not be an issue - it will simply set the (unused)
GIT_TRACE2_PARENT_SID varible for the process to a GUID.
Our current strategy of using a Stream to write to files for Trace2
tracing is not working - Git processes are overwriting GCM's data and vice
versa.

To resolve this, switch from using the Trace2StreamWriter to write to
files to a dedicated Trace2FileWriter class. This also includes a refactor
to add a parent base class for Trace2Writers for shared functaionality
between the Trace2StreamWriter and Trace2FileWriter.
The TRACE2 convention is for multi-word event names to be written in snake
case (all lower case with words separated by underscores). While up until
this point we've had single-word event names, this series will add the
multi-word child start and child exit events. In anticipation of that,
this change adds a string extension that will be used in later patches to
write multi-word enum values in snake case.
When a new process is created, standard error can optionally be
redirected. This is useful in situations where the output is verbose and
should be suppressed rather than output to the console, for example.

However, this can cause an issue with TRACE2 deadlocks, as Git writes to
standard error and GCM does nothing. This change corrects this by not
redirecting standard error for Git-related processes (which also extends
to places such as Environment.cs, given Git's reliance on envars).

Finally, there are some places where we should still redirect standard
error for the reason mentioned above. For these scenarios, comments have
been added to help readers understand the purposeful nature of these
redirects.
Diagnostics classes are dependent on multiple pieces of the
CommandContext. Adding child start and child exit events will mean yet
another dependency (Trace2). To simplify in anticipation of this, update
Diagnostic classes to take in the CommandContext and access applicable
dependencies from it.
The TRACE2 child start and child exit events will require some refactoring
around the way GCM handles processes to provide finer-grained control and
the ability to capture process-related information.

In preparation for this, we need to do some re-organization of the Trace2
code. With this change we:

1. Move TRACE2 tracing startup into Program.cs for GCM and the helper
exes. This allows us to start tracing earlier and capture more events.
2. Add an Initialize() method to Trace2.cs to detect whether Trace2
tracing is enabled and set long-lived variables. This also sets a new
_initialized property that allows us to determine whether Trace2 tracing
has been initialized. Trace2 tracing is disabled until _initialized is set
to true by the Initialize() method.
3. Change the name of TryParseSettings() to InitializeWriters() for
improved clarity.
4. Pass the CommandContext to Trace2 to use its Streams and Environment.
Wrap the Process class to allow finer-grained control for writing future
process-related TRACE2 events.

Pull process-relevant information out of IEnvironment/Environment and into
its own process-oriented ProcessManager class, interface, and child
classes. This allows us to  maintain a functional, non-circular order
of dependencies with our process wrapper.

Add a ChildProcess class to wrap certain functionality of Process.cs.
This gives us the level of control required to write child process
information to TRACE2 in the final patch in this series. Update all
current invocations of Process to instead use this wrapper.
Write the TRACE2 child_start and child_exit events for each child
process invoked by GCM.
This change adds the `child_start` and `child_exit` events to GCM's
`TRACE2` tracing system.

The first patches are either bug fixes or prep for the bigger refactors
involved in making child start and exit work. The first patch puts sid-related
logic into its own class, which allows us to immediately set up GCM's sid in
`Program.cs` to ensure Git processes that are executed before `TRACE2` tracing
is set up have the correct parent sid.

The next patch adds a `FileWriter` class and uses it (rather than
`Trace2StreamWriter`) to write to files, which ensures processes aren't
overwriting one another's data.

The third patch adds a snake casing string extension to ensure we write
multi-word enum values according to the `TRACE2` convention.

The fourth patch ensures we _do not_ redirect stderr for Git-related processes,
as it was discovered to cause deadlocks when `TRACE2` was enabled.

The fifth patch refactors GCM's diagnostics to depend on an instance of
`CommandContext` rather than individual dependencies within `CommandContext`.

The sixth patch re-configures `TRACE2` to depend on an instance of `CommandContext`
rather than individual dependencies within `CommandContext`, to start up earlier,
and for operations to no-op if `Trace2` has not yet been initialized
(i.e. when child `git config` processes run before we're able to set up tracing).

The seventh patch wraps `Process.cs` to give us the needed level of control to
write `TRACE2` child process start and exit events.

The eighth patch (finally) adds the new child start and exit events.
We are creating a `WindowsProcessManager` implementation on macOS and
a non-Windows/WSL aware, plain `ProcessManager` on Windows - this is the
reverse of what we should do. Fix this.
We are creating a `WindowsProcessManager` implementation on macOS and a
non-Windows/WSL aware, plain `ProcessManager` on Windows - this is the
reverse of what we should do. Fix this.
mjcheetham and others added 24 commits April 24, 2023 15:44
To keep the size of the Windows distribution small, directly target the
Win32 Avalonia packages rather than the Desktop package that includes
macOS and Linux dependencies.
)

Today GCM has an architecture that delegates all GUI prompting to
(bundled) external executables, using WPF-based helpers on Windows and
Avalonia UI helpers on Mac and Linux. Each host provider has its own GUI
helper executable, meaning we currently bundle four (GitHub.UI,
GitLab.UI, Atlassian.Bitbucket.UI, git-credential-manager.ui [for
generic auth]) helpers in addition to the core git-credential-manager
CLI executable.

On Linux we use the "publish as a single-file" model for each of these
executables, meaning we have a copy of the CLR included for each
executable! That means five CLRs!

Now that the Avalonia UI-based GUI helpers have matured, we can look at
dropping the WPF helpers and reduce the effort required to ship new GUI
by 50% (we no longer need to do everything twice: once with WPF and once
with Avalonia UI).

In addition, in a recent spike, I found that it may be possible to bring
the Avalonia UI helpers in-process, with nearly zero net-new code. This
would help reduce our installation footprint on Linux down to
approximately 1/4 of the current size. The single executable model would
also help decrease build times, and increase end-user performance since
we no longer pay the process startup costs using external processes!

We need to be mindful that this change may result in a small increase in
the Windows installation as we now bring in extra libraries used to
support Avalonia UI, whereas previously we relied on WPF libraries as
included with Windows 'for free'. This is expected to be minimal.

As for the current GUI extensibility model, we are not aware of any
consumers, but it would not be much effort to keep the existing external
process model, but now just default to using our in-process GUI options
in the absence of external configuration. This still gives tools like VS
or VSCode options to integrate native UI for GCM in the future.

Initially, we are still keeping the WPF helpers around on Windows but
disabled by default.
We can drop these after at least one release using the in-proc Avalonia
helpers.

To switch back to the WPF helpers, use
`credential.devUseLegacyUIHelpers` or `GCM_DEV_USELEGACYUIHELPERS`.
Add the ability to configure MSAL to use the default OS account when the
broker is enabled.

Default to disabled.
Detect when we are in a Microsoft Dev Box environment, and if we are,
then default to enabling the default OS account setting and enabling
WAM.
Bumps [DavidAnson/markdownlint-cli2-action](https://github.com/DavidAnson/markdownlint-cli2-action) from 9.0.0 to 10.0.0.
- [Release notes](https://github.com/DavidAnson/markdownlint-cli2-action/releases)
- [Commits](DavidAnson/markdownlint-cli2-action@5b7c9f7...cdfad95)

---
updated-dependencies:
- dependency-name: DavidAnson/markdownlint-cli2-action
  dependency-type: direct:production
  update-type: version-update:semver-major
...

Signed-off-by: dependabot[bot] <support@github.com>
Add options for enabling tracing and debugging in Git configuration.
Today the below options were only supported via environment variable:

 - GCM_TRACE
 - GCM_TRACE_SECRETS
 - GCM_TRACE_MSAUTH
 - GCM_DEBUG
Add options for enabling tracing and debugging in Git configuration.
Today the below options were only supported via environment variable:

 - `GCM_TRACE`
 - `GCM_TRACE_SECRETS`
 - `GCM_TRACE_MSAUTH`
 - `GCM_DEBUG`
Add Trace2 config/env var options to GCM's configuration/environment
documentation to improve discoverability.
Add Trace2 config/env var options to GCM's configuration/environment
documentation to improve discoverability.
Remove Nerdbank.GitVersioning dependency from project in favor of a new
VERSION file, which will be updated ahead of each release. Versioning in
this file will begin with the version we plan to use for the next release:
2.1.0.0.

This change involves the addition of a new custom MSBuild task, which
reads in the contents of the VERSION file, converts it to a Version
object, and then sets the various version-related MSBuild properties with
the correct value (some with the `Revision` component appended, others
without).

Note that there is a bug in MSAL [1] that causes build failures for
projects without dependencies with this change. We add Newtonsoft.Json as
a global dependency in Directory.Build.props to work around this problem
until the fix is released.

[1]: AzureAD/microsoft-authentication-library-for-dotnet#4108
Update release workflow to use VERSION file for versioning, rather than
Nerdbank.GitVersioning.
Remove Nerdbank.GitVersioning tasks (which have been superceded by the use
of the new VERSION file) from the release workflow.
Remove fetch-depth from checkout actions, since that was required by
Nerdbank.GitVersioning. Also take this opportunity to use consistent version
and naming for checkout actions.
With the [plan to migrate GCM to a formula for release via
`Homebrew/homebrew-core`](#1102),
it became clear that
[Nerdbank.GitVersioning](https://github.com/dotnet/Nerdbank.GitVersioning)
was no longer an option for versioning the project. This is because
`Nerdbank.GitVersioning` requires history to calculate things like ['Git
height'](https://github.com/dotnet/Nerdbank.GitVersioning#what-is-git-height),
and the formula requires a tarball to build, which, of course, has no
history.

This change pivots GCM to the simpler strategy of using a version
specified in a `VERSION` file at root. This version will be updated by
maintainers prior to release - giving them more granular control of
versioning, which in turn allows for versioning to be more predictable
(i.e. maintainers will know what the version will be before publication
of the release).

The version specified in the file is the one slated for the next
release: `2.1.0.0`.

**Note:** This change [fails on
Windows](https://github.com/ldennington/git-credential-manager/actions/runs/4824686907)
due to a [bug in
MSAL](AzureAD/microsoft-authentication-library-for-dotnet#4108)
unless we ensure all projects have at least 1 dependency. We are working
around this issue by adding Newtonsoft.Json as this dependency (since we
were already shipping it anyway).
Bumps [DavidAnson/markdownlint-cli2-action](https://github.com/DavidAnson/markdownlint-cli2-action) from 10.0.0 to 10.0.1.
- [Release notes](https://github.com/DavidAnson/markdownlint-cli2-action/releases)
- [Commits](DavidAnson/markdownlint-cli2-action@cdfad95...bb4bb94)

---
updated-dependencies:
- dependency-name: DavidAnson/markdownlint-cli2-action
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <support@github.com>
Add the ability to configure MSAL to use the default OS account when the
broker is enabled.
Also detect when we are in a Microsoft Dev Box environment, and if we
are, then default to enabling the new setting (and enable WAM).

Show a confirmation prompt before continuing to use the current OS
account, which is similar to how Microsoft Teams operates.

<img width="888" alt="windows-defaultaccount"
src="https://user-images.githubusercontent.com/5658207/234346956-b08eb43f-c964-4978-84ec-a0f75f021f08.png">

Left: Avalonia UI, Right: fallback WPF window

Fixes #917
@ldennington ldennington changed the title Create GCM 2.1 release Create GCM 2.1.0 release May 2, 2023
Copy link
Contributor

@ldennington ldennington left a comment

Choose a reason for hiding this comment

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

Looks great - thanks for the excellent notes!

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.

6 participants