Provide simple installation script #143
Conversation
| @@ -0,0 +1,79 @@ | |||
| @echo off | |||
There was a problem hiding this comment.
why make this a batch script? I hate them with a passion ;-) I'd much prefer powershell. Especially since this already uses some for loop magic and EnableDelayedExpansion that I can't understand without googling for an hour :P
There was a problem hiding this comment.
:) I hate PS with a passion. Tbh, asked customer what they prefer and the answer was batch.
|
|
||
|
|
||
| set InstallSource=%CD% | ||
| for /f "delims=" %%x in (%InstallSource%\install.cfg) do ( |
There was a problem hiding this comment.
I'm assuming that this is the part that magically reads in the install.cfg file. I'd prefer if the script prompted the user for these two questions (where to install - with a sane default directory) and whether to install the service instead of using the config file.
A config file would make sense if this needed to be automated but as this is a one-time install script I don't see the need. Asking the user makes it much more straight-forward for them to install the profiler. No documentation needed at all. Just run the script and it tells you what to do.
| echo Installing Profiler to: "%InstallDir%" | ||
|
|
||
| if exist "%InstallDir%" ( | ||
| if "%InstallUploadService%"=="true" ( |
There was a problem hiding this comment.
do we really need this? what if the user previously installed in a different directory? then this will do nothing and installing the service will still fail later on. I'd rather just not support the re-install case and instead add an uninstall.ps1 file that removes the daemon and prompts the user to manually delete the folder.
|
|
||
| setx /m COR_PROFILER {DD0A1BB6-11CE-11DD-8EE8-3F9E55D89593} | ||
|
|
||
| REM Enable the profiler globally (the actual profiling is disabled in the Profiler.yml) |
There was a problem hiding this comment.
Please ensure the user has actually created a yml file. By default, this doesn't exist and I don't know what happens then. Possibly, all .NET processes are being profiled, which is probably not intended.
There was a problem hiding this comment.
alternatively, ship with a "disable all" default config, though that might be confusing as we also ship with an example config.
|
Thanks for the comments! Although the basic install worked fine, it turned out for mass-rollout further tweaks were needed. Especially, you cannot execute scripts on network drives and so on. I'll have to update the PR with several tweaks. But tbh I think we should write a proper installer. I've used NSIS for this in the past. I'm pretty sure we can use it here as well. Last time I wrote an NSIS script was for CQSE RoomD, also with Windows Service :) |
|
@mpdeimos what is the status of this? Will there be progress or should we discard this? |
This PR provides an simple installation script for the profiler. However, I am not sure if it should be part of the official distribution, so maybe let's discuss this 1st and if we keep it what must be 1) documented and 2) which features are missing. We also have to decide on the default install directory.
Installation currently offers two steps:
Installation of the profiler for the whole system (and thus being configurable via YML). It will be installed in a way that 32bit, 64bit and DNX can be profiled at the same time.
Installation of the upload service.
The installation offers simple configuration by a properties file (install.cfg).