Skip to content
This repository was archived by the owner on Jun 21, 2023. It is now read-only.

Conversation

jcansdale
Copy link
Collaborator

Fixes #883

This PR makes it easy to install developer VSIX builds over the top of the release version (installed for all users).

The Publish configuration extension.vsixmanifest will include:

  <Installation AllUsers="true">

I wonder if we also should be using InstalledByMSI (see jaredpar/VsixUtil#1 (comment))?

The Debug and Release configurations will include:

  <Installation AllUsers="false" Experimental="true">

@jcansdale jcansdale force-pushed the fixes/883-experimental-builds branch from 14a9a53 to 4390c12 Compare February 24, 2017 09:41
@jcansdale
Copy link
Collaborator Author

jcansdale commented Feb 24, 2017

@tannergooding, I was wondering how the Roslyn team handles using slightly different source.extension.vsixmanifest files? You mentioned having, "an internal tool which fixes things up". Does this tweak the XML and add/remove the Experimental="true" attribute? How do you hook it into your .csproj?

The least-worst solution I've come up with so far is as follows. Unfortunately it involves duplicating the source.extension.vsixmanifest files in different folders, which isn't so elegant!

<None Condition="'$(Configuration)' == 'Debug'"
    Include="Configuration\Debug\source.extension.vsixmanifest" />
<None Condition="'$(Configuration)' == 'Release'"
    Include="Configuration\Release\source.extension.vsixmanifest" />
<None Condition="'$(Configuration)' == 'Publish'"
    Include="Configuration\Publish\source.extension.vsixmanifest" />

Thanks!

@jcansdale jcansdale force-pushed the fixes/883-experimental-builds branch from 4390c12 to e1d1dfa Compare February 24, 2017 12:44
@jcansdale
Copy link
Collaborator Author

My second attempt uses the following:

  <!--
  Update 'Debug' and 'Release' configurations to have AllUsers="false" and Experimental="true"
  The Publish configuration will keep AllUsers="true" and Experimental="false".
  -->
  <Target Name="MakeVsixManifestExperimental" AfterTargets="DetokenizeVsixManifestFile"
          Condition=" '$(Configuration)' == 'Debug' Or '$(Configuration)' == 'Release' ">
    <Warning Text="NOTE: Tweaking '$(IntermediateVsixManifest)' to be 'Experimental'" />
    <XmlPoke XmlInputPath="$(IntermediateVsixManifest)" Query="/x:PackageManifest/x:Installation/@AllUsers" Value="false"
             Namespaces="&lt;Namespace Prefix='x' Uri='http://schemas.microsoft.com/developer/vsx-schema/2011' /&gt;" />
    <XmlPoke XmlInputPath="$(IntermediateVsixManifest)" Query="/x:PackageManifest/x:Installation/@Experimental" Value="true"
             Namespaces="&lt;Namespace Prefix='x' Uri='http://schemas.microsoft.com/developer/vsx-schema/2011' /&gt;" />
  </Target>

I think this is more maintainable because there's only a single source.extension.vsixmanifest file.

@jcansdale jcansdale changed the title WIP: Add support for Experimental VSIX builds that can replace release versions Add support for Experimental VSIX builds that can replace release versions Feb 24, 2017
@jcansdale
Copy link
Collaborator Author

Once this PR goes though, you should be able to install the locally built VSIX over the top of the deployed version. The deployed version will use the Publish configuration and install with AllUsers=true and Experimental=false).

Also, you can associate VSIX files with the EXE found here: tools/VsixUtil/VsixUtil.exe
This will uninstall any previous Experimental version, before installing the target VSIX.

I'm adding a bunch of reviewers, so you all know this is possible. 😄

@tannergooding
Copy link

tannergooding commented Feb 24, 2017

@jcansdale, the tool source is here: https://github.com/dotnet/roslyn-tools/tree/master/src/ModifyVsixManifest/ModifyVsixManifest.

