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

[#107] Remove dynamically-linked 'tool-sync' asset #144

Merged
merged 2 commits into from
Oct 21, 2022
Merged

[#107] Remove dynamically-linked 'tool-sync' asset #144

merged 2 commits into from
Oct 21, 2022

Conversation

jim4067
Copy link
Contributor

@jim4067 jim4067 commented Oct 16, 2022

Resolves #107

Additional tasks

  • Documentation for changes provided/changed
  • Tests added
  • Updated CHANGELOG.md

@chshersh chshersh added the hacktoberfest-accepted https://hacktoberfest.com/participation/ label Oct 16, 2022
@chshersh chshersh changed the title feat: remove dynamic-libraries [#107] Remove dynamically-linked 'tool-sync' asset Oct 19, 2022
Copy link
Owner

@chshersh chshersh left a comment

Choose a reason for hiding this comment

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

The changes look good! 👍🏻

But the CI is failing. Not sure why tbh 🤔
@jim4067 What error do you get if you run tool-sync locally by trying to download tool-sync itself?

@jim4067
Copy link
Contributor Author

jim4067 commented Oct 20, 2022

not sure whether it is related but, I am IO error that the config file doesn't exists.

Even after creating the file and adding the default config, the error still persists
Screenshot from 2022-10-20 16-24-38

@MitchellBerend
Copy link
Collaborator

It works with mine but I get a different error, maybe there is a typo in your file name?

mitchell@mitchell-workstation:~/rust/tool-sync$ cargo run -- --config tool.toml install tool-sync 
   Compiling tool-sync v0.2.0 (/home/mitchell/rust/tool-sync)
    Finished dev [unoptimized + debuginfo] target(s) in 4.09s
     Running `target/debug/tool --config tool.toml install tool-sync`
🔄  Fetching info about 1 tools (this may take a few seconds)...                                                                                                          None
❌  tool-sync 
Multiple name matches found for this asset:

	 * tool-x86_64-unknown-linux-musl.sha256
	 * tool-x86_64-unknown-linux-musl.tar.gz

Please add one of these to the config.
🔄  All done!                                                                                                                                                             📦  Estimated total download size: 0B
Nothing to sync or encountered multiple errors prefetching tools.

------------------------------------------------------------------------------------------------------------------------

mitchell@mitchell-workstation:~/rust/tool-sync$ cat tool.toml 
# This configuration is automatically generated by tool-sync 0.2.0
# https://github.com/chshersh/tool-sync
#######################################
#
# Installation directory for all the tools:
store_directory = "$HOME/rust/tool-sync/bin"
#proxy = "http://10.10.0.1:8000"
#
# tool-sync provides native support for some of the tools without the need to
# configure them. Uncomment all the tools you want to install with a single
# 'tool sync' command:
#
[bat]
[difftastic]
[exa]
[fd]
[github]
[ripgrep]
[tool-sync]
#
# You can configure the installation of any tool by specifying corresponding options:
#
#[github]  # Name of the tool (new or one of the hardcoded to override default settings)
#    owner     = "cli"  # GitHub repository owner
#    repo      = "cli"     # GitHub repository name
#    exe_name  = "gh"          # Executable name inside the asset
#
#     Uncomment to download a specific version or tag.
#     Without this tag latest will be used
#     tag       = "13.0.0"

#     Asset name to download on linux OSes
#    asset_name.linux = "linux_amd64.tar.gz"

#     Uncomment if you want to install on macOS as well
#     asset_name.macos = "apple-darwin"

#     Uncomment if you want to install on Windows as well
#     asset_name.windows = "x86_64-pc-windows-msvc"

@jim4067
Copy link
Contributor Author

jim4067 commented Oct 20, 2022

A question

  • doesn't tool-sync generate a .tool.toml that already contains the default configuration? Do I need to fill in this information like store directory?

@MitchellBerend
Copy link
Collaborator

You can generate a default by running tool default-config > ~/.tool.toml but this is a manual step.

@jim4067
Copy link
Contributor Author

jim4067 commented Oct 20, 2022

You can generate a default by running tool default-config > ~/.tool.toml but this is a manual step.

@MitchellBerend Maybe it can be a future feature request to create the file with default configuration. As I am on Linux, I think this will require implementation of #71 first

@jim4067
Copy link
Contributor Author

jim4067 commented Oct 20, 2022

@chshersh I tried installing it but the connection timed out (multiple times).

I then tried installing exa as a control and it worked, not sure what the error was
Screenshot from 2022-10-20 18-04-37

@MitchellBerend
Copy link
Collaborator

What distro are you running @jim4067?

@jim4067
Copy link
Contributor Author

jim4067 commented Oct 20, 2022

@MitchellBerend I'm running Pop OS

NAME="Pop!_OS"
VERSION="22.04 LTS"
VERSION_ID="22.04"
VERSION_CODENAME=jammy
UBUNTU_CODENAME=jammy

@MitchellBerend
Copy link
Collaborator

Huh, thats weird Im running ubuntu 22.04 lts. Can you follow that link in your browser or does that also time out?

Distributor ID: Ubuntu
Description: Ubuntu 22.04 LTS
Release: 22.04
Codename: jammy

src/sync/db.rs Outdated Show resolved Hide resolved
@jim4067
Copy link
Contributor Author

jim4067 commented Oct 20, 2022

Huh, thats weird Im running ubuntu 22.04 lts. Can you follow that link in your browser or does that also time out?

Distributor ID: Ubuntu Description: Ubuntu 22.04 LTS Release: 22.04 Codename: jammy

I think I was having connection problems.

Similar issue of multiple name matches

Co-authored-by: Dmitrii Kovanikov <[email protected]>
@chshersh
Copy link
Owner

The changes in this PR look great and CI finally passes! 🎉 Happy to merge the PR 🚢

As for generating the default configuration, this was discussed before. I don't think we need to do this but we still can improve UX. I created a separate issue to improve the error message when there's no configuration found:

@chshersh chshersh merged commit d67f3c4 into chshersh:main Oct 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI hacktoberfest-accepted https://hacktoberfest.com/participation/ test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove the dynamically linked 'tool-sync' asset
3 participants