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

helix 0.6.0 (new formula) #93816

Closed
wants to merge 5 commits into from

Conversation

henryhchchc
Copy link
Contributor

  • Have you followed the guidelines for contributing?
  • Have you ensured that your commits follow the commit style guide?
  • Have you checked that there aren't other open pull requests for the same formula update/change?
  • Have you built your formula locally with brew install --build-from-source <formula>, where <formula> is the name of the formula you're submitting?
  • Is your test running fine brew test <formula>, where <formula> is the name of the formula you're submitting?
  • Does your build pass brew audit --strict <formula> (after doing brew install --build-from-source <formula>)? If this is a new formula, does it pass brew audit --new <formula>?

@BrewTestBot BrewTestBot added new formula PR adds a new formula to Homebrew/homebrew-core rust Rust use is a significant feature of the PR or issue labels Jan 26, 2022
Formula/helix.rb Outdated
end

test do
assert_match version.to_s, shell_output("#{bin}/hx --version")
Copy link
Member

Choose a reason for hiding this comment

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

We need a test that exercises the some of the functionality of the app. Version checks or usage checks (foo --version or foo --help) are not sufficient, as explained in the formula cookbook.

In most cases, a good test would involve running a simple test case: run #{bin}/foo input.txt.

  • Then you can check that the output is as expected (with assert_equal or assert_match on the output of shell_output)
  • You can also check that an output file was created, if that is expected: assert_predicate testpath/"output.txt", :exist?

Some advice for specific cases:

  • If the formula is a library, compile and run some simple code that links against it. It could be taken from upstream's documentation / source examples.
  • If the formula is for a GUI program, try to find some function that runs as command-line only, like a format conversion, reading or displaying a config file, etc.
  • If the software cannot function without credentials, a test could be to try to connect with invalid credentials (or without credentials) and confirm that it fails as expected.
  • Same if the software requires a virtual machine, docker instance, etc. to be running.

Copy link
Contributor Author

@henryhchchc henryhchchc Jan 26, 2022

Choose a reason for hiding this comment

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

The case for Helix is a little bit different. Actually the output of hx --help is

helix-term v0.6.0
Blaž Hrastnik <[email protected]>
A post-modern text editor.

USAGE:
    hx [FLAGS] [files]...

ARGS:
    <files>...    Sets the input file to use

FLAGS:
    -h, --help       Prints help information
    --tutor          Loads the tutorial
    -v               Increases logging verbosity each use for up to 3 times
                     (default file: /Users/<user>/.cache/helix/helix.log)
    -V, --version    Prints version information

Unlike vim or nevim, Helix has not yet provide a headless mode, which enables the test script to perform operations without blocking the execution. I think -h or -V are currently the only two options that can output something without blocking the execution.

Alternatively, do you think it a good idea if we run hx -v and check whether the log file exists?

Copy link
Member

Choose a reason for hiding this comment

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

If it's a program that runs in the terminal can't you just open it, wait a bit and send an exit command?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the suggestions. Looks like Helix does not provide a way for sending commands externally. I am not sure whether SIGINT is considered as one. I may need some references, is there any other formulas having a test script doing that?

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks but unfortunately, writing to stdin can not tell Helix to exit since it handles stdin its own way. Currently the way I do the testing is that:

  • On macOS, I try to write some content to stdin and check for the error message produced.
  • On Linux, I start the program, write something to stdin (the content will be in the editor buffer), wait for 5 seconds, make sure the process is still alive (otherwise kill will exit with 1), and kill it.

Comment on lines +4 to +6
url "https://github.com/helix-editor/helix.git",
tag: "v0.6.0",
revision: "ac1b7d8d0a608f47edfee2872d414e94fd26cc31"
Copy link
Member

Choose a reason for hiding this comment

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

Do we need a git clone? Can we use a tarball instead?

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 think so. There are 55 submodules need to be fetched for building the binary, which are not available in the tarball.

