Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[DDW-544] Fix 'Daedalus' state directory on 'Staging' and 'Testnet' #1316

Conversation

thedanheller
Copy link
Contributor

This PR fixes 'Daedalus' state directory on 'Staging' and 'Testnet' builds.

Since the Electron upgrade, the Staging and Testnet builds on Windows are creating a “Daedalus” state directory, within which they add an empty “logs” directory. It seems to be a known Electron issue.

Screenshots:

screen_shot_2019-01-04_at_17 39 44


Review Checklist:

Basics

  • PR is updated to the most recent version of target branch (and there are no conflicts)
  • PR has good description that summarizes all changes and shows some screenshots or animated GIFs of important UI changes
  • CHANGELOG entry has been added and is linked to the correct PR on GitHub
  • Automated tests: All acceptance tests are passing (yarn run test)
  • Manual tests (minimum tests should cover newly added feature/fix): App works correctly in development build (yarn run dev)
  • Manual tests (minimum tests should cover newly added feature/fix): App works correctly in production build (yarn run package / CI builds)
  • There are no flow errors or warnings (yarn run flow:test)
  • There are no lint errors or warnings (yarn run lint)
  • Text changes are proofread and approved (Jane Wild)
  • There are no missing translations (running yarn run manage:translations produces no changes)
  • UI changes look good in all themes (Alexander Rukin)
  • Storybook works and no stories are broken (yarn run storybook)
  • In case of dependency changes yarn.lock file is updated

Code Quality

  • Important parts of the code are properly documented and commented
  • Code is properly typed with flow
  • React components are split-up enough to avoid unnecessary re-rendering
  • Any code that only works in Electron is neatly separated from components

Testing

  • New feature / change is covered by acceptance tests
  • All existing acceptance tests are still up-to-date
  • New feature / change is covered by Daedalus Testing scenario
  • All existing Daedalus Testing scenarios are still up-to-date

After Review:

  • Merge PR
  • Delete source branch
  • Move ticket to done on the Youtrack board

@thedanheller
Copy link
Contributor Author

@DominikGuzei @nikolaglumac I made some attempts of playing around with package.js, but it didn't work. Do you have any suggestion? Maybe something here?

@nikolaglumac
Copy link
Contributor

@daniloprates it is important to figure out whether this default Daedalus folder is created at the installation time or during the first Daedalus launch. This could also be an Electron bug. The application name is different for each of the 3 applications (Daedalus, Daedalus Staging and Daedalus Testnet).

@nikolaglumac
Copy link
Contributor

@nikolaglumac
Copy link
Contributor

@daniloprates I would suggest you try to disable logging and check if the Daedalus directory is created in that case. I think it should be enough to comment out this line: https://github.com/input-output-hk/daedalus/blob/develop/source/main/index.js#L54

@thedanheller
Copy link
Contributor Author

@daniloprates it is important to figure out whether this default Daedalus folder is created at the installation time or during the first Daedalus launch. This could also be an Electron bug. The application name is different for each of the 3 applications (Daedalus, Daedalus Staging and Daedalus Testnet).

@nikolaglumac Daedalus folder is only created during the first Daedalus launch. Cool, I'll test these suggestions.

@thedanheller
Copy link
Contributor Author

@nikolaglumac removing the logs didn't work. Still creating the Daedalus folder.

@thedanheller
Copy link
Contributor Author

@nikolaglumac @DominikGuzei failed attempts so far:

@DominikGuzei
Copy link
Member

@daniloprates how should hardcoding name: 'Daedalus' result in anything else but a Daedalus folder being created? 😄 I would rather try to hardcode Daedalus Staging etc. (the correct name) … my guess is that electron-builder just picks the productName in package.json so i would try to rewrite that property before building + change it back afterwards.

@nikolaglumac
Copy link
Contributor

@cleverca22 any idea about this one? It's a Windows only issue :(

@thedanheller
Copy link
Contributor Author

Tried to rename productName when running package.js, but it didn't work.

@cleverca22 cleverca22 force-pushed the fix/ddw-544-fix-daedalus-state-directory-on-staging-and-testnet-builds branch 13 times, most recently from 242109f to 46f604e Compare March 2, 2019 15:33
@DmitriiGaico
Copy link

Please see my findings below.
Steps and results provided in Chronological order. All the steps and results are valid for Test and Staging nets.

  1. Install Daedalus TestNet on a clean Windows VM. No Daedalus Folder was created.
  2. After installation a Blank short cut was created pointing to "Daedalus.exe" file.
  3. After running shortcut the error was displayed. (ScreenShot attached)
  4. In the root folder the .exe file created with the name "Daedalus Testnet.exe"
  5. Manually renamed Daedalus Testnet.exe > Daedalus.exe. VM Reboot.
  6. After VM Reboot the shortcut displayed as expected.
  7. Running Daedalus Shortcut caused the app to Run Just fine. After App started and Sync process started "Daedalus" folder was not created.
    testnet

@nikolaglumac
Copy link
Contributor

Looks like we are close to getting this fixed - can you help us fix the last part @cleverca22 🙏

@cleverca22
Copy link
Contributor

the dhall files will likely need to be updated (perhaps on every platform) to account for the new name for electron

@nikolaglumac nikolaglumac removed the WIP label Mar 5, 2019
@DmitriiGaico
Copy link

DmitriiGaico commented Mar 5, 2019

@cleverca22 Hello! I'll do some tests on Linux but for the Windows results are below.
Green: 1. After Installing and Starting "TestNet / Staging" no Daedalus folder created. 2. App can be started via desktop shortcut with no errors.
RED : 1. ShortCuts are still Blank.
shortcuts

NOTE :
Linux ShortCuts looks as expected. No issues.

@nikolaglumac
Copy link
Contributor

So we just need to sort out the icons - thanks for looking in to it @cleverca22 🙏

@nikolaglumac
Copy link
Contributor

@cleverca22 here's the root of the problem with the desktop icons:

image

@nikolaglumac
Copy link
Contributor

@cleverca22 Mainnet installer has no icon issues on Windows 🎉

@cleverca22 cleverca22 force-pushed the fix/ddw-544-fix-daedalus-state-directory-on-staging-and-testnet-builds branch from b8913b4 to fe67767 Compare March 6, 2019 09:01
@nikolaglumac nikolaglumac removed the WIP label Mar 6, 2019
@nikolaglumac nikolaglumac self-requested a review March 6, 2019 09:19
Copy link

@DmitriiGaico DmitriiGaico left a comment

Choose a reason for hiding this comment

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

Tested at Windows / Linux. All Green.

@nikolaglumac nikolaglumac merged commit 4efdf14 into develop Mar 6, 2019
@iohk-bors iohk-bors bot deleted the fix/ddw-544-fix-daedalus-state-directory-on-staging-and-testnet-builds branch March 6, 2019 14:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants