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

pipe in shell extension fails silently #233

Closed
NeoTeo opened this issue Apr 6, 2020 · 25 comments
Closed

pipe in shell extension fails silently #233

NeoTeo opened this issue Apr 6, 2020 · 25 comments
Labels
bug Something isn't working documentation Documentation related to this repo (for devs) MacOS Related to the MacOS operating system RFNR Ready to be shipped in next release

Comments

@NeoTeo
Copy link

NeoTeo commented Apr 6, 2020

The output of this command
curl -s 'https://api.chucknorris.io/jokes/random' | jq '.value'
in a bash shell returns the value value of the json, as expected.

Putting the same in the cmd field of an espanso rule triggers the rule but replaces the trigger word with nothing.
eg.:

 - trigger: ":tst"
    replace: "{{output}}"
    vars:
       - name: output
          type: shell
          params:
                 cmd: "curl -s 'https://api.chucknorris.io/jokes/random' | jq '.value'"       
@federico-terzi
Copy link
Collaborator

Hey @NeoTeo,

I tried your exact match on my Linux machine and it works as expected:

ezgif-2-a6e2a36fbe09

The only thing I did was fixing the indentation a bit:

  - trigger: ":tst"
    replace: "{{output}}"
    vars:
     - name: output
       type: shell
       params:
          cmd: "curl -s 'https://api.chucknorris.io/jokes/random' | jq '.value'"   

Could you check espanso log and post the output here?

Cheers :)

@NeoTeo
Copy link
Author

NeoTeo commented Apr 7, 2020

Thanks for the quick reply.
The espanso log does not seem to have any mention of the issue:

20:47:58 [ INFO] espanso version 0.5.4
20:47:58 [ INFO] using config path: /Users/teo/Library/Preferences/espanso
20:47:58 [ INFO] using package path: /Users/teo/Library/Application Support/espanso/packages
20:47:58 [ INFO] starting daemon...
20:47:58 [ INFO] Status icon already initialized, skipping.
20:47:58 [ INFO] Initializing EspansoNotifyHelper in /Users/teo/Library/Application Support/espanso
20:47:58 [ INFO] EspansoNotifyHelper already initialized, skipping.
20:47:58 [ INFO] Binded to IPC unix socket: /Users/teo/Library/Application Support/espanso/espanso.sock
20:47:58 [ INFO] espanso is running!
08:24:19 [ INFO] SecureInput has been acquired by loginwindow, preventing espanso from working correctly. Full path: /System/Library/CoreServices/loginwindow.app/Contents/MacOS/loginwindow
08:24:23 [ INFO] SecureInput has been disabled.

https://asciinema.org/a/lucT4S2hzATnZQvZbhcMfQp4R

@federico-terzi
Copy link
Collaborator

All right, I'll test it on my macOS machine soon and let you know once I discover something.

Cheers :)

@NeoTeo
Copy link
Author

NeoTeo commented Apr 7, 2020

For what it's worth I've now tried it on two different Macs: one running Mojave 10.14.6 and another running Catalina 10.15.5. Both system Terminal app, tried both bash and fish shell (I'm assuming Espanso uses bash when it shells out)

@federico-terzi
Copy link
Collaborator

Hey,
I've tested it on my macOS machine and it works as expected, which makes things even more interesting. Could you please try to run the command in the sh shell? (which is the one that espanso uses). Maybe we can spot the problem in that way.

Cheers :)

@NeoTeo
Copy link
Author

NeoTeo commented Apr 8, 2020

Most odd. Running the command also works in 'sh'
I've made sure all other apps that might interfere are not running and that there are no system text replacements set up either. 🤔

@federico-terzi
Copy link
Collaborator

federico-terzi commented Apr 8, 2020

@NeoTeo I think I've got it! It's most probably due to Environmental variables.
It seems like espanso (which is managed by launchd on macOS) doesn't inherit the complete PATH variable, which means that neither thesh shell does. As a result, the commands that were available on a normal shell are not recognized, thus making espanso fail.

Luckily, the fix is very easy, just include the complete path for the commands (easy to find, just type which jq for example):

  - trigger: ":tst"
    replace: "{{output}}"
    vars:
      - name: output
        type: shell
        params:
          cmd: "/usr/bin/curl -s 'https://api.chucknorris.io/jokes/random' | /usr/local/bin/jq '.value'"

Make sure to verify the above paths with yours.

Let me know if that helps :)

@NeoTeo
Copy link
Author

NeoTeo commented Apr 8, 2020

