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

tools.vpm: improve prints for install command #19760

Merged
merged 2 commits into from
Nov 4, 2023

Conversation

ttytm
Copy link
Member

@ttytm ttytm commented Nov 3, 2023

  • Improves the feedback messages that are printed during installation.

    E.g. currently all this is printed in non-verbose mode, when installing a module.

    ❯ v install https://github.com/vlang/markdow
    Installing module "markdown" from "https://github.com/vlang/markdown" to "/home/t/.vmodules/vlang/markdown" ...
    Relocating module from "vlang/markdown" to "markdown" ("/home/t/.vmodules/markdown") ...
    Warning module "/home/t/.vmodules/markdown" already exists!
    Removing module "/home/t/.vmodules/markdown" ...
    Module "markdown" relocated to "markdown" successfully.
    

    But when using --once, the already installed info would only be printed with the verbose flag added. Without it, the install command would immediately finish, without printing any info.

    ❯ v install -v --once https://github.com/vlang/markdown.git
    Already installed modules: ['markdown']
    All modules are already installed.
    ╭─ ~/Dev/vlang/v
    ╰─❯ ▌
    
    # Current no verbose (no feedback)
    ❯ v install --once https://github.com/vlang/markdown.git
    ╭─ ~/Dev/vlang/v
    ╰─❯ ▌
    
    
    # PR no verbose
    ❯ v install --once https://github.com/vlang/markdown.git
    All modules are already installed.
    ╭─ ~/Dev/vlang/v
    ╰─❯ ▌
    

    With the PR the the All modules are already installed. info is printed also in non-verbose mode. I think giving the short feedback to the users is useful also here to confirm that their request was processed.

  • Removes some info from verbose mode that is imho only relevant for debugging and not for the user. E.g.

    ❯ v install --once -v https://github.com/vlang/markdown.git
    - cli params: ['install', 'https://github.com/vlang/markdown.git']
    -      command: git --version
    -      command: git clone --depth=1 --recursive --shallow-submodules "https://github.com/vlang/markdown.git" "/home/t/.vmodules/vlang/markdown.git"
    ...

@ttytm ttytm force-pushed the vpm/no-dbg-in-berbose-prints branch from cd4e2a9 to a1b50b4 Compare November 3, 2023 22:21
@@ -57,14 +57,13 @@ fn main() {
// args are: vpm [options] SUBCOMMAND module names
params := cmdline.only_non_options(os.args[1..])
options := cmdline.only_options(os.args[1..])
verbose_println('cli params: ${params}')
Copy link
Member

@spytheman spytheman Nov 4, 2023

Choose a reason for hiding this comment

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

That is useful for debugging problems when the main v executable invokes the vpm tool.
I would prefer if it stays, but you can wrap it in something like if os.getenv('VPM_DEBUG') != '' { if you like.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, definitely useful for debugging as mentioned. I left it as a comment where this is more apparent. The idea with the debug env var is great. When the simple version-installs are finished I'll add it. We could use the log package for it and set the log level to debug when the env var is passed. Would that fit here?

@spytheman spytheman merged commit 2ef49b8 into vlang:master Nov 4, 2023
42 checks passed
@ttytm ttytm deleted the vpm/no-dbg-in-berbose-prints branch November 13, 2023 14:45
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