-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Fix packaging of startpage and inlinereviews vsix's #1314
Conversation
@@ -1,7 +1,7 @@ | |||
<?xml version="1.0" encoding="utf-8"?> | |||
<PackageManifest Version="2.0.0" xmlns="http://schemas.microsoft.com/developer/vsx-schema/2011" xmlns:d="http://schemas.microsoft.com/developer/vsx-schema-design/2011"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This source.extension.vsixmanifest
shouldn't be used (we're not creating a .vsix for this project). Can we delete it from this project? The same would apply to the on in GitHub.StartPage
.
Am I missing something and there's a reason why they can't be deleted?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The vsix file has an entry exporting the pkgdef. I'm not sure you can kill the manifest and still have the project packaging properly. I think you may have to change the project type, and/or maybe have another project type for the pkgdef-but-no-vsix logic. There used to be a project type for this at some point iirc, but honestly I can't be sure so it would need testing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's what I thought as well, but I tried deleting the .vsixmanifest
and it still seemed to compile. Maybe this has changed in a recent VSIX project type update? Dunno.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could have, I wouldn't be surprised. I remember the file being mandatory but if they updated the targets file to skip it since then, then that would make sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The extension is now installing properly for me. 👍
Just a comment about the .vsixmanifest
files.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm all for simplifying the solution configurations. 😄
The build system overhaul removed some key parts of the entries that control what gets packaged into the VSIX so pkgdef files for GitHub.StartPage and GitHub.InlineReviews weren't being included. Additionally, both these projects should not be deployable as VSIXs because they're not standalone extensions (they can be for testing purposes but that should be done separately and manually on a case-by-case basis as needed). The combination of these two things means that default local builds would cause these packages to be deployed to the VS experimental instance and override the loading directory for DLLs, which is Not A Good Thing(tm)
Also take the opportunity to fix the publisher and identity of the InlineReviews package to match our other packages.
Fixes #1310