That was it! Will the next version of espanso inherit the $PATH or do we just get used to adding the full path ourselves (in which case it should probably be mentioned in the docs)?

Thanks again for the fast turn-around, for the fix and for the excellent tool!

:) teo

@federico-terzi
Copy link
Collaborator

That was it! Will the next version of espanso inherit the $PATH or do we just get used to adding the full path ourselves (in which case it should probably be mentioned in the docs)?

Glad to hear that! I'll definitely try to make it include the PATH in the next version, as it's what most users would expect. If for any reason this turn out difficult/impossible, I'll mention it in the docs :)

Thanks again for the fast turn-around, for the fix and for the excellent tool!

You're welcome :) Thanks for reporting this problem.

I'll keep the issue open so that I can keep a track of it

@federico-terzi federico-terzi added bug Something isn't working MacOS Related to the MacOS operating system labels Apr 8, 2020
@federico-terzi federico-terzi added the documentation Documentation related to this repo (for devs) label May 1, 2020
@atika
Copy link

atika commented May 11, 2020

It's normal behavior, the process is executed in a limited environment. If you run the command on the terminal, your profile file .bashrc, .profile, .zshrc are loaded before and define the paths, aliases, etc. On macOS you can define global environment variables that work everywhere or like Systemd define the variables in the unit file.

Or other options, set absolute paths in the command as you do, or I think you can define this in the program before executing the command like nodeJS, but as there are more simple solutions...

It's true that without an error message, it could be hard to see where is the problem. But it's not really a bug.

Example with Espanso LaunchAgent on macOS

<?xml version="1.0" encoding="UTF-8"?>
<!DOCTYPE plist PUBLIC "-//Apple//DTD PLIST 1.0//EN" "http://www.apple.com/DTDs/PropertyList-1.0.dtd">
<plist version="1.0">
<dict>
	<key>EnvironmentVariables</key>
	<dict>
		<key>CONFIG</key>
		<string>/Users/<username>/Library/Preferences/espanso</string>
		<key>PATH</key>
		<string>/usr/bin:/bin:/usr/sbin:/sbin:/usr/local/bin</string>
	</dict>
	<key>Label</key>
	<string>com.federicoterzi.espanso</string>
	<key>ProgramArguments</key>
	<array>
		<string>/usr/local/bin/espanso</string>
		<string>daemon</string>
	</array>
	<key>RunAtLoad</key>
	<true/>
	<key>StandardErrorPath</key>
	<string>/tmp/espanso.err</string>
	<key>StandardOutPath</key>
	<string>/tmp/espanso.out</string>
</dict>
</plist>

And when the shell script is run you get:

SHELL=/bin/zsh
USER=<username>
SSH_AUTH_SOCK=/private/tmp/...
CONFIG=/Users/<username>/Library/Preferences/espanso
PATH=/usr/bin:/bin:/usr/sbin:/sbin:/usr/local/bin
GLOBAL_VAR=A_GLOBAL_VAR
PWD=/
XPC_FLAGS=0x0
XPC_SERVICE_NAME=0
SHLVL=1
HOME=/Users/<username>
LOGNAME=<username>
_=/usr/bin/env

I think you can just add /usr/local/binto the path as many user programs are installed here.

@federico-terzi
Copy link
Collaborator

It's normal behavior, the process is executed in a limited environment. If you run the command on the terminal, your profile file .bashrc, .profile, .zshrc are loaded before and define the paths, aliases, etc. On macOS you can define global environment variables that work everywhere or like Systemd define the variables in the unit file.

That's exactly what I thought, thanks for pointing that out.

Or other options, set absolute paths in the command as you do, or I think you can define this in the program before executing the command like nodeJS, but as there are more simple solutions...

It's true that without an error message, it could be hard to see where is the problem. But it's not really a bug.

In the last version, I added an error for this kind of situations in the log, so it should be much easier to diagnose 👍

Example with Espanso LaunchAgent on macOS

<?xml version="1.0" encoding="UTF-8"?>
<!DOCTYPE plist PUBLIC "-//Apple//DTD PLIST 1.0//EN" "http://www.apple.com/DTDs/PropertyList-1.0.dtd">
<plist version="1.0">
<dict>
	<key>EnvironmentVariables</key>
	<dict>
		<key>CONFIG</key>
		<string>/Users/<username>/Library/Preferences/espanso</string>
		<key>PATH</key>
		<string>/usr/bin:/bin:/usr/sbin:/sbin:/usr/local/bin</string>
	</dict>
	<key>Label</key>
	<string>com.federicoterzi.espanso</string>
	<key>ProgramArguments</key>
	<array>
		<string>/usr/local/bin/espanso</string>
		<string>daemon</string>
	</array>
	<key>RunAtLoad</key>
	<true/>
	<key>StandardErrorPath</key>
	<string>/tmp/espanso.err</string>
	<key>StandardOutPath</key>
	<string>/tmp/espanso.out</string>