Basically our default VSIX manifest has Experimental=true and does not specify AllUsers or InstalledByMSI. We then (post build, but pre-signing) copy the VSIX to a separate location and run the below command. The original output retains the Experimental flag and the copied output will lose the Experimental flag and gain the AllUsers and InstalledByMSI flags. We then use the copied output as the VSIX we insert into VS and you should be able to give the original to people who need to dogfood the latest bits.

& $modifyVsixManifestTool --vsix=$insertionVsix --remove=//x:PackageManifest/x:Installation/@Experimental --add-attribute=//x:PackageManifest/x:Installation`;AllUsers`;true --add-attribute=//x:PackageManifest/x:Installation`;InstalledByMSI`;true

@jcansdale jcansdale force-pushed the fixes/883-experimental-builds branch from cb8ad1b to 5732aad Compare February 27, 2017 13:26
@jcansdale
Copy link
Collaborator Author

@tannergooding Thanks for the link! You have a bunch if interesting tools in there. 😄

I can see how it makes sense to tweak this pre-signing. Since we have a separate configuration for Publish, using the XmlPoke seems to work well. I'll chat to the team about this.

@jcansdale
Copy link
Collaborator Author

jcansdale commented Feb 27, 2017

Oh, it seems we don't actually use the Publish configuration. 😕

I've changed it so that it defaults to AllUsers=false and Experimental=true. For the Release configuration, this will be tweaked to be AllUsers=true and Experimental=false.

Updated script uses `vsixutil.exe` which is like a scriptable version
of `vsixinstaller.exe`. Extension will be installed for all versions of
Visual Studio. It will only be installed for one VS 2017 SKU
(prioritizing Enterprise, Professional then Community).
Add an MSBuild Target that will update `extension.vsixmanifest` to have
AllUsers="false" and Experimental="true" on 'Debug' and 'Release'
configurations. The 'Publish' configuration will keep AllUsers="true"
and Experimental="false".
Build was failing when CreateVsixContainer was False.
By default the VSIX will be created with `AllUsers=false` and
`Experimental=true`. It will be tweaked to have `AllUsers=true` and
`Experimental=false` for the `Release` configuration.
The `Debug` configuration will be build with AllUsers=false and
Experimental=true. This configuration can be installed without needing
to uninstall the `AllUsers` version.
Changed .cmd so that it works without the private `script` repo.
Running tests in `Debug` configuration doesn't currently work.
@jcansdale jcansdale force-pushed the fixes/883-experimental-builds branch from a01d902 to 4732caf Compare February 28, 2017 14:39
Copy link
Contributor

@paladique paladique 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, works great! Not sure if there's been a discussion about this already, but should we have a name other than "Experimental" for a local install? I could see confusion about what experimental actually means to someone building locally. It's just two definitions now, but a VS experimental instance is also local.

@paladique
Copy link
Contributor

paladique commented Feb 28, 2017

Also, what happens if I keep automatic updates checked and an update happens? Does it overwrite my experimental install?

@jcansdale
Copy link
Collaborator Author

jcansdale commented Feb 28, 2017

@paladique,

Not sure if there's been a discussion about this already, but should we have a name other than "Experimental" for a local install?

Good question! At the moment when you build a Debug configuration of the VSIX, its extension.vsixmanifest will be have Experimental=true and AllUsers=false. This is the only configuration that can feasibly be installed locally.

If you build a Release configuration of the VSIX, it will have the Experimental=false and AllUsers=true. You would struggle to install this version and if you managed, it would silently revert to the last version installed via Extensions and Updates. You'll get the kind of thing @grokys had when trying to uninstall the AllUsers=true version:

28/02/2017 17:32:17 - Uninstalling 'GitHub Extension for Visual Studio', version 2.2.0.8.
28/02/2017 17:32:21 - System.InvalidOperationException: The VSIX's catalog does not include a 'Component' which is required for install/uninstall.

