Skip to content

Etcm 52 make sure mantis works on windows #671

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

Merged
merged 7 commits into from
Sep 18, 2020

Conversation

kapke
Copy link
Contributor

@kapke kapke commented Sep 16, 2020

Description

This PR fixes issues found when trying to run Mantis on Windows, that is:

  • generating proper .bat files
  • fixing the way SecureRandom instance is obtained
  • KeyStore tests which were too UNIX-oriented
  • Update RocksDB to latest version, so Ethereum fork doesn't need to be used

Testing

All tests, sbt dist and syncing from both sbt and dist package should work

@kapke kapke marked this pull request as ready for review September 16, 2020 09:08
@kapke kapke force-pushed the etcm-52-make-sure-mantis-works-on-windows branch from 8ba58a3 to 661d333 Compare September 16, 2020 09:09
@kapke kapke requested review from mirkoAlic, KonradStaniec and ntallar and removed request for mirkoAlic September 16, 2020 09:10
@kapke kapke self-assigned this Sep 16, 2020
Copy link

@ntallar ntallar left a comment

Choose a reason for hiding this comment

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

Both running Mantis from sbt dist or sbt run worked on my machine! (it's a linux machine)

@@ -510,7 +510,7 @@ trait GenesisDataLoaderBuilder {

trait SecureRandomBuilder {
lazy val secureRandom: SecureRandom =
Config.secureRandomAlgo.map(SecureRandom.getInstance).getOrElse(new SecureRandom())
Config.secureRandomAlgo.flatMap(name => Try(SecureRandom.getInstance(name)).toOption).getOrElse(new SecureRandom())
Copy link

Choose a reason for hiding this comment

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

Doesn't this hide the fact that the selected algorithm is available and other one is instead being used?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If I add logging about algorithm non available, will it be sufficient?

Copy link

Choose a reason for hiding this comment

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

Yes, given our current handling of config errors I think that with logging with error will be enough here

Copy link
Contributor

@KonradStaniec KonradStaniec left a comment

Choose a reason for hiding this comment

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

I had run mantis on macos, from pre-exsiting database and it seems to work fine. @kapke did you check it on windows ?

@kapke
Copy link
Contributor Author

kapke commented Sep 17, 2020

I did, whole PR was prepared on Windows machine

@kapke kapke force-pushed the etcm-52-make-sure-mantis-works-on-windows branch 2 times, most recently from 6a08ed2 to 811f3d7 Compare September 17, 2020 13:34
@kapke kapke added the high priority This PRs should be reviewed and merged ASAP label Sep 17, 2020
@kapke kapke force-pushed the etcm-52-make-sure-mantis-works-on-windows branch from 952a4f9 to 02510ad Compare September 17, 2020 14:37
@kapke kapke force-pushed the etcm-52-make-sure-mantis-works-on-windows branch from 02510ad to f270dac Compare September 17, 2020 14:55
Copy link

@ntallar ntallar left a comment

Choose a reason for hiding this comment

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

LGTM!

 .\\            //.
. \ \          / /.
.\  ,\     /` /,.-
 -.   \  /'/ /  .
 ` -   `-'  \  -
   '.       /.\`
      -    .-
      :`//.'
      .`.'
      .' BP 

@kapke kapke merged commit 56e9336 into develop Sep 18, 2020
@kapke kapke deleted the etcm-52-make-sure-mantis-works-on-windows branch September 18, 2020 10:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
high priority This PRs should be reviewed and merged ASAP
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants