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

Upgrader: do not include Micrsoft.Diagnostic.Tracing.EventSource #1137

Merged
merged 2 commits into from May 10, 2019

Conversation

jamill
Copy link
Member

@jamill jamill commented May 9, 2019

The upgrade verb attempts to copy
Microsoft.Diagnostic.Tracing.EventSource when copying the
ProductUpgrader to the temporary directory to run. However, this dll is
no longer needed or installed during installation.

Older installations will have copied this file into the install
directory will still have a copy for the upgrade verb to copy over.
However, clean installs, or if you uninstall VFS4G and re-install, will
not have this dll and upgrade will fail.

This is a quick fix - we will also see how to make this less fragile.
One option might be to install "upgrader app" into its own directory
under the VFS For Git installation directory, and then just copy the
directory to the temp location, instead of providing a list of files
that need to be kept in sync.

@@ -36,7 +36,6 @@ public abstract class ProductUpgrader : IDisposable
UpgraderToolConfigFile,
"GVFS.Common.dll",
"GVFS.Platform.Windows.dll",
"Microsoft.Diagnostics.Tracing.EventSource.dll",
Copy link
Member

@wilbaker wilbaker May 9, 2019

Choose a reason for hiding this comment

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

Should we also remove any references to Microsoft.Diagnostics.Tracing left in projects\config files?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes - we should. I will go and remove the other ones I find.

@wilbaker
Copy link
Member

wilbaker commented May 9, 2019

Older installations will have copied this file into the install directory will still have a copy for the upgrade verb to copy over.

Does the upgrader only need to worry about files that were included with its own installation?

I couldn't tell from your comment if there is an issue with older installations having this file around.

@jamill
Copy link
Member Author

jamill commented May 9, 2019

Does the upgrader only need to worry about files that were included with its own installation?

I couldn't tell from your comment if there is an issue with older installations having this file around.

The upgrader needs to copy over its dependencies. In this case, the file is no longer a dependency (so we do not need to copy it over), but it is still in the list of files that the upgrade verb tries to copy over. The reason why we didn't notice this before (and lessens the impact), is that users who already have this file in the installation directory (via an older installation) will still be able to upgrade, as this file is apparently not removed when installing a new version over an old version...

I will update the commit message to include the commit where this file was removed from the installer (cbe5787)

Copy link
Member

@wilbaker wilbaker left a comment

Choose a reason for hiding this comment

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

Approved with suggestions

@jrbriggs jrbriggs requested review from alameenshah and removed request for alsheehan May 10, 2019 00:47
@jrbriggs
Copy link
Member

Looks good, thanks @jamill. Please port to release branch.

The upgrade verb attempts to copy
Microsoft.Diagnostic.Tracing.EventSource when copying the
ProductUpgrader to the temporary directory to run. If this file is not
present, then the upgrade verb will fail. This following commit removes
this file:

  cbe5787 ("Remove in-proc ETW trace event listener", 2019-02-18)

Clean installs of VFS4G that include this commit will no longer have
this file in the installation directory, and will not be able to
upgrade.

This dll is no longer needed or installed during installation, so we
remove it from the list of files copied over for upgrade.

This is a quick fix - we will also see how to make this less fragile.
One option might be to install "upgrader app" into its own directory
under the VFS For Git installation directory, and then just copy the
directory to the temp location, instead of providing a list of files
that need to be kept in sync.
Product code no longer has a dependency on
Microsoft.Diagnostics.Tracing.EventSource, so remove references to NuGet
packages.
@jamill
Copy link
Member Author

jamill commented May 10, 2019

I added a new commit to remove references to the Microsoft.Diagnostic.Tracing.EventSource package as well.

I will only bring the first commit over when I port the changes to release branches.

/cc @wilbaker, @derrickstolee, @mjcheetham

@jamill jamill requested a review from mjcheetham May 10, 2019 14:34
Copy link
Member

@mjcheetham mjcheetham left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@alameenshah alameenshah left a comment

Choose a reason for hiding this comment

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

Looks good.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants