Skip to content
This repository has been archived by the owner on Aug 18, 2020. It is now read-only.

[DDW-380] Configure launcher to read/write safe mode parameter #3421

Merged
merged 3 commits into from
Aug 23, 2018

Conversation

disassembler
Copy link
Contributor

Description

This adds safe mode parameter daedalus team requested.

Linked issue

Type of change

  • 🐞 Bug fix (non-breaking change which fixes an issue)
  • 🛠 New feature (non-breaking change which adds functionality)
  • ⚠️ Breaking change (fix or feature that would cause existing functionality to change)
  • 🏭 Refactoring that does not change existing functionality but does improve things like code readability, structure etc
  • 🔨 New or improved tests for existing code
  • ⛑ git-flow chore (backport, hotfix, etc)

Developer checklist

  • I have read the style guide document, and my code follows the code style of this project.
  • If my code deals with exceptions, it follows the guidelines.
  • I have updated any documentation accordingly, if needed. Documentation changes can be reflected in opening a PR on cardanodocs.com, amending the inline Haddock comments, any relevant README file or one of the document listed in the docs directory.
  • CHANGELOG entry has been added and is linked to the correct PR on GitHub.

Testing checklist

  • I have added tests to cover my changes.
  • All new and existing tests passed.

QA Steps

Screenshots (if available)

@disassembler disassembler force-pushed the ddw-380-launcher-daedalus-safe-mode branch from 7192f06 to ead549f Compare August 16, 2018 22:20
CodiePP
CodiePP previously approved these changes Aug 17, 2018
Copy link
Contributor

@CodiePP CodiePP left a comment

Choose a reason for hiding this comment

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

LGTM
there is work underway to remove 'RecordWildCards'

@@ -280,7 +281,7 @@ main =
setEnv "LC_ALL" "en_GB.UTF-8"
setEnv "LANG" "en_GB.UTF-8"

LO {..} <- getLauncherOptions
lo@LO {..} <- getLauncherOptions
Copy link
Contributor

Choose a reason for hiding this comment

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

'RecordWildCards' will be gone..
can the field accessed in LO be named?

Copy link
Member

Choose a reason for hiding this comment

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

@cleverca22 I would actually leave that as it is for now.

What we are currently doing is moving RecordWildCards from the default language extensions to a LANGUAGE pragma at the top of the files that need it.

While that is being done we are recording how RCWs are being used and removing the ones that are the most difficult to read (ie using RCW for types in an upstream package).

In this case LauncherOptions is in the same package and hence probably does not need to change.

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah, the code was also looking much worse after removing it, so i put it back in

@@ -453,33 +442,28 @@ clientScenario
:: (HasCompileInfo, HasConfigurations)
=> ProtocolMagic
-> NodeDbPath
-> Maybe FilePath -- ^ Log prefix
Copy link
Contributor

Choose a reason for hiding this comment

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

red is my preferred colour! :-)
great, it gets lighter.

-> Maybe FilePath -- ^ Node log file
-> LauncherOptions -- ^ Launcher Options
-> M ExitCode
runWallet shouldLog nd nLogPath lo = do
runWallet nd nLogPath lo = do
safeMode <- readSafeMode lo
let wpath = ndPath nd
wargs = (ndArgs nd) <> (if safeMode then [ "--safe-mode", "--disable-gpu", "--disable-d3d11" ] else [])
Copy link
Contributor

Choose a reason for hiding this comment

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

more options turned off.

Copy link
Contributor

Choose a reason for hiding this comment

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

nikola requested that safemode send more flags

Copy link
Contributor

@parsonsmatt parsonsmatt left a comment

Choose a reason for hiding this comment

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

There are some style issues but nothing that will hold the PR up. All behavior stuff looks fine to me.

@@ -11,6 +11,7 @@
{-# LANGUAGE RankNTypes #-}
{-# LANGUAGE TemplateHaskell #-}
{-# LANGUAGE TypeSynonymInstances #-}
{-# LANGUAGE RecordWildCards #-}
Copy link
Contributor

Choose a reason for hiding this comment

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

stylish-haskell

@@ -630,31 +618,70 @@ spawnNode nd doesWalletLogToConsole = do
Just ph -> do
logInfo "Node has started"
return (ph, asc)
data SafeModeConfig = SafeModeConfig { smcSafeMode :: ! Bool } deriving Generic
Copy link
Contributor

Choose a reason for hiding this comment

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

this should be a newtype instead of a data

decoded <- liftIO $ Y.decodeFileEither safeModeConfigFile
case decoded of
Right value -> pure $ smcSafeMode value
Left _ -> pure False
Copy link
Contributor

Choose a reason for hiding this comment

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

Please put whitespace in between declarations to make it easier to read

@disassembler disassembler merged commit c14a5f2 into release/1.3.1 Aug 23, 2018
@disassembler disassembler deleted the ddw-380-launcher-daedalus-safe-mode branch August 23, 2018 18:24
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants