New CLI tool #3085

LepilkinaElena · 2019-06-19T17:51:16Z

Please, review style of usage new CLI tool that we discuss. Work in progress, I'll replace it and add to default libraries of K/N compiler, add more tests.
There is no all features we discussed https://jetbrains.quip.com/izMpAJIY2djt/CLI-design-meeting, but I amn't sure if we should cover most features in one moment.

But still your comments about approach of usage and design are appreciated.
I don't know if libraries require JavaDocs, I looked mostly in compiler, there is no JavaDocs.

kliopt/src/tests/ArgumentsTests.kt

cy6erGn0m · 2019-06-20T15:33:10Z

kliopt/src/main/kotlin/org/jetbrains/kliopt/ArgType.kt

+                    ?: error("Option $name is expected to be double number. $value is provided.\n$helpMessage") }
+    }
+
+    class Choice(val values: List<kotlin.String>) : ArgType<kotlin.String>(true) {


Shouldn't some characters be prohibited in values such as spaces and commas?

I amn't sure why we should provide such limitations, it can be any string value. What problems do you see?

kliopt/src/main/kotlin/org/jetbrains/kliopt/ArgType.kt

ilya-g

In general, it would be easier to review, if you had a design document about this somewhere. Then some of the questions that arise during the review could be answered by that document. For example now it's unclear which features are not implemented yet and which are intentionally omitted.

My first impressions of this design are:

hard to correspond terms argument to positional arguments and option to named arguments.
different style of subcommand declaration: subcommand parsers inherit Subcommand, but root parser just instantiate ArgParser.
callback invocation style of subcommands (abstract fun execute) is dubious. What if I want to parse arguments, then find what is the invoked subcommand and switch based on its type?
it would be convenient if an argument with non-null default value could be obtained as non-nullable, e.g. val x: String by argument(ArgType.String, "x", defaultValue="y")
missing multivalue argument can default to an empty list
having to specify the full parameter name in combination with delegation of the same named property violates DRY principle
it might be more convenient to specify default argument value in the target type rather than in string form

LepilkinaElena · 2019-06-23T02:17:01Z

now it's unclear which features are not implemented yet and which are intentionally omitted.

I just marked features in document from design meeting https://jetbrains.quip.com/izMpAJIY2djt/CLI-design-meeting. Should I create one more detailed document or add details to this one?

hard to correspond terms argument to positional arguments and option to named arguments

As far as I know argument/option are well-known terms in this area, that are used in most part of such libraries. Why their are hard? Arguments are also be named.

different style of subcommand declaration: subcommand parsers inherit Subcommand, but root parser just instantiate ArgParser

What design would you suggest? I looked at

val cli = CommandLineInterface("testNestedCommands", addHelp = false)

        val commons = cli.helpEntriesGroup("Common arguments:")
        commons.help()
        val commonFlag by commons.flagArgument("--common", "common flag for foo and bar")

        val fooCmd = CommandLineInterface("foo")
        val fooX by fooCmd.positionalArgument("X", "X argument for foo")
        val fooY by fooCmd.positionalArgument("Y", "Y argument for foo")

        val barCmd = CommandLineInterface("bar")
        val barX by barCmd.positionalArgument("X", "X argument for bar")
        val barFlag by barCmd.flagArgument("--flag", "flag argument for bar")

        cli.subcommands("COMMAND", "Available subcommands:",
                Subcommand(fooCmd, "foo subcommand", { foo(commonFlag, fooX, fooY) }),
                Subcommand(barCmd, "bar subcommand", { bar(commonFlag, barX, barFlag) })
        )

But I see here problems with setting common options for different subcommands, I should repeat them and it's hard to understand what is common part. Also when user have many options he gets functions with big number of parameters, that looks very bad.

callback invocation style of subcommands (abstract fun execute) is dubious. What if I want to parse arguments, then find what is the invoked subcommand and switch based on its type?

It's connected with previous point. But here I don't understand use case, switch is made by parser, all what you need you can do in action.

having to specify the full parameter name in combination with delegation of the same named property violates DRY principle

There are cases when name differs. I can add overload without name.

LepilkinaElena · 2019-07-08T17:27:38Z

Document with design decisions https://jetbrains.quip.com/gXgYABJEbiwt/Kotlin-CLI-design

ilmat192 · 2019-07-11T10:24:39Z

defaultLibraries/kliopt/build.gradle

+
+def commonSrc = new File("$projectDir/src/main")
+
+targetList.each { target ->


Why not use the MPP plugin to build the klib too?

After discussion decided to postpone because of crossDist targets.

build.gradle

ilmat192 · 2019-07-11T11:23:19Z

Interop/StubGenerator/build.gradle

 dependencies {
    compile "org.jetbrains.kotlin:kotlin-stdlib:$buildKotlinVersion"
    compile "org.jetbrains.kotlin:kotlin-compiler:$buildKotlinVersion"
    compile project(':Interop:Indexer')
    compile "org.jetbrains.kotlin:kotlin-native-shared:$konanVersion"
+    compile files("$rootDir/defaultLibraries/kliopt/build/libs/kliopt-jvm.jar")


Why not just project("defaultLibraries:kliopt")?

LepilkinaElena · 2019-07-15T13:34:45Z

There also some new things in https://jetbrains.quip.com/gXgYABJEbiwt/Kotlin-CLI-design

ilmat192 · 2019-07-16T04:26:49Z

Approve the Gradle-related part.

Interop/StubGenerator/build.gradle

olonho · 2019-07-16T06:08:12Z

...rop/StubGenerator/src/main/kotlin/org/jetbrains/kotlin/native/interop/gen/jvm/CommandLine.kt

+open class CommonInteropArguments(val argParser: ArgParser) {
+    val verbose by argParser.option(ArgType.Boolean, description = "Enable verbose logging output", defaultValue = false)
+    val flavor by argParser.option(ArgType.Choice(listOf("jvm", "native", "wasm")), description = "Interop target",
+            defaultValue = "jvm")


BTW, this shall be changed to native.

As I understood here is jvm, because this flavor is default in NativeInteropPlugin which we actively use in compiler exactly for jvm.
Change this? May be in separate review?

olonho · 2019-07-16T06:10:32Z

cmd/run_konan

@@ -89,12 +89,13 @@ SHARED_JAR="${KONAN_HOME}/konan/lib/shared.jar"
 EXTRACTED_METADATA_JAR="${KONAN_HOME}/konan/lib/konan.metadata.jar"
 EXTRACTED_SERIALIZER_JAR="${KONAN_HOME}/konan/lib/konan.serializer.jar"
 KLIB_JAR="${KONAN_HOME}/konan/lib/klib.jar"
+KLIOPT_JAR="${KONAN_HOME}/konan/lib/kliopt-jvm.jar"


Not sure if I like this approach. Can we avoid adding new JARs and repack to fewer JARs?

We discussed with Ilya, we need to think about right repacking all jars, but for now it's only possible approach without big refactoring.

olonho · 2019-07-16T06:12:05Z

defaultLibraries/kliopt/src/main/kotlin/org/jetbrains/kliopt/ExperimentalCli.kt

+ * annotating that usage with the [UseExperimental] annotation, e.g. `@UseExperimental(ExperimentalCli::class)`,
+ * or by using the compiler argument `-Xuse-experimental=org.jetbrains.kliopt.ExperimentalCli`.
+ */
+@Experimental(level = Experimental.Level.ERROR)


Why so harsh?

Set another level? I made same wsy as is done for stdlib in example which @ilya-g sent.

Given that the library itself is not stable, I don't insist that the level should be error as it is in stdlib experimental annotations. Here we use it as a marker of a less designed part of API.

ilya-g

I don't mind merging it in Native as an internal tool, but further design review is required before promoting this tool in public.

endorsedLibraries/kliopt/src/main/kotlin/org/jetbrains/kliopt/ArgType.kt

ilya-g · 2019-07-19T02:54:57Z

defaultLibraries/kliopt/src/main/kotlin/org/jetbrains/kliopt/ExperimentalCli.kt

+ * annotating that usage with the [UseExperimental] annotation, e.g. `@UseExperimental(ExperimentalCli::class)`,
+ * or by using the compiler argument `-Xuse-experimental=org.jetbrains.kliopt.ExperimentalCli`.
+ */
+@Experimental(level = Experimental.Level.ERROR)


Given that the library itself is not stable, I don't insist that the level should be error as it is in stdlib experimental annotations. Here we use it as a marker of a less designed part of API.

LepilkinaElena · 2019-07-19T08:53:40Z

further design review is required before promoting this tool in public

@ilya-g I've already added in this review mechanism for endorsed libraries and add kliot as first.
Now this library is accessible from code on Kotlin/Native, so it isn't internal.

What should be next steps in design to get approval to merge this PR fully?

LepilkinaElena requested review from elizarov, olonho and ilya-g June 19, 2019 17:51

olonho reviewed Jun 20, 2019

View reviewed changes

kliopt/src/tests/ArgumentsTests.kt Outdated Show resolved Hide resolved

cy6erGn0m reviewed Jun 20, 2019

View reviewed changes

kliopt/src/main/kotlin/org/jetbrains/kliopt/ArgType.kt Outdated Show resolved Hide resolved

ilya-g reviewed Jun 21, 2019

View reviewed changes

LepilkinaElena force-pushed the arg_parser branch from f9aed67 to e546006 Compare July 8, 2019 17:26

LepilkinaElena requested review from olonho and ilya-g July 8, 2019 17:29

Elena Lepilkina added 5 commits July 9, 2019 15:54

Started rewriting argParser

a816793

Type as object

bb2a706

Default values rework

6779dfe

Fixes

1c67776

Added library to compile resolve mechanism

ab1f0bb

LepilkinaElena force-pushed the arg_parser branch from e546006 to ab1f0bb Compare July 11, 2019 09:48

ilmat192 reviewed Jul 11, 2019

View reviewed changes

Elena Lepilkina added 4 commits July 11, 2019 15:07

Use jvmJar task

1fbe134

Forgotten changes in preformance project

4fe7e5b

Fixes for performance subproject

a1e7e5e

Fixes + tests for help messages

dd67d9c

LepilkinaElena changed the title ~~WIP: New CLI tool~~ New CLI tool Jul 15, 2019

ilmat192 approved these changes Jul 16, 2019

View reviewed changes

olonho reviewed Jul 16, 2019

View reviewed changes

Interop/StubGenerator/build.gradle Outdated Show resolved Hide resolved

olonho reviewed Jul 16, 2019

View reviewed changes

Renamed default to endorsed

2319df4

olonho approved these changes Jul 18, 2019

View reviewed changes

ilya-g reviewed Jul 19, 2019

View reviewed changes

Elena Lepilkina added 2 commits July 19, 2019 12:08

Fix typo and warning level

7834699

Fix command line argumens style

849c409

LepilkinaElena merged commit 3c2a5b0 into master Jul 22, 2019

LepilkinaElena deleted the arg_parser branch July 23, 2019 07:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New CLI tool #3085

New CLI tool #3085

LepilkinaElena commented Jun 19, 2019

cy6erGn0m Jun 20, 2019

LepilkinaElena Jun 23, 2019

ilya-g left a comment

LepilkinaElena commented Jun 23, 2019 •

edited

Loading

LepilkinaElena commented Jul 8, 2019

ilmat192 Jul 11, 2019

LepilkinaElena Jul 11, 2019

ilmat192 Jul 11, 2019

LepilkinaElena commented Jul 15, 2019

ilmat192 commented Jul 16, 2019

olonho Jul 16, 2019

LepilkinaElena Jul 16, 2019 •

edited

Loading

olonho Jul 16, 2019

LepilkinaElena Jul 16, 2019

olonho Jul 16, 2019

LepilkinaElena Jul 16, 2019

ilya-g Jul 19, 2019

ilya-g left a comment

ilya-g Jul 19, 2019

LepilkinaElena commented Jul 19, 2019


		def commonSrc = new File("$projectDir/src/main")

		targetList.each { target ->

New CLI tool #3085

New CLI tool #3085

Conversation

LepilkinaElena commented Jun 19, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ilya-g left a comment

Choose a reason for hiding this comment

LepilkinaElena commented Jun 23, 2019 • edited Loading

LepilkinaElena commented Jul 8, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LepilkinaElena commented Jul 15, 2019

ilmat192 commented Jul 16, 2019

Choose a reason for hiding this comment

LepilkinaElena Jul 16, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ilya-g left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LepilkinaElena commented Jul 19, 2019

LepilkinaElena commented Jun 23, 2019 •

edited

Loading

LepilkinaElena Jul 16, 2019 •

edited

Loading