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

Use "internal/project" in commands #455

Merged
merged 8 commits into from
Dec 29, 2023

Conversation

adambabik
Copy link
Collaborator

@adambabik adambabik commented Dec 15, 2023

In #438, a new package for handling projects was introduced: internal/project. In this PR, internal/project is used in all TUI and non-TUI CLI commands.

This is ready to be reviewed. All tests should pass, I did substantial manual testing, but it likely needs more of that.

@adambabik adambabik force-pushed the adamb/internal-project-in-commands-2 branch from d372831 to e34d87b Compare December 19, 2023 19:51
@adambabik adambabik marked this pull request as ready for review December 20, 2023 06:30
@sourishkrout
Copy link
Member

Starting with some functional testing, @adambabik:

  • It's hard to tell whether or not the binary is stuck when using massive-project trees (edge-case), e.g. runme --project .. where the parent dir contains a lot of git repos. The visual feedback of ⠙ Initializing... is subtle and easily overlooked. I believe it's the ignorePattern traversal that takes extra time here until the files are being enumerated in place. The visual feedback does not need immediate solving, could track a follow-up ticket for it.
  • That being sad, I wonder if there's a way to speed-up the ingorePattern traversal? Using some heuristics? It appears "the old impl" finishes faster (might not be as accurate). I'm curious to hear if you have any ideas here.

I am going to continue explorative testing now ahead of an actual code review.

@sourishkrout
Copy link
Member

It's not immediately actionable; just dropping some notes here. Two things:

  1. The sort order is different.
  2. Printing the full path might be problematic with narrow terminals. Relative path here should be enough.
❯ runme
runme 2.0.5

> buf-token RELEASE.md
    export BUF_TOKEN=Your buf token (...)

  release-buf RELEASE.md
    make proto/publish (...)

  buf-token RELEASE-01HHMPPM7SQR4JRAAWTXHV82S4.md
    export BUF_TOKEN=Your buf token (...)

  release-buf RELEASE-01HHMPPM7SQR4JRAAWTXHV82S4.md
    make proto/publish (...)

  update-brew README.md
     brew update (...)


❯ ./runme
runme 2.0.6-4-g86b0079-86b0079

> upgrade-go-120 /Users/sourishkrout/Projects/stateful/oss/runme/CONTRIBUTING.md
    make install/dev

  update-brew /Users/sourishkrout/Projects/stateful/oss/runme/README.md
     brew update (...)

  install-runme /Users/sourishkrout/Projects/stateful/oss/runme/README.md
     brew install runme (...)

  install-npm /Users/sourishkrout/Projects/stateful/oss/runme/README.md
     npm install -g runme (...)

  install-via-go /Users/sourishkrout/Projects/stateful/oss/runme/README.md
     go install github.com/stateful/runme@latest (...)


1/42  Choose ↑↓←→  Run [Enter]  Expand [Space]  Show Unnamed [u]  Quit [q]

@adambabik
Copy link
Collaborator Author

@sourishkrout thanks for the tests!

Regarding the problem of stuck ⠙ Initializing... and the fact of slow recursive traversal of .gitignore files, there is one line responsible for it: patterns, readErr := gitignore.ReadPatterns(p.fs, nil). It's the same implementation as in pkg/project. It turns out that this line is not executed when running runme --project .. with the latest version of runme and it is executed with the changes from this PR. I will check how it should be.

In terms of speeding this up, there are two things: (1) the current logic walks the project dir recursively twice; once for gitignore.ReadPatterns and the second time by the main logic, (2) the logic seems to be wrong, because it collects all patterns from all dirs and uses them against the currently processed dir. It should only take into consideration .gitignore files from the current path when doing DFS.

Regarding the next comments, I will verify (1) and will fix (2). One more thing I noticed is that when running runme --project .. I have a different number of tasks found by the latest runme version vs these changes. It might be related to the difference described in the first paragraph of this comment.

@sourishkrout
Copy link
Member

@adambabik sounds like a plan! Let's tackle 1. and 2., and then see if runme --project .. before versus after still show differences 👍.

Another cosmetic change, if you have a minute, is the absolute vs relative paths inside the TUI below. I think with these things done, we should be pretty close to a merge.

❯ ./runme
runme 2.0.6-4-g86b0079-86b0079

> upgrade-go-120 /Users/sourishkrout/Projects/stateful/oss/runme/CONTRIBUTING.md
    make install/dev

  update-brew /Users/sourishkrout/Projects/stateful/oss/runme/README.md
     brew update (...)

  install-runme /Users/sourishkrout/Projects/stateful/oss/runme/README.md
     brew install runme (...)

  install-npm /Users/sourishkrout/Projects/stateful/oss/runme/README.md
     npm install -g runme (...)

  install-via-go /Users/sourishkrout/Projects/stateful/oss/runme/README.md
     go install github.com/stateful/runme@latest (...)


1/42  Choose ↑↓←→  Run [Enter]  Expand [Space]  Show Unnamed [u]  Quit [q]

@adambabik
Copy link
Collaborator Author

  1. The sort order is different.

This is fixed. runme output is now consistent with the order of the dir walk which is alphabetical and depth-first.

2. Printing the full path might be problematic with narrow terminals. Relative path here should be enough.

This is fixed too. It shows a relative path to the dir from which runme was executed. For example, running it with --project=.. will print paths starting with ../ which allows actions like opening them by click from the VS Code terminal. Also, no more absolute paths as indicated in #455 (comment).


With regard to #455 (comment), I was not right 100%. The logic is correct as gitignore.ReadPatterns() correctly prefixes all found .gitignore files with "domain" which indicates the full path, so later uses are correct.

The optimization I talked about to avoid walking the directory twice won't speed things much because the second pass is very quick, likely due to pruning thanks to ignoring dirs.

In terms of its speed, I compared runme --project=.. ls with tree. gitignore.ReadPatterns() takes around 95% of the whole execution time and the whole execution is 2x slower than tree (it does less work, of course). 90% of the execution time is spent in the kernel.

It's also a fact that in the latest release of runme the gitignore files are not collected at all (gitignore.ReadPatterns() is not called) when running with --project=... I think it's a bug. If we want to be correct, calling runme on a large directory will be as slow as it is here. To avoid reading .gitignores, it's possible to use --git-ignore=false and it will result in the experience as in the latest runme.

Copy link
Member

@sourishkrout sourishkrout left a comment

Choose a reason for hiding this comment

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

Code LGTM 👍

@sourishkrout
Copy link
Member

@adambabik aye on all of the above's comments counts. If it's more accurate, I think that's totally fair. Let's ship it 🚀.

Copy link

sonarcloud bot commented Dec 29, 2023

Quality Gate Passed Quality Gate passed

The SonarCloud Quality Gate passed, but some issues were introduced.

12 New issues
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@adambabik adambabik force-pushed the adamb/internal-project-in-commands-2 branch from 9ec1c13 to 779d901 Compare December 29, 2023 18:09
@adambabik adambabik force-pushed the adamb/internal-project-in-commands-2 branch from 779d901 to 44f7d1b Compare December 29, 2023 18:13
@adambabik adambabik merged commit 3e2de68 into main Dec 29, 2023
4 checks passed
@adambabik adambabik deleted the adamb/internal-project-in-commands-2 branch December 29, 2023 19:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants