-
Notifications
You must be signed in to change notification settings - Fork 728
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
experimental windows setup #421
Conversation
Codecov Report
@@ Coverage Diff @@
## master #421 +/- ##
=======================================
Coverage 91.93% 91.93%
=======================================
Files 199 199
Lines 6708 6708
=======================================
Hits 6167 6167
Misses 541 541
Flags with carried forward coverage won't be shown. Click here to find out more. |
8b936ee
to
59b9136
Compare
if($modified) | ||
{ | ||
$newPath = [string]::Join(';', $paths); | ||
New-ItemProperty -Path $registryPath -Name $environmentVariable -PropertyType ExpandString -Value $newPath -Force | Out-Null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this work without a reboot? I found that updating the reg value didn't work when I wrote the install instructions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Worked on my home machine immediately. The installer does something to make explorer.exe reload environment variables (there's a ChangesEnvironment=yes) setting. What you were probably seeing was explorer.exe launched processes getting stale environment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It will not help with any open command shells (cmd.exe, powershell, etc.) until they are reopened. However, that's a standard behavior for environments. I can make the installer force a reboot, but I don't think it's necessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's fine, no need to force a reboot. The behavior I was seeing was also with newly opened cmd/powershell windows. Sounds like the installer is doing something special to handle it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, explorer or windows must have some sort of API for these situations.
--> | ||
<Project Sdk="Microsoft.Build.NoTargets"> | ||
<PropertyGroup> | ||
<TargetFramework>net46</TargetFramework> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this mean the machine has to have .NET 4.6 installed to run the installer?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nope, it's just a dummy target framework needed for a NoTargets project. The installer is not .net-based btw. I believe it's native code (Delphi I think.)
@@ -0,0 +1 @@ | |||
bicep/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think better to be more precise and prefix with a /
(or whatever path prefix you're trying to exclude)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This .gitignore is specific to this directory only, though. I think we did something like that in the VSIX project too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unless you need to create bicep
folders at different levels it feels unnecessary. It's often done for more general build artifacts like out
or node_modules
which can appear at different levels, but in this case I think you should know exactly where the folder is created.
@@ -80,6 +84,36 @@ jobs: | |||
with: | |||
flags: dotnet | |||
|
|||
build-windows-setup: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should probably be added to the release as well (lower down in this file)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah intentionally left it out until we validate the installer more.
@@ -0,0 +1,49 @@ | |||
param ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would you mind making a note to update the install instructions for Windows when we next have a release? Maybe we can just start another 'running items for 0.2 release' note.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Created #424
Added a windows installer and uninstaller. It copies bicep.exe and bicep.pdb to the user's chosen installation directory and adds that directory to the PATH. Uninstall reverts all changes.
The installer is built using an open-source tool called Inno Setup. VS code uses it for its own Windows setup.