search

digital-asset / daml

The Daml smart contract language
https://www.digitalasset.com/developers
796 stars 201 forks source link

Make `daml json-api` use the ledger.host and ledger.port options in daml.yaml #3147

Open GeorgSchneider opened 4 years ago

GeorgSchneider commented 4 years ago

Currently, it does not use them so I need to specify --ledger-host and --ledger-port explicitly, even if ledger.host and ledger.port are present in daml.yaml

ghost commented 4 years ago

daml json-api runs the jar produced by the runtime team (cc @gerolf-da @leo-da). This jar should read the project config for ledger.host and ledger.port if --ledger-host or --ledger-port are missing.

leo-da commented 4 years ago

I personally think the daml.yaml configuration reading should be implemented as part of DAML assistant. Instead of pushing this logic into every command/executable that assistant calls we can do it in one place. Assistant can read the settings in a generalized way and pass them via existing CLI arguments.

ghost commented 4 years ago

@leo-da That is not the responsibility of daml-assistant, it is only concerned with SDK versioning, installation, and command line redirection via sdk-config.yaml. It on purpose doesn't have any knowledge of what's inside daml.yaml besides the sdk-version field.

If you wish to change the way daml-helper runs json-api.jar by adding a specific command in Haskell, go ahead. I think that it is roughly the same effort as reading the configuration in scala.

If you are going to do add the fallbacks (for --ledger-host and --ledger-port) in Scala, please use the library @rautenrieth-da wrote for reading daml.yaml.

leo-da commented 4 years ago

@bame-da please prioritize.

gerolf-da commented 4 years ago

@bame-da, @associahedron: I have the same understanding as @leo-da. The tools accept various parameters that the SDK Assistant can/should pass along. Trying to read a daml.yaml file doesn't make sense as soon as you want to start the application/tool outside of a DAML SDK context. It also distributes the mechanics of how this data is read across multiple applications instead of just the SDK Assistant. WDYT?

ghost commented 4 years ago

@gerolf-da The JAR is distributed as part of the SDK -- it doesn't make sense to consider running it outside of the DAML SDK, and it's not something we should support. Feel free to distribute a standalone version of the JAR, but there's no reason it has to be the same JAR as the one we ship in the SDK. So, let's work with the premise that the JAR is part of the SDK, not a standalone program.

The daml assistant is super agnostic to the inner workings of the SDK. That's by design -- it makes the assistant extremely compatible across multiple SDK versions, which it has to be. (The previous da assistant kept breaking because it didn't do this.) So any specific behavior has to come inside the SDK itself.

Currently the json-api command is hooked up to the JAR, which means the JAR has to do the options parsing itself. It's possible to have a daml-helper wrapper for the json-api command which passes arguments to the JAR in a special way. But it's not like you're saving any work over just reading the arguments properly in the JAR, given @rautenrieth-da's scala library for looking at the daml.yaml project config, which is used in the navigator.

bame-da commented 4 years ago

Figuring out the project root, and reading in the project config sounds like something that should be done in a centralised place like the assistant. Mapping that config to command line parameters or environment variables for the individual tools should probably sit in the daml-helper of each tool.

I don't have a strong feeling about where the code sits, though. Mostly agree with @georg-da that the API should respect the daml.yaml settings, and with @associahedron that running the JAR independent from the SDK is not important.

leo-da commented 4 years ago

I think we need just one generic daml-helper tool that knows how to convert daml.yaml settings into CLI arguments. Does not matter if it is written in Haskell and it is part of the assistant or written in Scala and just wraps the Main of the tool that assistant delegates to.

hurryabit commented 4 years ago

I'm not convinced having this sort of logic in daml-helper is a good idea. It means we're replicating the command line flags of every tool in at least two places: the tool itself and its wrapper command in daml-helper. The mapping between daml.yaml options and the command line options for a tool is daml-helper is very likely to get out of sync, as we know from the past. Instead, we should have all the logic for a tool's configuration, be it via daml.yaml or command line flags, in a single place, namely the tool. To make this as easy as possible, we should provide a Scala library to locate and read the daml.yaml. In fact, we already have such a library and it is successfully used by navigator: https://github.com/digital-asset/daml/tree/master/daml-assistant/scala-daml-project-config I would call this a success story and prefer very much to continue in this style!

leo-da commented 4 years ago

It means we're replicating the command line flags of every tool in at least two places

that is not what I meant when said that we should implement it in a generic way.

To start HTTP JSON API you call: daml json-api. The helper might be written the way so it checks json-api element of daml.yaml and tries to convert all json-api child elements into CLI args (not knowing anything about what options are currently supported, if you mess any option in the daml.yaml you are going to get an error from json-api CLI parsing logic). Sure there will be common settings, like ledger-host and ledger-port.

leo-da commented 4 years ago

However if we only care about daml json-api ledger host and ledger port configurations... yeah it is way easier to do this config read as part of the json-api Main. And yeah the library is awesome and as a matter of fact it is also being used in the java/scala codegen front-end (daml codegen) and that is exactly the reason why I brought up the idea of a generic helper/utility here. The amount of code to parse daml.yaml safely is a bit more than I would like to have.

See https://github.com/digital-asset/daml/blob/master/language-support/codegen-common/src/main/scala/com/digitalasset/daml/lf/codegen/conf/CodegenConfigReader.scala

leo-da commented 4 years ago

@GeorgSchneider you understand that the way you wrote the issue you only going to get ledger-host and ledger-port derived from the daml.yaml and nothing else :).

GeorgSchneider commented 4 years ago

I didn't expect such a controversial discussion to spark out of that issue. @gerolf-da and @hurryabit can you align and come to a decision?

@leo-da : if there are more options that make sense to take out of daml.yaml of course please do so. The ticket doesn't have to be taken literally.