Skip to content

Etcm 52 make sure mantis works on windows#671

Merged
kapke merged 7 commits intodevelopfrom
etcm-52-make-sure-mantis-works-on-windows
Sep 18, 2020
Merged

Etcm 52 make sure mantis works on windows#671
kapke merged 7 commits intodevelopfrom
etcm-52-make-sure-mantis-works-on-windows

Conversation

@kapke
Copy link
Copy Markdown
Contributor

@kapke kapke commented Sep 16, 2020 •
edited
Loading

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 KonradStaniec, mirkoAlic and ntallar and removed request for mirkoAlic September 16, 2020 09:10
@kapke kapke self-assigned this Sep 16, 2020
ntallar
ntallar suggested changes Sep 16, 2020
View reviewed changes
Copy link
Copy Markdown

@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)

Comment thread build.sbt Outdated
Comment thread build.sbt Outdated
Comment thread src/main/scala/io/iohk/ethereum/db/dataSource/RocksDbDataSource.scala
Comment thread src/main/scala/io/iohk/ethereum/nodebuilder/NodeBuilder.scala Outdated
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
Copy Markdown

@ntallar ntallar Sep 16, 2020

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
Copy Markdown
Contributor Author

@kapke kapke Sep 17, 2020

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
Copy Markdown

@ntallar ntallar Sep 17, 2020

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

KonradStaniec
KonradStaniec approved these changes Sep 17, 2020
View reviewed changes
Copy link
Copy Markdown
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
Copy Markdown
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
ntallar
ntallar approved these changes Sep 17, 2020
View reviewed changes
Copy link
Copy Markdown

@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
@kapke kapke mentioned this pull request Sep 18, 2020
Merged
This was referenced Sep 18, 2020
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

2 more reviewers

@KonradStaniec KonradStaniec KonradStaniec approved these changes

@ntallar ntallar ntallar approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@kapke kapke

Labels

high priority This PRs should be reviewed and merged ASAP

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@kapke @KonradStaniec @ntallar