Good point about the potential for confusion with the "Experimental" instance of Visual Studio! Let me know if you can think of a way to clarify the wording.

@jcansdale
Copy link
Collaborator Author

@paladique,

Also, what happens if I keep automatic updates checked and an update happens? Does it overwrite my experimental install?

It looks like experimental installs are sticky and don't automatically update. I did some experiments with a feed containing experimental installs and it looks like you have to explicitly trigger the update.

This can being the build time from 19s to 13s (on my laptop).
Bump to VisualStudio.Sdk.BuildTasks 14.0.14.0.23-pre.
Default to `IsProductComponent=false`.
Decouple IsExperimental from Debug/Release configurations.
Update README.md with description of build flavors.
@jcansdale jcansdale changed the title Add support for Experimental VSIX builds that can replace release versions Add support for different build flavors using IsExperimental and IsProductComponent properties Mar 2, 2017
@jcansdale
Copy link
Collaborator Author

From README.md:

Build Flavors

By default, building will create a VSIX with Experimental="true" and AllUsers="false" in its extension.vsixmanifest. These settings are necessary in order to easily install a standalone VSIX file. There is no need to uninstall the version previously installed via Visual Studio setup / Extensions and Updates.

To build and install a Debug configuration VSIX:

build.cmd
install.cmd

To build and install a Release configuration VSIX:

set Configuration=Release
build.cmd
install.cmd

To build a VSIX that could be deployed via the gallery / Extensions and Updates:

set Configuration=Release
set IsExperimental=false
build.cmd
install.cmd

To build a VSIX that could be deployed via Visual Studio setup:

set Configuration=Release
set IsExperimental=false
set IsProductComponent=false
build.cmd
install.cmd

README.md Outdated
```txt
set Configuration=Release
set IsExperimental=false
set IsProductComponent=false
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be IsProductComponent=true however I'm not sure we should even put this section in the publicly readable readme as it's for internal use only.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I wanted to keep everything together as much as possible. The Roslyn build uses similar stuff, so it isn't secret knowledge or anything. Since the .csproj files are public, I thought this documentation for them should be as well.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Opps. Good catch on IsProductComponent=true. Updated.

README.md Outdated

By default, building will create a VSIX with `Experimental="true"` and `AllUsers="false"` in its `extension.vsixmanifest`. These settings are necessary in order to easily install a standalone VSIX file. There is no need to uninstall the version previously installed via Visual Studio setup / Extensions and Updates.

To build and install a `Debug` configuration VSIX:
Copy link
Contributor

Choose a reason for hiding this comment

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

Might be worth making clear these instructions are for cmd.exe - when running them in powershell the set commands have no effect.

jcansdale and others added 3 commits March 2, 2017 12:45
Should have been `IsProductComponent=true`!
Specify using `cmd.exe`.
No need to mention `IsProductComponent=true` flavor.
Tweaked gallery feed wording.
@grokys grokys merged commit 1283457 into master Mar 2, 2017
@grokys grokys deleted the fixes/883-experimental-builds branch March 2, 2017 14:42
@jcansdale
Copy link
Collaborator Author

@tannergooding do you know if Experimental has broken in recent version of the tooling? We're suddenly getting our Experimental version loaded at the same time as the AllUsers version. Very frustrating! 😭

@tannergooding
Copy link

I'm not aware of anything that changed here.

@jcansdale
Copy link
Collaborator Author

I've worked out what the problem is. I assumed the Experimental flag would make the extension load instead of the AllUsers extension. Apparently the extension is actually overlaid on top of the AllUsers extension. This seems to work fine as long as none of your package GUIDs have changed!

The issue we were seeing was triggered when one of our package GUIDs did change. This caused both versions of our extension to load and all kinds of problems!

This is definitely something to watch out for if you ever remove a package! I struggle to believe this is how it's supposed to work. Maybe @tinaschrepfer could confirm?

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

Successfully merging this pull request may close these issues.

4 participants