</dict>
</plist>

And when the shell script is run you get:

SHELL=/bin/zsh
USER=<username>
SSH_AUTH_SOCK=/private/tmp/...
CONFIG=/Users/<username>/Library/Preferences/espanso
PATH=/usr/bin:/bin:/usr/sbin:/sbin:/usr/local/bin
GLOBAL_VAR=A_GLOBAL_VAR
PWD=/
XPC_FLAGS=0x0
XPC_SERVICE_NAME=0
SHLVL=1
HOME=/Users/<username>
LOGNAME=<username>
_=/usr/bin/env

I think you can just add /usr/local/binto the path as many user programs are installed here.

This is a good approach indeed. I'm wondering whether it could be a good idea to load those variables from the .profile, .bashrc, .zshrc before executing the script. I may be wrong but probably executing source .profile before the actual command should import all these paths and make it seamless to the normal terminal bash usage.

What do you think about this approach @atika ?

@atika
Copy link

atika commented May 11, 2020

This is a good approach indeed. I'm wondering whether it could be a good idea to load those variables from the .profile, .bashrc, .zshrc before executing the script. I may be wrong but probably executing source .profile before the actual command should import all these paths and make it seamless to the normal terminal bash usage.

I am often taken to write daemon unit for ubuntu or my raspberry and this is the regular approach to set environment variables. You set variables for a specific environment.
In nodeJS, it happens that I have to modify the environment variables before executing the command used in the script, it's the same case.

Obviously macOS does not include the path to /usr/local/bin by default and this is the directory where a lot of user programs are installed.
If the user wants this directory or custom one to be added by default globally, to avoid absolute paths, there are many possibilities for that on macOS (/etc/launchd.conf).
It's not the first time I encounter this case, where you think that environment variables are available anywhere. Ex: using cron or at command is a good example.
On my shell script, I often use the absolute path to be sure.
For example, if you know the app CodeRunner, in the settings there are fields to customize these settings and prepare the environment where the scripts are executed, the app does not load the user profiles file.
Another thing is the portability, maybe the user has not the same profile on different machines.

The shell profiles files like .bashrc and .zshrc are to prepare an environment to work in the terminal and I don't think they are intended to be loaded for any other purpose.
Mostly these files contains a lot of useless things for the script itself, aliases, paths to compilation binaries, function, etc. In particular, mine is big and load multiples files.

The simple approach for this to work for everyone is just to add this specific directory /usr/local/bin to the unit as I do in 10s or let the user do it by itself globally.

@federico-terzi
Copy link
Collaborator

Thanks for the in-depth explanation @atika . I agree with you, better to keep .bashrc and all the others outside of this scope.

My only concern about adding /usr/local/bin by default is that it may create an inconsistent behavior: a user may get 90% of its commands working, but others (the ones stored in other directories) won't work and it may be harder for average users to track down why.

For sure, all these strange behaviors must be accurately documented in the docs.

@atika
Copy link

atika commented May 11, 2020

Yes, I was trying to express my point of view...
The problem here is that for the user it's almost obvious that, as the program (jq) is installed (surely by brew) in a typical system directory (/usr/local/bin), the absolute path for the command is somewhat optional.
I would surely have made the same mistake. When I write a shell script, this depends on how the script is executed, if the script is intended to be run in a different environment than the terminal,
I usually write directly the name of the command if the command is in a folder like /bin, but I set the absolute path to my custom commands as I know that the PATH is not defined.

As an example, default paths on my ubuntu server:

# /etc/environment
PATH="/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin:/usr/games:/usr/local/games"
LC_ALL=en_GB.UTF-8
LANG=en_GB.UTF-8

By default, /usr/local/bin is here on linux.

And also in a new installation of macOS:

#cat /etc/paths
/usr/local/bin
/usr/bin
/bin
/usr/sbin
/sbin
# echo $PATH
/usr/local/bin:/usr/bin:/bin:/usr/sbin:/sbin

