Skip to content

Move temp folder into repo to avoid state that causes build errors from time to time when rebuilding locally (and packages have updated) #903

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

Merged

Conversation

bergmeister
Copy link
Contributor

Ideally, we should avoid this business and just use NuGet's global cache for that but this is a quickfix for not having to delete those folders in AppData that cause build errors when rebuilding. Now, a clean of the repo will sort this out (which is usually run when building the extension or PSES) to make those annoying local build errors disappear

@bergmeister bergmeister requested a review from rjmholt as a code owner April 1, 2019 21:40
@corbob
Copy link
Contributor

corbob commented Apr 1, 2019

Should this new temp directory be added to the .gitignore?

@rjmholt
Copy link
Contributor

rjmholt commented Apr 2, 2019

@bergmeister Would you be able to give a quick description of the kind of errors you're seeing?

@bergmeister
Copy link
Contributor Author

bergmeister commented Apr 2, 2019

I have not recorded it but it should be somewhere in the Slack channel, multiple other people have experienced it as well where they had to delete those packages folders created by the build in the temp appdata folder.

@rkeithhill
Copy link
Contributor

I occasionally get build failures that require me to remove the NuGet packages from some folder in my AppData directory before the build will succeed.

@corbob
Copy link
Contributor

corbob commented Apr 2, 2019

Here is (hopefully) relevant discussion from Discord/Slack when I encountered the error:

Screenshot from Discord of relevant discussion and errors

The error:

ERROR: Cannot find path 'C:\Users\corbob\AppData\Local\Temp\System.Security.Principal.Windows.4.5.0\runtimes\win\lib\netcoreapp2.0\System.Security.Principal.Windows.dll' because it does not exist.
At C:\Users\corbob\repos\PowerShellEditorServices\PowerShellEditorServices.build.ps1:163 char:5
+     Copy-Item -Path $internalPath -Destination $DestinationPath -Forc ...
+     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
At C:\Users\corbob\repos\PowerShellEditorServices\PowerShellEditorServices.build.ps1:403 char:1
+ task LayoutModule -After Build {
+ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
At C:\Users\corbob\repos\PowerShellEditorServices\PowerShellEditorServices.build.ps1:327 char:1
+ task Build {
+ ~~~~~~~~~~~~
Build FAILED. 4 tasks, 1 errors, 0 warnings 00:00:09.9432025
Copy-Item : Cannot find path 'C:\Users\corbob\AppData\Local\Temp\System.Security.Principal.Windows.4.5.0\runtimes\win\lib\netcoreapp2.0\System.Security.Principal.Windows.dll' because it does not exist.
At C:\Users\corbob\repos\PowerShellEditorServices\PowerShellEditorServices.build.ps1:163 char:5
+     Copy-Item -Path $internalPath -Destination $DestinationPath -Forc ...
+     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+ CategoryInfo          : ObjectNotFound: (C:\Users\corbob...pal.Windows.dll:String) [Copy-Item], ItemNotFoundException
+ FullyQualifiedErrorId : PathNotFound,Microsoft.PowerShell.Commands.CopyItemCommand

And a link if you have Discord

@rjmholt
Copy link
Contributor

rjmholt commented Apr 2, 2019

Sucks that the .NET API is giving us such a dud folder. I'd hope a solution to PowerShell/PowerShell#4216 doesn't have this issue.

@bergmeister
Copy link
Contributor Author

Even if .Net or PowerShell adds better API support for temp folders, the main point of this PR is to avoid any of that because it is effectively a global state that is causing issues and any state of a repo should be possible to be cleared with git clean -dfx.
Yes, one could possibly enhance the code to work better with this state or just use the NuGet cache directly but fixing it in this way provides the same benefit to the user experience whilst being a simple change to get rid of this annoying problem

@TylerLeonhardt
Copy link
Member

@bergmeister do you want to also update the Clean task to just use git clean -dfx?

@bergmeister
Copy link
Contributor Author

@TylerLeonhardt Good point about the Clean task. I think a git clean -dfx is a bit much and should only be executed manually as a last resort as always executing it might slow down the build quite a bit. I added the deletion of the folder to the clean task though but I am also happy to take it out.

@rjmholt
Copy link
Contributor

rjmholt commented Apr 3, 2019

I think a git clean -dfx is a bit much and should only be executed manually as a last resort

I agree. When tools have secretly run git clean, I have lost work. Because git touches local and remote history, I tend not to automate calls to it -- seems like a bit of a layering violation too.

Copy link
Member

@TylerLeonhardt TylerLeonhardt left a comment

Choose a reason for hiding this comment

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

LGTM

@TylerLeonhardt
Copy link
Member

We don't need to backport this since it's build related.

@TylerLeonhardt TylerLeonhardt merged commit 46eb697 into PowerShell:master Apr 5, 2019
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.

5 participants