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

[RCD-52] cardano-launcher | win64: kill self via PID, not process name #3926

Merged
merged 4 commits into from
Dec 4, 2018

Conversation

deepfire
Copy link
Contributor

@deepfire deepfire commented Dec 4, 2018

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)

How to merge

Send the message bors r+ to merge this PR. For more information, see
docs/how-to/bors.md.

@deepfire deepfire self-assigned this Dec 4, 2018
@deepfire deepfire requested review from KtorZ and a team December 4, 2018 12:00
@deepfire deepfire force-pushed the rcd-52-indiscriminate-killing branch from 76ede80 to bceb3d5 Compare December 4, 2018 12:48
writeFile (toString runnerPath) $ unlines
[ "TaskKill /IM cardano-launcher.exe /F"
[ "TaskKill /PID "<>show selfPid<>" /F"
Copy link
Contributor

Choose a reason for hiding this comment

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

Not a big fan of this to be honest, and the pragma on top the file... And, what the comment truly says is that, if someone ever needs the windows update runner, it means they've ran the launcher on windows first, and thus, that those lines have been ran on windows? Is that correct?

I mean, this code still runs on linux and darwin machines, right? It's just that we make no use of the "update runner"...

Copy link
Contributor Author

@deepfire deepfire Dec 4, 2018

Choose a reason for hiding this comment

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

@KtorZ, the runnerPath = udWindowsPath ud :: Maybe FilePath governs whether writeWindowsUpdaterRunner will be run, and is naturally Nothing on non-Windows. We can fix the type defaulting, of course, if that's critical.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To clarify, in other circumstances I'd not resort to -Wno-type-default, but I needed a sure-fire way to get past the thing quick -- and I don't have ready access to a Windows machine..

..and this is a release blocker..

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, got you (hence why I haven't blocked anything on this PR). I was just wondering. One way to avoid the warning would be to go for a string right away show <$> Process.getCurrentProcessId, but that's really minor.

@deepfire
Copy link
Contributor Author

deepfire commented Dec 4, 2018

@alan-mcnicholas confirmed the fix was a success!

@deepfire
Copy link
Contributor Author

deepfire commented Dec 4, 2018

bors r+

iohk-bors bot added a commit that referenced this pull request Dec 4, 2018
3926: [RCD-52]  cardano-launcher | win64:  kill self via PID, not process name r=deepfire a=deepfire





Co-authored-by: Kosyrev Serge <[email protected]>
@deepfire deepfire merged commit a43f378 into release/2.0.0 Dec 4, 2018
@iohk-bors iohk-bors bot deleted the rcd-52-indiscriminate-killing branch December 4, 2018 16:01
@deepfire
Copy link
Contributor Author

deepfire commented Dec 4, 2018

Merged, to expedite Daedalus build.

@KtorZ KtorZ mentioned this pull request Jan 4, 2019
12 tasks
@disassembler disassembler restored the rcd-52-indiscriminate-killing branch January 14, 2019 15:05
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.

3 participants