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

vscode.extensions.getExtension("...").packageJSON gives the wrong value after a reload window #40500

Closed
sean-mcmanus opened this issue Dec 19, 2017 · 26 comments
Assignees
Labels
debug Debug viewlet, configurations, breakpoints, adapter issues extensions Issues concerning extensions under-discussion Issue is under discussion for relevance, priority, approach

Comments

@sean-mcmanus
Copy link
Contributor

sean-mcmanus commented Dec 19, 2017

  • VSCode Version: 1.19
  • OS Version: Windows

Steps to Reproduce:

  1. Download the C/C++ extension. Upon installation, it rewrites the package.json, a requirement to have debugging work.
  2. Reload window.

Bug: The value of vscode.extensions.getExtension("...").packageJSON is incorrect and doesn't match the value of the re-written package.json on disk. It gives back the old value, which causes our debugger to fail, because we need the re-written package.json values. Doing a 2nd reload window fixes the issue. This bug also does not repro when extension developers debug their code via Launch Extension, which is why we never noticed it before (i.e. it only starts to repro after installing a new vsix).

I don't think our extension can fix this -- we need VS Code to use the new package.json that we wrote to disk, although a far better solution would be to enable some preactivate scenario that does:

  1. Calls preactivate(), which does custom install stuff, such as modifying the package.json. In this stage, the extension is not really active.
  2. Afterwards, the newly written package.json would automatically be used, and the normal activate() would be called.

This would enable a seeemless experience for users requiring 0 extra reloads. With this bug, users have to do 2 extra reloads (a simple fix of the bug would require 1 extra reload).

Reproduces without extensions: Yes

@vscodebot vscodebot bot added the extensions Issues concerning extensions label Dec 19, 2017
@sean-mcmanus
Copy link
Contributor Author

sean-mcmanus commented Dec 19, 2017

This bug only repros on Windows (not on Linux or Mac). I don't know yet if this repros for all Windows machines or if there's a special requirement.

@weinand
Copy link
Contributor

weinand commented Dec 19, 2017

@alexandrudima could this be related to your recent package.json work?

@weinand weinand added the info-needed Issue requires more information from poster label Dec 19, 2017
@alexdima
Copy link
Member

@sean-mcmanus

Upon installation, it rewrites the package.json, a requirement to have debugging work.

This is highly unexpected. Why is this a requirement to have debugging work? Why would an extension rewrite its package.json? It sounds like something is really missing in our API or extension story...


