Skip to content
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

Improved CLI #287

Merged
merged 39 commits into from
May 2, 2024
Merged

Improved CLI #287

merged 39 commits into from
May 2, 2024

Conversation

vitorpy
Copy link
Contributor

@vitorpy vitorpy commented Apr 23, 2024

Closes #1516
Closes #1517
Closes #1518
Closes #1519

Uses meow to rewrite the CLI. Things I added in this PR:

  • Compile single file
  • Implement --func
  • Implement --check
  • Test the release process to amke sure it packages properly
  • Remove the old bin/tact file.
  • Update GitHub workflow

Does this sounds reasonable?

  • I have updated CHANGELOG.md
  • I have added unit tests to demonstrate the contribution is correctly implemented
  • I have run all the tests locally and no test failure was reported
  • I did not do unrelated and/or undiscussed refactorings

@anton-trunov anton-trunov requested a review from novusnota April 24, 2024 07:19
@anton-trunov anton-trunov added the scope: cli Tact's command-line interface (bin) label Apr 24, 2024
@novusnota
Copy link
Member

@vitorpy Hey, nice initiative! But adding oclif is a bit too much as it's a full-blown suite for making CLIs and it implicitly brings over its structure layout. This could've been the issue with the tests failing to find ./dist/grammar/ all of the sudden.

If you want to change the current argument parser library from arg, I'd suggest using meow as it's targets easy creation of single-command CLIs, like Tact compiler. Similar in spirit to docopt, you only need to specify your desired help message and then the tool will parse all the arguments and options based on that. Here's an example of it in the app, which uses ESM.

Additionally, when it comes to --func option, this was in the works of @anton-trunov, so we shall first align the work with him :)

@vitorpy
Copy link
Contributor Author

vitorpy commented Apr 24, 2024

If you want to change the current argument parser library from arg, I'd suggest using meow as it's targets easy creation of single-command CLIs, like Tact compiler.

Cool - I haven't seen meow but it looks nice. I'll rewrite it sometime later this week. I agree with oclif being a bit bloated for something simple as the tact cli.

Additionally, when it comes to --func option, this was in the works of @anton-trunov, so we shall first align the work with him :)

Yeah, that's why I started to look into this. I wanted to look at FunC outputs (and from there to Fift outputs).

@anton-trunov
Copy link
Member

anton-trunov commented Apr 24, 2024

I think it's ok to work on the --func CLI flag because I just did a quick-and-dirty hack to help me with hunting down a nasty bug related to the WASM version of the FunC compiler (I basically added an option like funcOnly for each project in tact.config.json and --func just overrides all of those options).

@anton-trunov
Copy link
Member

Hey @vitorpy, do you think we can still manage do this CLI improvement before 1.3.0 release? Btw, I think we could also add --check CLI flag to stop the compilation pipeline right after typechecking contracts.

@vitorpy
Copy link
Contributor Author

vitorpy commented Apr 27, 2024

Yes, let's aim for the 1.3 release. I should have some time this weekend to look into this. I'll add --check as well!

@vitorpy
Copy link
Contributor Author

vitorpy commented Apr 27, 2024

@novusnota Hey, can you take a look? I haven't test this yet, but I want to make sure you're OK with the approach. meow is an ES module and with Tact being CommonJS, there's a little bit of import magic in the tact entry point script. It's not too ugly, IMHO, but please take a look.

If you're OK with the approach, I'll give it some testing, work out the kinks and publish this PR.

@novusnota
Copy link
Member

novusnota commented Apr 27, 2024

@vitorpy if you rename bin/tact into bin/tact.mjs and add the bin: { "tact": "bin/tact.mjs" } property to package.json, then you'll be able to use ESM imports there without actually modifying the CLI name — it will stay "tact" as is.

And seems like you forgot to add the --config flag. Other then that, splendid work!

@vitorpy
Copy link
Contributor Author

vitorpy commented Apr 28, 2024

@novusnota Oh, thanks! I thought it wouldn't be OK to rename the tact bin. It looks so much cleaner now.

I kinda feel config_path should be an argument (being always required) but since a breaking change in the binary syntax doesn't make sense, I re-introduced the --config flag as a required flag.

@vitorpy vitorpy marked this pull request as ready for review April 28, 2024 06:03
@vitorpy
Copy link
Contributor Author

vitorpy commented Apr 28, 2024

