Upgrader: implement cross platform functionality#1234
Conversation
jamill
requested review from
alameenshah,
derrickstolee,
mjcheetham,
nickgra and
wilbaker
jamill
force-pushed
the
upgrader_tasks/platform_tasks
branch
from
e84db69 to
9ee728f
Compare
jamill
force-pushed
the
upgrader_tasks/platform_tasks
branch
6 times, most recently
from
a4638d4 to
b9980d0
Compare
alameenshah
left a comment
alameenshah
left a comment
There was a problem hiding this comment.
Have one question about the whether running launchctl list elevated returns active console user's services or not.
Otherwise changes look Good.
jamill
force-pushed
the
upgrader_tasks/platform_tasks
branch
from
b9980d0 to
220a99f
Compare
|
|
||
| function GetBlockingProcesses() | ||
| { | ||
| var watchList = [ "gvfs", "GVFS.Mount", "git", "gitk", "Wish" ]; |
There was a problem hiding this comment.
Why was gvfs removed from this list? Could a gvfs command still be running when installing?
There was a problem hiding this comment.
On the Mac, we can run the installer while the ‘GVFS upgrade’ verb is running - as do not have the restriction we have on windows where we cannot delete the application while it is running. This allows us to run upgrade in the same window as the upgrade verb, and not in a separate window.
This means we need to allow running the installer while the GVFS upgrade command is running. We can come back and tighten up this logic if needed (maybe check parent processes?).
There was a problem hiding this comment.
What about if I run some gvfs command that could take a while like gvfs dehydrate and run gvfs upgrade in another prompt? Will it be okay? Will the mount get ripped out from under the dehydrate?
There was a problem hiding this comment.
Good points -
GVFS.Mount is still a blocking process - so if there is a mount process running, we will not run the installer.
More generally, I will note this in the list of things to improve on. For instance, maybe:
- The GVFS upgrade verb can check for other running GVFS commands
- The upgrade process can check to only not block for
GVFS upgradeverbs (if we can get the command line args for the command), - The upgrade process can check and not run if a GVFS command that is not in a parent process is running (also - I need to investigate if this is possible).
I think for the initial version, it is OK to not block for running GVFS commands (and provide instructions for initial users to not run upgrade while running other GVFS commands).
What about if I run some gvfs command that could take a while like gvfs dehydrate and run gvfs upgrade in another prompt?
I wonder if it is ok to run GVFS unmount while the repository is being dehydrated? If not, should the gvfs unmount command not run if it is not safe?
There was a problem hiding this comment.
I wonder if it is ok to run GVFS unmount while the repository is being dehydrated? If not, should the gvfs unmount command not run if it is not safe?
There is some protection with the lock and not running if an unmount is in process or another command has the lock but there could be some gaps in that logic. Not sure if we have much testing around this for each of the commands. But upgrade is kind of outside that but maybe the upgrade will fail when it tries to unmount the repos and something is already running that has the lock?
There was a problem hiding this comment.
I will note this for follow up (I will update the MVP issue with a list of deferred items, and might copy that into a new issue for post MVP items).
jamill
force-pushed
the
upgrader_tasks/platform_tasks
branch
2 times, most recently
from
d72e4d2 to
db1a7d4
Compare
mjcheetham
left a comment
mjcheetham
left a comment
There was a problem hiding this comment.
A few nits on naming and typos
| } | ||
|
|
||
| public static string WhereDirectory(string processName) | ||
| public static string GetProgramLocation(string programLocaterCommand, string processName) |
There was a problem hiding this comment.
Since we're not going with the ILocator component idea (just yet), we might just want to replace string programLocatorCommand with GVFSPlatform.Instance.Constants.ProgramLocatorCommand since this method is parsing output which is specific to the locator command, and also in practice this method is always going to be called with GVFS.Platform.Instance.Constants.ProgramLocatorCommand.
There was a problem hiding this comment.
The problem is that this ProcessHelper class is included in the Hooks projects, which do not have access to GVFSPlatform - so it has to be passed in from callers who do (always) have access to GVFSPlatform
Currently, this is hardcoded to run the "where" program, which is not appropriate on non-Windows platforms. Add a platform abstraction so different platforms can run different programs.
Different platforms have different requirements for the list of processes that block installation. The POSIX platforms also remove "gvfs" from the list of blocked processes, as they can run upgrade while the 'gvfs upgrade' command is running. We might want to come back and restrict this to only the `gvfs upgrade` command (or only if the running command is a parent process) in a future change.
Determine the services that are installed and running on the mac platform via the launchctl program.
jamill
force-pushed
the
upgrader_tasks/platform_tasks
branch
from
87157ea to
92bd571
Compare
5 participants
Includes the following changes to enable cross platform functionality required by the upgrade functionality:
To see how these changes fit in with the larger Mac Upgrader work, please look at #1129