Refresh compilation before build and deploy to fix 8239 #8721
Conversation
bhsubra
changed the title
| @@ -134,6 +134,8 @@ private Compilation GetCompilation(DocumentUri documentUri) | |||
| { | |||
| var fileUri = documentUri.ToUri(); | |||
|
|
|||
| // Bicep file could contain load functions like loadTextContent(..). We'll refresh compilation to detect changes in files referenced in load functions. | |||
There was a problem hiding this comment.
@anthony-c-martin @majastrz Could you please review this PR? I assume the changes are probably sufficient to fix deployment from vscode. But is this indicative of a deeper problem that should be fixed elsewhere? E.g. should we be watching for changes to loadTextContent()/etc targets? Not sure if it affects anything else.
@bhsubra Does this not also affect the build command?
There was a problem hiding this comment.
RefreshCompilation() was added so we could recompile after module restore is performed because there isn't a small set of files to watch in that case.
We're able to recompile as needed when files that the module depends on change today. The existing logic handles bicep files and ARM template files, but not anything that load...() functions load. It would be good to fix that problem this way and not rely on RefreshCompilation()
The tricky part will be that other file linking is done via syntax before the semantic model is created, but this logic would have to happen after binding because you need symbols to identify load...() functions.
I am also curious why we didn't have to do it for the build command.
There was a problem hiding this comment.
Should bicep have a new command to list all inputs?
There was a problem hiding this comment.
But is this indicative of a deeper problem that should be fixed elsewhere? E.g. should we be watching for changes to loadTextContent()/etc targets? Not sure if it affects anything else.
Yes, it is. This PR is a stopgap - we should be thinking about addressing the root cause in the long run.
There was a problem hiding this comment.
Yup, I'm totally fine with this fix!
There was a problem hiding this comment.
+1 I'm also fine with the stopgap solution.
There was a problem hiding this comment.
Should bicep have a new command to list all inputs?
Yes, I think we should.
There was a problem hiding this comment.
Thanks @anthony-c-martin and @majastrz
src/Bicep.LangServer/Handlers/BicepDeploymentScopeRequestHandler.cs
Outdated
Show resolved
Hide resolved
| @@ -83,6 +83,8 @@ private string GenerateCompiledFileAndReturnBuildOutputMessage(string bicepFileP | |||
|
|
|||
| var fileUri = documentUri.ToUri(); | |||
|
|
|||
| // Bicep file could contain load functions like loadTextContent(..). We'll refresh compilation to detect changes in files referenced in load functions. | |||
| compilationManager.RefreshCompilation(fileUri); | |||
There was a problem hiding this comment.
@majastrz, @StephenWeatherford , this change is for build command. We have same issue for build as well.
Fixes #8239
Bicep file used in build and deploy could contain load functions like loadTextContent(..). If files used in load functions are updated, we don't update compilation. We need to manually refresh compilation before build and deploy operations so that file changes are picked up when build/deploy operations are kicked off.
Microsoft Reviewers: Open in CodeFlow