Ah,

      - name: Compare Tact version from CLI flag `--version` against package.json
        if: runner.os != 'Windows'
        run: |
          if [ "$(./bin/tact.mjs --version)" != "$(jq -r '.version' < package.json)" ];
          then false
          fi

This is now a redundant step in the GitHub workflow, as the version from --version is extracted from the package.json. Should I remove it?

Copy link
Member

@anton-trunov anton-trunov left a comment

Choose a reason for hiding this comment

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

Really nice!

package.json Outdated Show resolved Hide resolved
bin/tact.mjs Outdated Show resolved Hide resolved
bin/tact.mjs Outdated Show resolved Hide resolved
bin/tact.mjs Outdated Show resolved Hide resolved
bin/tact.mjs Outdated Show resolved Hide resolved
bin/tact.mjs Outdated Show resolved Hide resolved
bin/tact.mjs Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
src/pipeline/build.ts Outdated Show resolved Hide resolved
.github/workflows/tact.yml Outdated Show resolved Hide resolved
@anton-trunov
Copy link
Member

We should also add some CLI tests to CI. At least one test per each CLI flag to demonstrate it it's there. You can put the data you need in this folder: https://github.com/tact-lang/tact/tree/main/test/tact-cli

@anton-trunov anton-trunov added enhancement kind: refactoring Improve code readability labels Apr 28, 2024
@anton-trunov
Copy link
Member

anton-trunov commented Apr 28, 2024

@vitorpy Lots of improvements in this PR! --check and --func are really a huge help when debugging Tact issues. There is one more thing that I'm personally missing: CLI should support compiling single contract projects without creating a tact.config.json file. Basically, I'd like to be able to write a test.tact file and compile it like so: tact test.tact (the default output folder could be . in this case -- this makes inspecting compilation results easier). Of course, tact test.tact --check should also work, etc.

vitorpy and others added 2 commits April 28, 2024 10:40
Co-authored-by: Anton Trunov <[email protected]>
Co-authored-by: Anton Trunov <[email protected]>
@anton-trunov
Copy link
Member

anton-trunov commented May 1, 2024

@novusnota Something went wrong with single-contract compilation, which works as expected.

Here is a shell session with tact.config.json:

❯ ls
tact.config.json  test.tact

tact/contract new-cli*​ ≡
❯ cat tact.config.json 
{
  "$schema": "http://raw.githubusercontent.com/tact-lang/tact/main/grammar/configSchema.json",
  "projects": [
    {
      "name": "test",
      "path": "./test.tact",
      "output": "./",
      "mode": "funcOnly"
    }
  ]
}

tact/contract new-cli*​ ≡
❯ cat test.tact       
contract Foo {
    get fun foo(): Int {
        return 42;
    }
}

tact/contract new-cli*​ ≡
❯ ../bin/tact --config tact.config.json
💼 Compiling project test...
   > Foo: tact compiler
✔️ FunC code generation succeeded.

tact/contract new-cli*​ ≡
❯ ls
tact.config.json  test_Foo.abi      test_Foo.headers.fc  test_Foo.storage.fc
test.tact         test_Foo.code.fc  test_Foo.stdlib.fc

And here is a shell session with single-contract compilation with the same contract and and bin/tact:

❯ rm -f *.abi *.boc *.fc *.fif *.pkg *.md *.ts

tact/contract new-cli*​ ≡
❯ ls
tact.config.json  test.tact

tact/contract new-cli*​ ≡
❯ ../bin/tact --func test.tact                
💼 Compiling project main...
   > 👀 Enabling debug
   > Foo: tact compiler
   > Foo: func compiler
   > Foo: fift decompiler
   > Packaging
   > Foo
   > Bindings
   > Foo
   > Reports
   > Foo

tact/contract new-cli*​ ≡
❯ ls
main_Foo.abi       main_Foo.code.fif      main_Foo.md         main_Foo.storage.fc  test.tact
main_Foo.code.boc  main_Foo.code.rev.fif  main_Foo.pkg        main_Foo.ts          
main_Foo.code.fc   main_Foo.headers.fc    main_Foo.stdlib.fc  tact.config.json

It does not stop after generating FunC files.

@novusnota
Copy link
Member

novusnota commented May 1, 2024

@anton-trunov hmm, can't reproduce that error, my --func emits only .fc + .abi code. Have you pulled the latest commit?

