Skip to content

Conversation

@enriquerodbe
Copy link
Contributor

@enriquerodbe enriquerodbe commented Dec 22, 2020

Description

See #851

Proposed Solution

As @aakoshh commented in the issue, one solution is to extend the the generated bash script file so that we can set the config files paths correctly.

Testing

These instructions should work for Unix and for Windows (just by changing / to \).

  1. Build: sbt dist
  2. Move to target/universal: cd target/universal
  3. Unzip: unzip mantis-3.2.1.zip
  4. Verify the following commands

Should use etc by default (this is the test for the original issue)

  • mantis-3.2.1/bin/mantis - Check that it starts the client and connects to etc network.

Should use given chain

  • mantis-3.2.1/bin/mantis-launcher sagano - Check that it starts and connects to sagano testnet.

Should not break other scripts

All of the following should be valid and work as usual

  • mantis-3.2.1/bin/mantis-vm 8080
  • mantis-3.2.1/bin/eckeygen 8
  • mantis-3.2.1/bin/signatureValidator f0b91b55bdea6a6da2bcbd1270451e9ca014747b97e190a5a4275bad8eeda7436ab8081f0df5741a86a1bfb62798221103fff5898c4f768b707ff34b110329ed 6bf862422f0103d8ca6847019c0113c9b343a2489c2dc3fc67d853e87d98f68e5a5be1d021ec706197960912037d46bde44c3534a4ff562fd48e664f32c1392d1c 5fe7f977e71dba2ea1a68e21057beebb9be2ac30c6410aa38d4f3fbe41dcffd2
  • mantis-3.2.1/bin/faucet-server

@enriquerodbe enriquerodbe added the bug Something isn't working label Dec 22, 2020
@enriquerodbe enriquerodbe self-assigned this Dec 22, 2020
Copy link
Contributor

@dmitry-worker dmitry-worker left a comment

Choose a reason for hiding this comment

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

Looks good overall, but wouldn't there be a problem for a MS Windows OS?
Just to be sure, because that OS won't treat / as a file separator

Maybe there should also be batScriptDefines...

@enriquerodbe
Copy link
Contributor Author

Looks good overall, but wouldn't there be a problem for a MS Windows OS?
Just to be sure, because that OS won't treat / as a file separator

Maybe there should also be batScriptDefines...

Yes good catch 👍

@enriquerodbe enriquerodbe linked an issue Dec 22, 2020 that may be closed by this pull request
Copy link
Contributor

@aakoshh aakoshh left a comment

Choose a reason for hiding this comment

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

Awesome, thanks for following up on this!

@aakoshh
Copy link
Contributor

aakoshh commented Dec 22, 2020

I was actually thinking that it might be a good idea to use this option to add script file that does what mantis-launcher itself is doing right now: check if there's an argument that specifies the chain (if there need to be one), exit if there isn't, or set the config file if there is and pop that argument before lanuching the Java app. So mantis-launcher itself wouldn't do anything but call mantis, for backwards compatibility. This way the other complaint would be covered, which was that nobody thinks about using mantis-launcher when there's mantis right there.

Just have to make sure that all the other launchers don't get hijacked by this logic, so maybe it could check if the first argument looks like a .conf file and do the conversion, or exit if there are no arguments at all, otherwise carry on.

@enriquerodbe
Copy link
Contributor Author

Ok this seems to work as expected now with bash script. I haven't had the opportunity to test the Windows batch file.

@enriquerodbe
Copy link
Contributor Author

I would propose that executing just the start script ./bin/mantis should start and use etc network as the default. This way we can remove the check to see if there are any given arguments at all.

If we want to require a "chain" parameter I'll have to add a known issue because doing something like ./bin/mantis -d would effectively bypass the requirement and use etc.

@lemastero
Copy link
Contributor

Tested on Mac - works nicely.

@enriquerodbe enriquerodbe merged commit 279627f into develop Dec 30, 2020
@enriquerodbe enriquerodbe deleted the feature/ETCM-491-launch-dir branch December 30, 2020 15:51
CHAIN_PARAM="-Dconfig.file=$CONFIG_FILE"
fi

./bin/mantis ${CHAIN_PARAM:+"$CHAIN_PARAM"} "$@"
Copy link
Contributor

Choose a reason for hiding this comment

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

What does this fallback from CHAIN_PARAM to itself achieve?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I found this is the way in bash to not add anything if CHAIN_PARAM is not set. Otherwise it adds the empty string and makes the command fail

@aakoshh
Copy link
Contributor

aakoshh commented Jan 4, 2021

If I understand it correctly it will launch etc by default because if you don't specify anything then it will use these defaults. That means it will not use the conf/etc.conf file, correct?

I can imagine someone would find that confusing if they tried to add some setting to the "default etc" config and not seeing any effect.

@enriquerodbe
Copy link
Contributor Author

If I understand it correctly it will launch etc by default because if you don't specify anything then it will use these defaults. That means it will not use the conf/etc.conf file, correct?

I can imagine someone would find that confusing if they tried to add some setting to the "default etc" config and not seeing any effect.

Right. Maybe I'll explicitly set the config to etc.conf if the user doesn't choose any chain.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Mantis cannot be launched except from its install dir

5 participants