Use a separate install.Lock for each dependency #2707
Conversation
akshita31
force-pushed
the
installLock
branch
from
8d46587 to
0468a8b
Compare
Codecov Report
@@ Coverage Diff @@
## master #2707 +/- ##
==========================================
- Coverage 65.55% 65.49% -0.06%
==========================================
Files 99 100 +1
Lines 4331 4350 +19
Branches 631 632 +1
==========================================
+ Hits 2839 2849 +10
- Misses 1308 1317 +9
Partials 184 184
Continue to review full report at Codecov.
|
akshita31
requested review from
DustinCampbell,
chuckries,
gregg-miskelly and
rchande
akshita31
changed the title
|
How do this work if I upgrade an existing installation? We'll just stop looking at my existing install.lock and be good to go? |
|
What is the meaning of upgrading existing installation ? Whenever the extension updates it creates a new folder and is not concerned with the contents of the old one right? |
|
Ah, right. |
| private extensionPath: string) { | ||
| } | ||
|
|
||
| public async installRuntimeDependencies(): Promise<boolean> { |
There was a problem hiding this comment.
We are now always calling this method when the C# extension starts right? If so, shouldn't we avoid posting some of these events to the event stream until after we have determined if there are any missing packages?
There was a problem hiding this comment.
@gregg-miskelly You are right, but with the current code structure it is not possible and we will need some refactoring in order to achieve that. I will do that in a subsequent PR.
|
@NTaylorMullen Do you have any concerns with this change ? |
Based on what you wrote in the description of the PR this sounds great! Means we wont have busted partial installs of dependencies it sounds like. Any negative implications to this? |
|
@NTaylorMullen I am still unsure of what part fails on the user's systems when they have an improper download/install. With this PR, I think it will help us identify better which of the three dependencies got corrupted as each will have its own install.Lock file that should be put only when both the download and install completed without throwing an exception. And if for a dependency say OmniSharp, the download failed this time and the install.Lock was not put into the expected location, the next time the user opens vscode, it would detect that it needs to install Omnisharp as the install.Lock is not present. |
|
Awesome, ya this would be a great addition, no concerns! |
4 participants
Fixes: #2478
With this change we will have an install.Lock for each package that is downloaded rather than having a single install.Lock for the runtime dependencies.
This will be a step forward towards the validation of the downloads as listed here: #2694.
Also, in this PR, I have added an "Id" attribute to the packages so that whatever part of the code needs access to a particular package to verify if the required dependency is installed or not, they could filter the packages from package.json easily, eg, for the case of the debugger packages as I have done in this PR.
cc @NTaylorMullen