The root cause is our newly introduced caching of package.json. We cache the user installed extensions manifests in the VS Code user data folder (same one containing User/keybindings.json or User/settings.json), in a file called CachedExtensions/user. We introduced the cache in order to speed up the start-up time of VSCode (see #28331, #39593). Before, we would need to do a readdir, followed by multiple readfile for each extension/package.json, and now we do a single readfile to get all the extensions information in one fs call.

The cache includes user installed extensions (and not extensions under development), so running locally with F5 will not include the package.json of the extension under development in the cache.


I suggest two paths forward:

  1. Short-term work-around: Patching your package.json results in the mtime of your extension folder to change. Please add something in your patching code to result in the mtime of the extensions folder to change (the parent folder). The mtime of the extensions folder is used as a cache invalidation key.
  2. Long-term: Why do you need to patch package.json ?

@sean-mcmanus
Copy link
Contributor Author

sean-mcmanus commented Dec 19, 2017

I assume someone on VS Code has to be aware of this, because the C/C++ and C# extensions have been doing this for a while (mostly due to the debugger).

  1. The debugger requires it in order to set the packageJson.contributes.debuggers, which is required to be done post-install due to OS specific values in it.
  2. We also need it for setting up the correct configurationSnippets in the package.json. Our debugger guy says that we could get rid of this after VS Code enables an API for configuration snippet providers.
  3. We also require it so that all users download our OS's specific bits (via activationEvents = "*"), but we change that to the real activationEvents by re-writing the package.json, because we don't want users to install the bits after they open a C++ file (they might no longer have internet access).

Okay, thanks. I'll investigate modifying the extensions folder mtime. Some API for doing post-installation processing before real activation might solve this.

Oh, I see, this started to repro on 1.19 -- my Linux/Mac testing was using 1.18.1 still, so the bug repros on Linux/Mac now. This is a scary change, because it executes a code path that was previously impossible to occur -- it actually runs our "offline installer" code, which thankfully seems to be benign, other than re-writing "Finished installing dependencies" when it shouldn't.

@weinand
Copy link
Contributor

weinand commented Dec 19, 2017

@sean-mcmanus what exactly are the debugger contributions in package.json that you need to do post-install?

@sean-mcmanus
Copy link
Contributor Author

sean-mcmanus commented Dec 19, 2017

You can see the changes in our javascript. This is code belonging to our debugger team (which also works on the C# extension), i.e. @pieandcakes . Without it, native debugging fails or C/C++ configuration snippets don't appear in the launch.json.

    util.packageJson.contributes.debuggers[0].runtime = undefined;
    util.packageJson.contributes.debuggers[0].program = './debugAdapters/OpenDebugAD7';
    util.packageJson.contributes.debuggers[0].windows = { "program": "./debugAdapters/bin/OpenDebugAD7.exe" };
    if (os.platform() === 'win32') {
        util.packageJson.contributes.debuggers[1].runtime = undefined;
        util.packageJson.contributes.debuggers[1].program = './debugAdapters/vsdbg/bin/vsdbg.exe';
    }

and

            util.packageJson.contributes.debuggers[0].configurationSnippets = configurationSnippet.map(snippet => {
                delete snippet.isInitialConfiguration;
                delete snippet.debuggerType;
                return snippet;
            });

@weinand
Copy link
Contributor

weinand commented Dec 19, 2017

@sean-mcmanus the first part could be replaced by implementing the adapterExecutableCommand in the C++ extension. It can determine and then return the executable path and arguments of a debug adapter to VS Code which will execute this instead of using the static contribution from the package.json.

@WardenGnaw
Copy link
Member

WardenGnaw commented Dec 19, 2017

@weinand Will there be support for configuration snippets similar to initial configurations with DebugConfigurationProvider?

Also do configurationSnippets allow commands? #23961

@sean-mcmanus
Copy link
Contributor Author

sean-mcmanus commented Dec 19, 2017

According to #34668, it sounds like dynamic configuration snippets was abandoned as "out-of-scope"?

@sean-mcmanus
Copy link
Contributor Author

sean-mcmanus commented Dec 20, 2017

@alexandrudima Using fs.utime to modify the mtime of the extensions folder fixes the issue for us...that seems good enough to me. Thanks.

@WardenGnaw
Copy link
Member

@weinand adapterExecutableCommand looks good, however, is there a way for the command to not display an error message saying it does not exist or cannot read property of 'command' null? There are instances where the adapter is not ready and we would want user to run at a later time. We would just display a vscode.window.showInformationMessage.

@alexdima
Copy link
Member

@sean-mcmanus Thank you for going with the workaround (touching the extensions folder) and I'm sorry for the breakage.

@chrisdias @seanmcbreen Can we please reach out to the C# extension authors and let them know we're caching package.json files.

@weinand Should we leave this item for you to continue figuring out how to remove the need for patching package.json ?

@weinand weinand assigned weinand and unassigned alexdima Dec 20, 2017
@weinand weinand added debug Debug viewlet, configurations, breakpoints, adapter issues under-discussion Issue is under discussion for relevance, priority, approach and removed info-needed Issue requires more information from poster labels Dec 20, 2017
@weinand
Copy link
Contributor

weinand commented Dec 20, 2017

@gregg-miskelly you might be affected by this issue as well. Please follow @alexandrudima 's suggestions.

@weinand
Copy link
Contributor

weinand commented Dec 20, 2017

@WardenGnaw issues around the adapterExecutableCommand will be addressed as part of #33801 in the January timeframe.

@isidorn
Copy link
Contributor

isidorn commented Dec 20, 2017

@sean-mcmanus fyi for the dynamic snippets I have commented here

@WardenGnaw
Copy link
Member

@weinand @gregg-miskelly is out until after the end of the year.

The C# extension is tracking it via dotnet/vscode-csharp#1929

@pieandcakes
Copy link
Contributor

@isidorn A CompletionItemProvider looks like a form of Intellisense when the user types in launch.json. Does this work when using the Add Configuration button in the launch.json window? That was what the configurationSnippets were supporting in the first place and what we were trying to support. By reading @weinand's comments, it looks like VSCode is moving away from having configurations to using DebugConfigurationProvider. As @WardenGnaw mentioned above, is there a way to provide these dynamically and have it hook into the Add Configuration button that appears when launch.json is open?

@weinand
Copy link
Contributor

weinand commented Dec 27, 2017

@pieandcakes no, in general VSCode is moving towards the DebugConfigurationProvider, not away from it.
But for snippets it might not even be necessary to support them via the DebugConfigurationProvider because they are already supported via existing APIs (see @isidorn's comment from above).

@pieandcakes
Copy link
Contributor

@weinand Was Isidorn's comment that you are referencing talking about using a CompletionItemProvider? If so, my comment above regarding usage seems to still be valid unless I'm misunderstanding what a CompletionItemProvider does.

@weinand
Copy link
Contributor

weinand commented Dec 27, 2017

Yes, I was refereing to the CompletionItemProvider. But @isidorn knows more about that than me.

I just wanted to correct your misinterpretation that we are moving away from the DebugConfigurationProvider.

@pieandcakes
Copy link
Contributor

@weinand Ok, I'll wait for his response then. My comment wasn't saying that you were moving away from DebugConfigurationProvider but rather towards it, so it seems like we're saying the same thing :).

@isidorn
Copy link
Contributor

isidorn commented Dec 28, 2017

@pieandcakes yes, it should work when the user presses the Add Configuration button. Try it out and let us know how it goes.
And yes that is the way to provide them dynamically.

@WardenGnaw
Copy link
Member

@isidorn Is this the correct DocumentSelector?

const documentSelector: vscode.DocumentSelector = {
        language: 'json',
        pattern: "**/launch.json"
    };

I am registering the CompletionItemProvider as vscode.languages.registerCompletionItemProvider(documentSelector, new ConfigurationSnippetProvider(), '{') but I can not trigger it with Add Configuration or '{' in launch.json.

Also would the CompletionItemKind be Snippet?

@isidorn
Copy link
Contributor

isidorn commented Jan 3, 2018

@WardenGnaw looks good to me. Yes the CompletionItemKind should be a Snippet.
I suggest to frist try writing a simple CompletionItemProvider for JSON that would just return a snippet for any json file. Once you have that working try using the pattern you specified.

Here is how we do it for our built in json extension https://github.com/Microsoft/vscode/blob/master/extensions/javascript/src/features/jsonContributions.ts#L39

fyi @jrieken to check if I am missing something here

@wmertens
Copy link

Triage: it seems that this issue is resolved? Can it be closed?

@sean-mcmanus
Copy link
Contributor Author

I'm fine with closing this.

@isidorn isidorn closed this as completed Sep 30, 2019
@vscodebot vscodebot bot locked and limited conversation to collaborators Nov 14, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
debug Debug viewlet, configurations, breakpoints, adapter issues extensions Issues concerning extensions under-discussion Issue is under discussion for relevance, priority, approach
Projects
None yet
Development

No branches or pull requests

7 participants