For me, I already have the /usr/local/bin directory in my environment variables. Or I have added it, or the missing directory by default is an old problem of macOS.
Almost all of my LaunchAgent I made, some few years ago, have this:

Capture d’écran 2020-05-11 à 21 53 09

If you want the user to be able to execute their own commands directly (custom script in other directories), you have to modify the program: adding an option, modifying the environment each time a script is executed, etc. Is the work worth it?

There is a lot of program like this in macOS, it's rare to have an option to define custom PATH to custom commands.

@federico-terzi
Copy link
Collaborator

These are indeed fair points. At this point, I think it's pretty safe to include the /usr/local/bin path by default as you suggested (even though I'll probably inject the ENV variables directly in Rust rather than modifying the LaunchAgent, which should be equivalent).

Then I'll make sure to update the docs to make the situation more clear.

Do you think this is a reasonable solution?

Thanks for your help :)

@atika
Copy link

atika commented May 12, 2020

The term inject is important here. For now, the shell script inherits env from espanso (LaunchAgent), who inherits from launchd.

 ┌─────────────┐  
 │   Launchd   │  
 └─────────────┘  
        │         
        ▼         
┌────────────────┐
│  LaunchAgent   │
│    espanso     │
└────────────────┘
        │         
        ▼         
┌────────────────┐
│   subprocess   │
│  shell script  │
└────────────────┘

It's true that by modifying the LaunchAgent and if the user set global environment variables like this:

launchctl setenv /usr/bin:/bin:/usr/sbin:/sbin:/usr/local/bin:/custom/folder

The changes will be overwritten.