UPD: I may have found a bigger source of potential bugs — bin/tact depends on src/node.ts, not on dist/node.js. Fixing this now.
UPD: Nevermind, didn't read the sources right.

@anton-trunov
Copy link
Member

@novusnota Yes, I'm on commit 0b18c8c. I can reproduce it after yarn install && yarn clean && yarn gen && yarn build

@anton-trunov
Copy link
Member

UPD: I may have found a bigger source of potential bugs — bin/tact depends on src/node.ts, not on dist/node.js

To me this sounds backwards. Why do you need to reverse the dependency?

@novusnota
Copy link
Member

novusnota commented May 1, 2024

No changes there, just a "read issue" :)
Reproduced the bug, now locating why it happens, hold on a bit

UPD: Found out the reason (enum over disjoint values messed up the CLI schema a bit), now working on solution
UPD: Fixed the bug

novusnota added 2 commits May 1, 2024 16:31
Separated the two in a more distinct way to reduce possible errors
Copy link
Member

@anton-trunov anton-trunov left a comment

Choose a reason for hiding this comment

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

Just one thing that is left: it's a de facto standard that more local flags should have priority over less local ones. Here how it fails for bin/tact:

❯ cat tact.config.json     
{
  "$schema": "http://raw.githubusercontent.com/tact-lang/tact/main/grammar/configSchema.json",
  "projects": [
    {
      "name": "test",
      "path": "./test.tact",
      "output": "./",
      "mode": "full"
    }
  ]
}

tact/contract new-cli*​ ≡
❯ ../bin/tact --config tact.config.json --check
💼 Compiling project test...
   > Foo: tact compiler
   > Foo: func compiler
   > Foo: fift decompiler
   > Packaging
   > Foo
   > Bindings
   > Foo
   > Reports
   > Foo

It is expected that the CLI flag --check has priority over "mode": "full" in tact.config.json.

.github/workflows/tact.yml Outdated Show resolved Hide resolved
.github/workflows/tact.yml Outdated Show resolved Hide resolved
.github/workflows/tact.yml Outdated Show resolved Hide resolved
.github/workflows/tact.yml Outdated Show resolved Hide resolved
@novusnota
Copy link
Member

novusnota commented May 1, 2024

It is expected that the CLI flag --check has priority over "mode": "full" in tact.config.json.

Yup, will correct priorities

Copy link
Member

@anton-trunov anton-trunov left a comment

Choose a reason for hiding this comment

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

Great effort @vitorpy, @novusnota! Tact is much more interactive now

@anton-trunov anton-trunov merged commit bc086fb into tact-lang:main May 2, 2024
@anton-trunov anton-trunov mentioned this pull request May 2, 2024
@anton-trunov
Copy link
Member

btw, added a few more things to work on in terms of enhancing Tact's CLI into the tracking issue #136

would be pretty cool to have the --expr option to experiment with Tact interactively

@novusnota
Copy link
Member

novusnota commented May 2, 2024

btw, added a few more things to work on in terms of enhancing Tact's CLI into the tracking issue #136

Nice! AST ones would drive their API counterparts, which is a big aid for tooling

would be pretty cool to have the --expr option to experiment with Tact interactively

Not sure about this, as there's already Nujan and (upcoming once I sort out my doc-related and Sublime/Emacs-related debts) interactive tact play toolkit. Tact is a DSL, after all, so proper experiments would require some interaction with blockchain, at least on the Sandbox level

@anton-trunov
Copy link
Member

proper experiments would require some interaction with blockchain, at least on the Sandbox level

this is why it's for expressions only :)

Tact has some unusual semantics for expressions: for instance, the integer division operation rounds towards negative infinity. That flag makes it easier to experiment when you do your stuff using your terminal. And with something like fzf you have easily searchable interaction history available, even if you do not have a nice REPL ready yet.

@novusnota
Copy link
Member

novusnota commented May 2, 2024

Ok, I see the point now.

even if you do not have a nice REPL ready yet.

Well, on that note, we may just make a basic REPL for Tact expressions a part of the CLI — by using the readline module of Node: https://nodejs.org/api/readline.html

As an idea, the actual expression for the --expr/-e may be omitted, in which case the expression REPL will be spawned.

@anton-trunov
Copy link
Member

A dedicated repl in addition to a cli option would be indeed nice! The option allows for better scriptability

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind: refactoring Improve code readability scope: cli Tact's command-line interface (bin)
Projects
None yet
3 participants