Skip to content

Ensure package.json is writable in *.vsix#508

Merged
joaomoreno merged 2 commits intomicrosoft:masterfrom
fmeum:master
Nov 20, 2020
Merged

Ensure package.json is writable in *.vsix#508
joaomoreno merged 2 commits intomicrosoft:masterfrom
fmeum:master

Conversation

@fmeum
Copy link
Contributor

@fmeum fmeum commented Nov 18, 2020

VSC requires the manifest file unpacked from the .vsix archive to be
marked as writable (see 1). Since generating writable artifacts is
difficult in some build systems (such as Bazel), it would be helpful if
vsce itself could ensure that the file mode of the manifest is correct.

With this commit, the correct file mode on package.json is set
automatically by the packaging process via a new Processor and yazl's
mode argument.

@ghost
Copy link

ghost commented Nov 18, 2020

CLA assistant check
All CLA requirements met.

Copy link
Member

@joaomoreno joaomoreno left a comment

Choose a reason for hiding this comment

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

Makes sense. But why not simply implement onFile on the ManifestProcessor and change the file mode when the package.json file comes along? This would be a much smaller change.

@fmeum
Copy link
Contributor Author

fmeum commented Nov 20, 2020

Thanks for the suggestion, I have made the required changes.

@fmeum fmeum requested a review from joaomoreno November 20, 2020 08:29
VSC requires the manifest file unpacked from the .vsix archive to be
marked as writable (see [1]). Since generating writable artifacts is
difficult in some build systems (such as Bazel), it would be helpful if
vsce itself could ensure that the file mode of the manifest is correct.

With this commit, the correct file mode on `package.json` is set
automatically by the packaging process via a new `Processor` and yazl's
`mode` argument.

[1]: https://github.com/microsoft/vscode/blob/88144f6d31718288c7a2c6fb741e0646d40804cf/src/vs/platform/extensionManagement/node/extensionsScanner.ts#L144
Copy link
Member

@joaomoreno joaomoreno left a comment

Choose a reason for hiding this comment

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

Looking great! Wanna try adding a test for it? It should be relatively straightforward.

@fmeum
Copy link
Contributor Author

fmeum commented Nov 20, 2020

I added a unit test for MetadataProcessor. Is that what you intended?

@joaomoreno joaomoreno merged commit 249376e into microsoft:master Nov 20, 2020
@joaomoreno
Copy link
Member

Great work, thanks! 🍻

@joaomoreno joaomoreno added this to the November 2020 milestone Nov 20, 2020
@fmeum
Copy link
Contributor Author

fmeum commented Dec 8, 2020

Would it be possible to get an NPM release of 1.83.0?

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.

2 participants