Hiroto/update system #231

HirotoShioi · 2019-07-19T06:40:28Z

ksaric

Good direction, several comments to take a look at.

ksaric · 2019-07-22T07:58:33Z

src/Cardano/Shell/Update/Lib.hs

+#endif
+
+-- | Updater path, args, windows runner path, archive path
+data UpdaterData = UpdaterData


Maybe you can have UpdaterData for each platform?
Not sure why you have udWindowsPath.

ksaric · 2019-07-22T07:58:52Z

src/Cardano/Shell/Update/Lib.hs

+
+updaterData :: UpdaterData
+updaterData = case buildOS of
+    Windows -> UpdaterData "WindowsPath" ["Windows", "Args"] (Just "Window path") Nothing


Here you can use actual executables for the installers found here - #216 (comment)

ksaric · 2019-07-22T08:00:02Z

src/Cardano/Shell/Update/Lib.hs

+                        removeFile updateArchivePath
+                    return . Right $ ()
+                ExitFailure code -> return . Left $ UpdateFailed code
+        else return . Left $ UpdaterDoesNotExist


Newline after else.

Also, is it possible to do return . Left . UpdaterDoesNotExist?
Just wondering.

Tried, but compiler complained so its not possible.

ksaric · 2019-07-22T08:00:58Z

src/Cardano/Shell/Update/Lib.hs

+    updaterExists <- doesFileExist path
+    if updaterExists
+        then do
+            exitCode <- case mWindowPath of


I don't think it should be optional.
An actual path should be there so it shouldn't be Maybe FilePath but an actual FilePath for an executable for each platform.

I think this is optional since this field is not required when you're on Mac/Linux. You can have it mempty but that's kind of awkward.

You know whether you are on Max/Linux or Windows so you don't need another check for that.

ksaric · 2019-07-22T08:01:36Z

src/Cardano/Shell/Update/Lib.hs

+                ExitFailure code -> return . Left $ UpdateFailed code
+        else return . Left $ UpdaterDoesNotExist
+  where
+    runCmd path args archive =


Please add the types to an anonymous (or where/let) function when you are writing it to make it more clear.

ksaric · 2019-07-22T08:03:42Z

src/Cardano/Shell/Update/Lib.hs

+    whenJust (Just a) f = f a
+    whenJust Nothing  _ = return ()
+
+writeWindowsUpdaterRunner :: FilePath -> IO ()


Please add a comment where you copied this from and why.

ksaric · 2019-07-22T08:03:55Z

src/Cardano/Shell/Update/Types.hs

@@ -3,8 +3,7 @@ module Cardano.Shell.Update.Types where
 import           Cardano.Prelude

 import qualified Data.Map as M
-
-import           Test.QuickCheck
+import           Test.QuickCheck (Gen, choose, frequency, listOf1)


Thanks, better.

ksaric · 2019-07-22T08:04:19Z

stack.yaml

@@ -36,6 +36,5 @@ extra-deps:
  - containers-0.5.11.0
  - libsystemd-journal-1.4.4

-


ksaric · 2019-07-23T07:21:53Z

src/Cardano/Shell/Update/Lib.hs

+updaterData :: UpdaterData
+updaterData = case buildOS of
+    Windows -> UpdaterData
+        "Installer.exe"


Please indent these after the constructor.

UpdaterData "Installer.exe" ...

This is hard to read.

ksaric

A couple of smaller fixes.

ksaric · 2019-07-25T11:05:03Z

app/Launcher/Main.hs

@@ -38,6 +39,8 @@ import           Data.X509.Extra (failIfReasons, genRSA256KeyPair,
 main :: IO ()
 main = do

+  -- _ <- runUpdater updaterData


Remove this.
We need a simple FilePath when somebody wants to call the runUpdater - then it will try to run the installer on the FilePath.

ksaric · 2019-07-25T11:06:11Z

src/Cardano/Shell/Update/Lib.hs

+    { udPath        :: !FilePath
+    , udArgs        :: ![Text]
+    , udWindowsPath :: Maybe FilePath
+    , udArchivePath :: Maybe FilePath


You don't need this since you know whether it's a Windows platform or not.
You just need one of these.

ksaric · 2019-07-25T11:06:55Z

src/Cardano/Shell/Update/Lib.hs

+        withCreateProcess (proc path (args <> [archive]))
+            $ \_in _out _err ph -> waitForProcess ph
+
+type RunCmdFunc = FilePath -> [String] -> FilePath -> IO ExitCode


Please inline this in the function type.

ksaric · 2019-07-25T11:08:27Z

src/Cardano/Shell/Update/Lib.hs

+    updaterExists <- doesFileExist path
+    if updaterExists
+        then do
+            exitCode <- case mWindowPath of


You know whether you are on Max/Linux or Windows so you don't need another check for that.

ksaric

LGTM.

Hiroto Shioi added 5 commits July 18, 2019 14:03

Update gitignore, Makefile

4137d33

Implement runUpdater

701aff6

Add fileDoesNotExist logic

7b9e70b

Use UpdaterData datatype

ddb7517

Add windowdUpdater runner

a108ac4

HirotoShioi added the wip Work In Progress label Jul 19, 2019

HirotoShioi requested review from erikd and ksaric as code owners July 19, 2019 06:40

Update nix file

cacb20b

ksaric reviewed Jul 22, 2019

View reviewed changes

ksaric and others added 3 commits July 22, 2019 10:05

Merge branch 'master' into hiroto/update-system

38b7541

Update updater

a0b03ed

Update

a130606

ksaric reviewed Jul 23, 2019

View reviewed changes

Hiroto Shioi added 2 commits July 23, 2019 17:04

Add test case for updater

7d815eb

Fix indentation

2324ade

ksaric suggested changes Jul 25, 2019

View reviewed changes

Hiroto Shioi added 2 commits July 26, 2019 15:09

Fix

65881ad

Fix test case

14f635d

ksaric approved these changes Jul 26, 2019

View reviewed changes

ksaric merged commit 7d7cafc into master Jul 26, 2019

ksaric deleted the hiroto/update-system branch July 26, 2019 08:24

HirotoShioi removed the wip Work In Progress label Jul 31, 2019

		@@ -36,6 +36,5 @@ extra-deps:
		- containers-0.5.11.0
		- libsystemd-journal-1.4.4

Hiroto/update system #231

Hiroto/update system #231

Uh oh!

Conversation

HirotoShioi commented Jul 19, 2019

Uh oh!

ksaric left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ksaric Jul 23, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ksaric left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ksaric left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ksaric Jul 23, 2019 •

edited

Loading