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

Feature/windows #44

Closed
wants to merge 9 commits into from
Closed

Feature/windows #44

wants to merge 9 commits into from

Conversation

abdheshnayak
Copy link

  • Added source in search to support multiple binary names on same repo.
  • Added ps1 to work with PowerShell.

@jpillora
Copy link
Owner

Thanks for the PR :) is there a particular reason we need to use BinSource? the shell script uses Program

@abdheshnayak
Copy link
Author

abdheshnayak commented Feb 16, 2024

The repository can be monolithic where multiple binaries can be distributed so, that's why I introduced source where users can provide the name of binary. And It will filter and download the exact binary.

For example:
github.com/kloudlite/kl

Where the repository is going to distribute two binaries kl and kli.

@abdheshnayak
Copy link
Author

Please review and if there is something wrong, you can mention i will update it so you can accept the pull request. In case of pr is not that useful or something else please do close the pr by providing your feedback.

@jpillora
Copy link
Owner

jpillora commented Feb 18, 2024

I would like to add windows support to installer, but I would also like it be high-quality, and work the same way as the bash script installer.

currently:

  • installing repo foobar gives you binary foobar
  • in this example, Program is "foobar"
  • you specify an alternative with param as=fb and then it renames it fb

(1) change would be:

  • by default, it should maintain this repo-name->program-name behaviour:
  • installing repo foobar gives you binary foobar.exe

I would consider multi-binary downloads as an extra feature - though it should work the same way as it does in bash

  • one way I can think of doing this is to provide a optional multi=true or all=true param (im open to param name ideas)
  • if the user add multi, it just searches the archive for all executables, and puts them into /usr/local/bin
  • for safety, it would have to prompt to overwrite
  • also, we should implement this in the bash script to reduce confusion
  • RE: this PR: I would suggest switch to doing (1)
  • if you want it to support multi, you can give it a shot though post your plan here first
  • either way, we don't need to BinSource field

(2) change would be to add a method to the Assets

  • (a Assets) FilterFor(filetype string) Assets
  • where "bash" returns darwin and linux assets
  • and "powershell" returns windows assets
  • this way you don't need the template function, you can just loop over the assets the template receives, like bash does

(3) change is optional, and it would be to support multiple archive types. id accept the PR if it only supported zip, but would be nice if we could do gz and tar.gz too (see https://github.com/kloudlite/bin-installer/blob/f055686c741e934ed6abc39de33645f4ffa3e933/handler/handler_execute.go#L154)

lastly, i am very short for time at the moment! so I may not respond quickly


$urlamd64 = "{{$urlamd64}}"
$urlarm64 = "{{$urlarm64}}"
$destinationPath = "$env:USERPROFILE\Documents\kl"
Copy link
Owner

Choose a reason for hiding this comment

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

just noticed, Documents\kl ?

is there a standard place to put EXEs in windows? in linux its one of the bins, most commonly /usr/local/bin

@abdheshnayak
Copy link
Author

Pull Request #46 embodies similar functionalities to this, yet it demonstrates an enhanced level of code quality. In light of this, and to maintain the integrity and optimization of the project's codebase, I propose the closure of this pull request. This decision is made with the consideration of ensuring that we adopt the most efficient and well-structured solutions.

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