Formula/helix.rb Outdated
Comment on lines 15 to 16
mv bin/"hx", bin/"hx-bin"
(bin/"hx").write_env_script(bin/"hx-bin", HELIX_RUNTIME: prefix/"runtime")
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
mv bin/"hx", bin/"hx-bin"
(bin/"hx").write_env_script(bin/"hx-bin", HELIX_RUNTIME: prefix/"runtime")
bin.env_script_all_files libexec/"bin", HELIX_RUNTIME: prefix/"runtime"

Is there any reason for you to be able to run hx without HELIX_RUNTIME set?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the suggestion. I was not aware of the convenient function provided by Homebrew lol.

As for the reason for setting HELIX_RUNTIME , it is mentioned in https://github.com/helix-editor/helix#installation that

Helix also needs its runtime files so make sure to copy/symlink the runtime/ directory into the
config directory (for example ~/.config/helix/runtime on Linux/macOS, or %AppData%/helix/runtime on Windows).
This location can be overridden via the HELIX_RUNTIME environment variable.

We have to install the runtime directory to the cellar and set the environment variable since we can not install it to the users' home directory.

BTW, I moved the runtime directory to libexec.

Copy link
Member

Choose a reason for hiding this comment

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

Judging by this it's more appropriate in etc

Copy link
Member

Choose a reason for hiding this comment

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

We have to install the runtime directory to the cellar and set the environment variable since we can not install it to the users' home directory.

I'm aware, but initially you installed hx without this environment variable set, which is why I asked if you ever want to be able to run hx without this set.

Judging by this it's more appropriate in etc

Probably not, because we can't ship updates to files in etc

@matoous matoous mentioned this pull request Feb 2, 2022
6 tasks
Formula/helix.rb Outdated
Comment on lines 27 to 29
stdin.write "test"
sleep 5.seconds
system "kill", "-SIGTERM", wait_thr.pid.to_s
Copy link

Choose a reason for hiding this comment

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

I took a quick look at how the vim test works, perhaps we can do something similar if homebrew supports injecting keys:

(testpath/"commands.vim").write <<~EOS
:python3 import vim; vim.current.buffer[0] = 'hello python3'
:wq
EOS
system bin/"vim", "-T", "dumb", "-s", "commands.vim", "test.txt"
assert_equal "hello python3", File.read("test.txt").chomp
assert_match "+gettext", shell_output("#{bin}/vim --version")

i hello esc :wq test.txt

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 don't think this is injecting keys. Actually in the test script of vim and neovim, they are creating a command script (i.e., commands.vim) and feed it to the editor. Unfortunately I don't see such feature in Helix (based on the latest release).

we can do something similar if homebrew supports injecting keys

I agree, and looks like sending key sequences is the only way we can control Helix in the test script.

Copy link
Member

Choose a reason for hiding this comment

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

You can use Ruby's PTY module if you want to inject keys.

Copy link
Contributor Author

@henryhchchc henryhchchc Feb 3, 2022

Choose a reason for hiding this comment

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

Thanks for the advice. I tried the following test script but hx still hangs. When I replace bin/"hx" with "vim", it just works. Maybe I am not using the right key sequences to control Helix. I need some help (maybe from @archseer, who is a contributor of Helix).

P.S.: When I type the key sequences manually on my keyboard, Helix works as expected.

test do
  require "pty"
  require "io/console"

  test_file = testpath / "test.txt"

  r, w, pid = PTY.spawn bin/"hx", test_file
  r.winsize = [80, 43]
  sleep 1
  w.write "itest\e"
  sleep 1
  w.write ":wq\n"
  sleep 1
  Process.wait(pid)
  assert_equal "test", test_file.read.chomp
end

Copy link

Choose a reason for hiding this comment

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

@henryhchchc can you try enabling r.raw! right after spawning?

Copy link

Choose a reason for hiding this comment

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

@github-actions
Copy link
Contributor

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new formula PR adds a new formula to Homebrew/homebrew-core rust Rust use is a significant feature of the PR or issue stale No recent activity
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants