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

Host script not executed #46

Closed
retog opened this issue Dec 14, 2020 · 15 comments
Closed

Host script not executed #46

retog opened this issue Dec 14, 2020 · 15 comments

Comments

@retog
Copy link

retog commented Dec 14, 2020

I've configured the setup script to e the default binary of the project. The idea was, to allow easy installation with

npx npm-project-nane

However, this generates a launch script that start node without the main script file:

reto@RETO-LAPTOP:~$ cat .config/scuttle_shell_browser/config/firefox/scuttle_shell_browser.sh
#!/bin/bash
/usr/local/bin/node

Things are working when I run the setup script from the project folder with node setup/native-host.js.

Any help would be greatly appreciated. My project is here: https://github.com/retog/scuttle-shell-browser/

@asamuzaK
Copy link
Owner

Give paths relative to process.cwd(), which should be setup in your case.

@retog
Copy link
Author

retog commented Dec 15, 2020

I've changed the path with retog/scuttle-shell-browser@fc6a41f, the script still doesn't work when run via npx or when run after being installed globally. I think the script shouldn't use cwd. I think it would be good if the setup utility could deploy the script-file[s] to the same locations as the shell-script.

npx --ignore-existing scuttle-shell-browser
Created: /home/reto/.config/scuttle_shell_browser/config/firefox
Created: /home/reto/.config/scuttle_shell_browser/config/firefox/scuttle_shell_browser.sh
Created: /home/reto/.mozilla/native-messaging-hosts/scuttle_shell_browser.json
-----
The Scuttle Shell Browser host has been installed.
Install the Scuttle Shell Browser Firefox Add-on if you haven't already.
You can download the Add-On here: https://github.com/retog/scuttle-shell-browser/releases/tag/1.0.0
reto@RETO-LAPTOP:~$ cat  .config/scuttle_shell_browser/config/firefox/scuttle_shell_browser.sh
#!/bin/bash
/usr/local/bin/node

@retog
Copy link
Author

retog commented Dec 15, 2020

The approach described here https://stackoverflow.com/a/3133313/1455912 might work when the application is installed globally. It wouldn't by itself work with npx as the files are removed after use, so the main script file and its dependencies would have to be copied to a permanent location.

asamuzaK added a commit that referenced this issue Dec 15, 2020
@asamuzaK
Copy link
Owner

asamuzaK commented Dec 15, 2020

Released v4.8.2
For mainScriptFile, It will now accept either rel path from process.cwd() or abs path,
Revert your previous changes and please try again.

@retog
Copy link
Author

retog commented Dec 15, 2020

Hi @asamuzaK, unfortunately, this doesn't solve the problem as it still using cwd. A patch that is irrelevant when the script is installed somewhere in the path or run with npx. Using absolute path is not an option, they should be relative to the location of the script rather than relative to the current working directory.

@asamuzaK
Copy link
Owner

If you pass the absolute path of the main script file, it doesn't matter what process.cwd() is.

Added some more tests to verify.
Update setup.test.js · asamuzaK/webExtNativeMsg@88107bd

@retog
Copy link
Author

retog commented Dec 16, 2020

I don't know the absolute path if it is installed with npm i -g or run with npx. I would like to use paths relative to the location of the script.

@asamuzaK
Copy link
Owner

asamuzaK commented Dec 17, 2020

process.argv[1] contains script path.
Ref Process | Node.js v15.4.0 Documentation

So I think you can get the path of mainScriptFile something like this.

const getMainScriptPath = () => {
  const [, scriptPath] = process.argv;
  const mainScriptPath = path.resolve(scriptPath, relative/path/from/scriptPath);
  return mainScriptPath;
};

const runSetup = async () => {
  const setup = new Setup();
  const mainScriptFile = getMainScriptPath();
  setup.mainScriptFile = mainScriptFile;
  setup.run();
};

@retog
Copy link
Author

retog commented Dec 18, 2020

Thanks @asamuzaK for your suggestions.

Unfortunately when installing my project a sym-link to the script is created an process.argv[1] contains the path to the sym-link rather than it's target.

$ npm i -g scuttle-shell-browser
/home/reto/.local/bin/scuttle-shell-browser -> /home/reto/.local/lib/node_modules/scuttle-shell-browser/setup/native-host.js

@asamuzaK
Copy link
Owner

asamuzaK commented Dec 19, 2020

How about below?
Ref File system | Node.js v15.4.0 Documentation

const getMainScriptPath = () => {
  const [, binPath] = process.argv;
  const scriptPath = fs.realpathSync(binPath);
  const mainScriptPath = path.resolve(scriptPath, relative/path/from/scriptPath);
  return mainScriptPath;
};

const runSetup = async () => {
  const setup = new Setup();
  const mainScriptFile = getMainScriptPath();
  setup.mainScriptFile = mainScriptFile;
  setup.run();
};

@retog
Copy link
Author

retog commented Dec 19, 2020

Thanks, @asamuzaK, that helped!

It now works perfectly both when run from the source folder as well as when installed globally.

A remaining issue is a situation when it's run with npx and the script is in a temporary folder (such as C:\Users\me\AppData\Roaming\npm-cache\_npx\14848\node_modules\scuttle-shell-browser\host\host-script.js). A solution would be to copy the script to the same directory where the generated shell-script is stored. Well, because of the dependencies of the script it should copy the whole project folder.

@asamuzaK
Copy link
Owner

A solution would be to copy the script to the same directory where the generated shell-script is stored. Well, because of the dependencies of the script it should copy the whole project folder.

It's out of scope for this module and is a waste of disk space.
Removing the temporarily installed packages after finished is the default behavior of npx.
I think you should let users to install your package globally and then run setup.

@asamuzaK
Copy link
Owner

Can close?

@retog
Copy link
Author

retog commented Dec 23, 2020

Hi @asamuzaK, I understand that making the project run with npx persistent is outside the scope of this project.

I can do that in my project by wrapping around setup._createShellScript as I've done here: retog/scuttle-shell-browser@623c4c8

The only not so nice part is that I have to meddle around with a method starting with '_' rather than using an actually exposed function, but it works for me and this issue can be closed. Thanks for your support!

@asamuzaK
Copy link
Owner

asamuzaK commented Dec 25, 2020

FYI
I think you don't need to wrap anything.

(async () => {
  const setup = new Setup({
    hostDescription: "Exposes an ssb-client to the scuttle shell browser extension",
    browser: "firefox",
    hostName: "scuttle_shell_browser",
    webExtensionIds: ["scuttle-shell-browser@example.org"],
    overwriteConfig: true,
    callback: handlerAfterSetup,
  });
  const targetPath = path.resolve(setup.configPath, 'app');
  fs.ensureDir(targetPath);
  await fs.copy(getProjectRoot(), targetPath, { overwrite: true });
  setup.mainScriptFile = path.resolve(targetPath, './host/host-script.js');
  await setup.run();
})();

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

No branches or pull requests

2 participants