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

Add installer #488

Closed
wants to merge 2 commits into from
Closed

Add installer #488

wants to merge 2 commits into from

Conversation

syumai
Copy link
Contributor

@syumai syumai commented Jun 9, 2019

Related to #471 .

What this does

  • Add command deno_install.
  • This enables installing executable deno script in a easy way.

Usage

# Install deno script
deno_install https://deno.land/std/http/file_server.ts
# Now, you can run installed command
file_server
  • For the details, please read the README.
  • If you want to try behavior of this command, run the command below.
deno -A \
https://raw.githubusercontent.com/syumai/deno_std/add-installer/installer/deno_install.ts \
https://raw.githubusercontent.com/syumai/deno_std/add-installer/installer/deno_install.ts

TODO

  • Remove wget.
    • This is necessary for parsing target script's shebang.
    • If deno's fetch supports redirect, this can be removed.
  • Add tests.

Why I needed wget?

  • deno's fetch can't get data from scripts using redirect server like deno.land. (e.g. https://deno.land/std/http/file_server.ts)
  • fetch returns blank data.
  • redirect destination script's body is needed to parse shebang.

@CLAassistant
Copy link

CLAassistant commented Jun 9, 2019

CLA assistant check
All committers have signed the CLA.

@syumai syumai mentioned this pull request Jun 9, 2019
@ry
Copy link
Member

ry commented Jun 9, 2019

Thanks @syumai

@kt3k
Copy link
Member

kt3k commented Jun 10, 2019

@syumai
There seem to be a few lint errors:

/home/vsts/work/1/s/installer/mod.ts
  109:1   warning  Missing return type on function                                                                 @typescript-eslint/explicit-function-return-type
  128:1   warning  Missing return type on function                                                                 @typescript-eslint/explicit-function-return-type
  157:29  error    Array type using 'Array<Permission>' is forbidden for simple types. Use 'Permission[]' instead  @typescript-eslint/array-type

/home/vsts/work/1/s/installer/shebang.ts
   3:9   error    Array type using 'Array<string>' is forbidden for simple types. Use 'string[]' instead  @typescript-eslint/array-type
   8:25  error    Array type using 'Array<string>' is forbidden for simple types. Use 'string[]' instead  @typescript-eslint/array-type
  11:39  warning  Missing return type on function

@syumai
Copy link
Contributor Author

syumai commented Jun 10, 2019

@kt3k
Thanks! I'll fix them.

### 2. Add `~/.deno/bin` to PATH

```
echo 'export PATH="$HOME/.deno/bin:$PATH"' >> ~/.bashrc # change this to your shell
Copy link
Member

Choose a reason for hiding this comment

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

This message should be displayed to user after step 1.

Also I think it makes sense to make it a param if message should be shown - if we add this script to deno as deno install then .deno/bin will already be in PATH.

How about Windows?

- This defines what permissions are needed.

```sh
#!/usr/bin/env deno --allow-read --allow-write --allow-env --allow-run
Copy link
Member

Choose a reason for hiding this comment

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

It's pretty cool, but... not all scripts have shebang line - then it deno_install should handle manually specified perm flags as described by @ry:

So now people can run this:

deno_install file_server https://deno.land/std/http/file_server.ts --allow-write --allow-net

This will create a new executable shell script called $PATH/file_server, which would have the following contents:

#!/bin/sh
deno run --allow-write --allow-net https://deno.land/std/http/file_server.ts $@

If we skip this feature in first version we can drop wget as well

if (!HOME && !DENO_DIR) {
throw new Error('$DENO_DIR and $HOME are not defined.');
}
if (DENO_DIR) {
Copy link
Member

Choose a reason for hiding this comment

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

Using DENO_DIR instead of default ~/.deno/bin will require to add $DENO_DIR/bin/ to path as well to run scripts from shell - maybe we can skip this step for now?

if (!modulePath.startsWith('http')) {
throw new Error('module path is not correct.');
}
const moduleName = path.basename(modulePath, '.ts');
Copy link
Member

Choose a reason for hiding this comment

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

What if installed module is JavaScript file?

];

writeFileSync(FILE_PATH, encoder.encode('#/bin/sh\n'));
writeFileSync(FILE_PATH, encoder.encode(commands.join(' ')));
Copy link
Member

Choose a reason for hiding this comment

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

Won't second call overwrite first one? Maybe use template string?

const tpl = `#/bin/sh
${commands.join(' ')}`;
writeFileSync(FILE_PATH, encoder.encode(tpl));

this.path = pathBase.slice(2);
this.args = [...parts];
} else {
this.path = '';
Copy link
Member

Choose a reason for hiding this comment

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

Will it work when path is empty?

@bartlomieju bartlomieju mentioned this pull request Jun 13, 2019
@syumai
Copy link
Contributor Author

syumai commented Jun 13, 2019

@bartlomieju
I'm sorry for being late fixing my PR.
I've confirmed this PR. thanks!
#489
Should I close this PR?

@bartlomieju
Copy link
Member

@syumai I wanted to get it up to speed but I can't push to your branch :) that's why I opened another one - we can merge them later

@syumai
Copy link
Contributor Author

syumai commented Jun 14, 2019

#489 completed implementing this. I close this PR.

@syumai syumai closed this Jun 14, 2019
@syumai syumai deleted the add-installer branch June 14, 2019 15:46
inverted-capital pushed a commit to dreamcatcher-tech/napps that referenced this pull request Oct 24, 2024
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.

5 participants