Yes, If you really want to do this in Rust (I can't help you with this, this gave me a headache trying to understand the code 😁), you have 2 options:

  • Just verify and inject /usr/local/bin in PATH. And I think a lot of people will be happy with this.
  • Or add a config option to add more custom folders, but users can do this in LaunchAgent or with the launchctl command. Just remember than they can do this because a lot of message as seen is "it's works in the terminal, not in espanso".

The initial issue, and I encountered the same problem, was that there is no error message to help to find the error. The problem is resolved.

Just another thing, a proposal. Why not take the opportunity to add a CONFIG and DATA environment variable, like this we can use it in our scripts and shell command, in the spirit to sync config between machine.

@federico-terzi
Copy link
Collaborator

The term inject is important here. For now, the shell script inherits env from espanso (LaunchAgent), who inherits from launchd.

 ┌─────────────┐  
 │   Launchd   │  
 └─────────────┘  
        │         
        ▼         
┌────────────────┐
│  LaunchAgent   │
│    espanso     │
└────────────────┘
        │         
        ▼         
┌────────────────┐
│   subprocess   │
│  shell script  │
└────────────────┘

It's true that by modifying the LaunchAgent and if the user set global environment variables like this:

launchctl setenv /usr/bin:/bin:/usr/sbin:/sbin:/usr/local/bin:/custom/folder

The changes will be overwritten.

Yes, If you really want to do this in Rust (I can't help you with this, this gave me a headache trying to understand the code 😁), you have 2 options:

  • Just verify and inject /usr/local/bin in PATH. And I think a lot of people will be happy with this.
  • Or add a config option to add more custom folders, but users can do this in LaunchAgent or with the launchctl command. Just remember than they can do this because a lot of message as seen is "it's works in the terminal, not in espanso".

All you said is correct. Luckily, it's pretty simple to do in Rust when spawning a command (you can see this example).

What I would probably do is setting PATH=/usr/local/bin by default on macOS, and then let the user customize this choice with a config variable in the default.yml (something like shell_path).
How does it sound to you?

I'm wondering whether also on Linux we have the same problem. From what I can remember, a Systemd-spawned espanso was able to find the commands so the PATH should be all right there, but I may be wrong. What do you think?

The initial issue, and I encountered the same problem, was that there is no error message to help to find the error. The problem is resolved.

Indeed now it should be easier to track down.

Just another thing, a proposal. Why not take the opportunity to add a CONFIG and DATA environment variable, like this we can use it in our scripts and shell command, in the spirit to sync config between machine.

Could you explain this point a bit more? It might be a good idea to open a separate issue

Cheers :)

@Scrumplex
Copy link
Contributor

I'm wondering whether also on Linux we have the same problem. From what I can remember, a Systemd-spawned espanso was able to find the commands so the PATH should be all right there, but I may be wrong.

As per man 5 systemd.exec (Line 1170):

       $PATH
           Colon-separated list of directories to use when launching executables.  systemd uses a fixed value of "/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin" in the system manager. When compiled for
           systems with "unmerged /usr" (/bin is not a symlink to /usr/bin), ":/sbin:/bin" is appended. In case of the the user manager, a different path may be configured by the distribution. It is
           recommended to not rely on the order of entries, and have only one program with a given name in $PATH.

On Arch Linux the PATH for user services is PATH=/usr/local/bin:/usr/bin

@Scrumplex
Copy link
Contributor

Scrumplex commented May 12, 2020

I don't know how well this would be handled on macOS, but instead of a configuration option I would propose a default path for an environment file.

With systemd you could define something like

[Service]
...
EnvironmentFile=~/.config/espanso/env
...

which could then in turn contain configuration like $PATH.
I guess you could try to emulate this on macOS with env $(cat <espanso-conf-path>/env | xargs) espanso

Or you could just load the file from espanso when running scripts :D

@atika
Copy link

atika commented May 12, 2020

Yes, it's exactly the page I found but I did not manage to make it work.

You can't set the path only like this PATH=/usr/local/bin or all others command will not work... You have to get the environment variable PATH first, verify that /usr/local/bin is not here, and inject it before executing the command. And this could be the same thing for Linux.

Do you use docker and docker-compose? You have your example: docker-compose environment variables
shell_path let me think that I have to define the path to the shell.

# Example default.yml
environment:
  - CONFIG=/Users/<username>/Library/Preferences/espanso
  - ANOTHER_VARIABLE=1
  - PATH:
    - /another/custom/location
    - /my/custom/script/directory

Where PATH could be ADDITIONAL_PATHS.

It's exactly the same thing in Linux for environment variables. But I think like macOS the path /usr/local/bin is present by default. I don't know if this path was here by default in old releases of macOS, I think this is where the problem comes from.

Like I explained in the other issue when I tried to use the $CONFIG variable for shell script. If you work on a system to customize the environment variables why not add theses variables to simplify the config. And this will also work for scripts.

Example

- trigger: ":test"
    replace: "{{output}}"
    vars:
      - name: output
        type: shell
        params:
          cmd: "$CONFIG/scripts/command.sh"
          trim: true

@atika
Copy link

atika commented May 12, 2020

@Scrumplex I referred to the docker-compose file as an example, and effectively we can add environment variables directly in the YAML file or pointing to an external environment file.

@federico-terzi Indeed if you add a system like this, we could avoid using absolute paths even with our custom commands and also add global variables for all shell and scripts expanders. 👍

@federico-terzi
Copy link
Collaborator

Thank you @Scrumplex for your insight, that's exactly the answer I was looking for.

@atika all right, thank you. I now understand what you meant when talking about the $CONFIG variable. That could be a great solution indeed. I've opened an issue for that #277

@federico-terzi
Copy link
Collaborator

Hey,
In the end I settled with a simple approach on macOS:
When the user first registers espanso with espanso register, the command gets the user PATH (which is present as the command is invoked in the terminal) and adds it to the Plist file.
That way, the shell extension behavior is the same as the terminal one. Moreover, if the path changes, the user can simply call espanso unregister; espanso register to update it.

This change will be shipped in the 0.6.2 release. Do you see any problem with it?

@federico-terzi federico-terzi added the RFNR Ready to be shipped in next release label Jun 10, 2020
@atika
Copy link

atika commented Jun 10, 2020

Hi,
The only thing I see is that it will overwrite the possible changes made by the user (like me) and add a lot of useless paths (developer tools). But it's simpler.

<key>EnvironmentVariables</key>
<dict>
  <key>CONFIG</key>
  <string>/Users/<username>/Library/Preferences/espanso</string>
  <key>PATH</key>
  <string>/usr/bin:/bin:/usr/sbin:/sbin:/usr/local/bin</string>
</dict>

Why macOS? This is not only a macOS issue.

@federico-terzi
Copy link
Collaborator

The reason why I chose this approach is that it provides the same experience a user would get in the terminal without any configuration.

On the other hand, if you don't like this approach is pretty easy to customize, it's just a matter of editing the launch agent file as you previously did.

So that seems like a reasonable default: good experience out of the box and easy customizability if you don't like it.

Why macOS? This is not only a macOS issue.

It mostly is, because on Windows there is not this problem and on Linux Systemd automatically adds the /usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin PATH by default

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working documentation Documentation related to this repo (for devs) MacOS Related to the MacOS operating system RFNR Ready to be shipped in next release
Projects
None yet
Development

No branches or pull requests

4 participants