Skip to content

Download and install via tarball instead of NPM#8

Open
webwarrior-ws wants to merge 1 commit intotarsgate:masterfrom
webwarrior-ws:install-tarball
Open

Download and install via tarball instead of NPM#8
webwarrior-ws wants to merge 1 commit intotarsgate:masterfrom
webwarrior-ws:install-tarball

Conversation

@webwarrior-ws
Copy link
Copy Markdown
Contributor

The NPM option is still available with the -n/--npm flag.

Fixes #2

Comment thread src/index.ts Outdated
Comment thread src/index.ts Outdated
Comment thread src/index.ts Outdated
@knocte knocte force-pushed the master branch 2 times, most recently from bc34195 to 488f42f Compare April 14, 2026 17:23
Comment thread src/index.ts Outdated
@knocte
Copy link
Copy Markdown
Contributor

knocte commented Apr 15, 2026

And let's rebase.

Comment thread src/index.ts Outdated
Comment thread ReadMe.md Outdated
Comment thread src/index.ts Outdated
@knocte
Copy link
Copy Markdown
Contributor

knocte commented Apr 15, 2026

Let's change one more thing here. In case the -e flag is used, and the -n flag is not used, let's launch a "which npm" process, and if that process fails (because probably npm is not installed), then install awto-pi-lot extension via git instead of npm.

@webwarrior-ws
Copy link
Copy Markdown
Contributor Author

Let's change one more thing here. In case the -e flag is used, and the -n flag is not used, let's launch a "which npm" process, and if that process fails (because probably npm is not installed), then install awto-pi-lot extension via git instead of npm.

When trying to test it, it occurred to me that without npm we can't start installation process.

@knocte
Copy link
Copy Markdown
Contributor

knocte commented Apr 15, 2026

it occurred to me that without npm we can't start installation process.

That's why we're coding this change! so that if someone prefers tarball installation then she doesn't use the -n flag. Did you read it backwards?

@webwarrior-ws
Copy link
Copy Markdown
Contributor Author

it occurred to me that without npm we can't start installation process.

That's why we're coding this change! so that if someone prefers tarball installation then she doesn't use the -n flag. Did you read it backwards?

Installation is done by running either npm run or npx. Tarball or not, installation script can't be started without npm.

@knocte
Copy link
Copy Markdown
Contributor

knocte commented Apr 15, 2026

Installation is done by running either npm run or npx

Oh, you mean the launch of skynot itself, right? Mmm good point

Comment thread src/index.ts Outdated
Comment thread ReadMe.md Outdated
Comment thread src/index.ts Outdated
@knocte
Copy link
Copy Markdown
Contributor

knocte commented Apr 15, 2026

Now that with this type of download we can know what's the progress on it, let's add a progress bar so that we know if it's stuck (this is very useful for me since I have bad connection these days).

@webwarrior-ws
Copy link
Copy Markdown
Contributor Author

Now that with this type of download we can know what's the progress on it, let's add a progress bar so that we know if it's stuck (this is very useful for me since I have bad connection these days).

If you run installation with verbose flag, wget output is redirected to stdout, so you'll see the progress.

Comment thread src/index.ts Outdated
@knocte
Copy link
Copy Markdown
Contributor

knocte commented Apr 16, 2026

wget output is redirected to stdout, so you'll see the progress.

Ah ok, acceptable. But then let's only use the verbose flag of wget when skynot is being used with the verbose flag.
And let's use longer flag names on wget!

@knocte
Copy link
Copy Markdown
Contributor

knocte commented Apr 16, 2026

And let's use longer flag names on wget!

And for tar!

@webwarrior-ws
Copy link
Copy Markdown
Contributor Author

wget output is redirected to stdout, so you'll see the progress.

Ah ok, acceptable. But then let's only use the verbose flag of wget when skynot is being used with the verbose flag. And let's use longer flag names on wget!

I don't use any flags on wget. It's the logic of runAsPi function that redirects output of the programs when verbose is true.

@webwarrior-ws
Copy link
Copy Markdown
Contributor Author

And let's use longer flag names on wget!

And for tar!

Changed to using long flag names in tar command.

@knocte
Copy link
Copy Markdown
Contributor

knocte commented Apr 16, 2026

It's the logic of runAsPi function that redirects output of the programs when verbose is true.

What do you mean man? Have you checked what runAsPi() does with the verbose flag? it's just being used for the sudo call; so it doesn't have any effect on wget

@webwarrior-ws
Copy link
Copy Markdown
Contributor Author

It's the logic of runAsPi function that redirects output of the programs when verbose is true.

What do you mean man? Have you checked what runAsPi() does with the verbose flag? it's just being used for the sudo call; so it doesn't have any effect on wget

if (verbose) {

@knocte
Copy link
Copy Markdown
Contributor

knocte commented Apr 16, 2026

Ok that actually tells me that the way you have done this is wrong. Sudo should only be used for the things that REQUIRE sudo, and downloading with wget does not. Please dismember that big command to only use sudo for the thing that really needs sudo: installing in $installDir.

The NPM option is still available with the `-n`/`--npm` flag.

Fixes tarsgate#2
@webwarrior-ws
Copy link
Copy Markdown
Contributor Author

Ok that actually tells me that the way you have done this is wrong. Sudo should only be used for the things that REQUIRE sudo, and downloading with wget does not. Please dismember that big command to only use sudo for the thing that really needs sudo: installing in $installDir.

Split command into 2: one for download, one for unpacking (run as pi user).

@knocte
Copy link
Copy Markdown
Contributor

knocte commented Apr 16, 2026

Why are you creating a new runCommand func?

@webwarrior-ws
Copy link
Copy Markdown
Contributor Author

Why are you creating a new runCommand func?

Because we don't have a function that runs a command? Only the on that runs with sudo.

@knocte
Copy link
Copy Markdown
Contributor

knocte commented Apr 16, 2026

Because we don't have a function that runs a command?

WRONG! execAsync() is there!!

@webwarrior-ws
Copy link
Copy Markdown
Contributor Author

Because we don't have a function that runs a command?

WRONG! execAsync() is there!!

With execAsync() progress bar is not shown.

@knocte
Copy link
Copy Markdown
Contributor

knocte commented Apr 16, 2026

Then we adjust execAsync, we DONT DUPLICATE IT

@webwarrior-ws
Copy link
Copy Markdown
Contributor Author

Then we adjust execAsync, we DONT DUPLICATE IT

execAsync is defined as

const execAsync = promisify(exec);

There is nothing to adjust.

@knocte
Copy link
Copy Markdown
Contributor

knocte commented Apr 16, 2026

Sigh man I don't understand why you make this so complicated, when it's very simple really.

There's already a way to call processes: exec
There's also a way to call processes with sudo, that receives a verbose flag.

It turns out you need a way to call processes but needing a verbose flag. Then there are 2 obvious options on how to do this here:

  1. Figure out how to adjust/wrap exec/execAsync to have a verbose flag like the sudo thing has, and then only use that across everywhere where there is no need for sudo (to not have 3 ways to call processes).
  2. Adjust the sudo wrapper to have sudo flag as an option: this way you're reusing the process wrapper without having to DUPLICATE CODE which is what you did!

@webwarrior-ws
Copy link
Copy Markdown
Contributor Author

  1. Can't adjust exec to redirect output while the command is running.
  2. runSudoWithPassword is already complicated. Adding the option to run command without sudo will make it even more complicated. And there's not that much overlap with the new function.

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.

Download and install via tarball instead of NPM

2 participants