Fix bug with bash autocompletion for the users who has . in their PATH environment variable

[thoth291]

Motivation

This should address issue of builtins to consider local files as executables during parsing of the command.
This will fix issues with PATH having "." and script being executable.

Test Plan

Need to figure out how to make this into a test here - need some help.

# Copyright (c) Facebook, Inc. and its affiliates. All Rights Reserved
import subprocess
#import textwrap

def test_bash_completion():
    process = subprocess.Popen(
#            textwrap.dedent(
"""
bash -l <<- 'HEREDOC'
# Navigate to app with minimal configuration
cd .

# Locate python
PYTHON=$(command -v python)

# Setup hydra bash completion
eval "$(python simple.py -sc install=bash)"

# Setup debugging
export HYDRA_COMP_DEBUG=1

# Do actual test
test=$(COMP_LINE='python simple.py' hydra_bash_completion | grep EXECUTABLE_FIRST | awk '{split($0,a,"=");b=substr(a[2],2,length(a[2])-2);print b}')

if [ $test == $PYTHON ]; then
    echo TRUE
else
    echo FALSE
fi
HEREDOC
"""
#           )
    ,
    shell=True,
    stdout=subprocess.PIPE,
    stderr=subprocess.PIPE,
    )
    stdout, stderr = process.communicate()
    assert stderr == b''
    assert stdout == b'TRUE\n'
    return

Related Issues and PRs

Look here: #858

[thoth291]

Hi, @omry . It passed all my tests in my setup - so this solution seems to be mature. You asked me to make a test - I shared it above (#868 (comment)) - but I don't know how to put it in your framework for tests - could you please put it together yourself?

[omry]

Can you create a test bash script that validates this behavior of the bash function you changed?
you can execute it from the test suite as a subprocess.

You can add your script under tests/scripts

[thoth291]

I don't know how to make it to see proper python within my script. Is there any environment I need to activate?

[omry]

I don't know how to make it to see proper python within my script. Is there any environment I need to activate?

Is this working locally with pytest and with nox?
see: https://hydra.cc/docs/next/development/testing

[thoth291]

Yes, @omry, I tested it with pytest and it passes (locally). I don't really know why it doesn't pass it here in the CI.

[omry]

The CI is running nox. you can run it locally. see the doc I send you.
Does it work when you use nox locally (you can tell it to run a specific session).

[omry]

Getting close.
You have lint failures, the the style_guide for details on how to test locally.

[omry]

Also, please add a news fragment.

[thoth291]

They named mypy wrong - it should have been called mypain...
The issue of original failing of the test was due to bash v.s. bash -l - the later do not inherit env variables properly on the test machine.
The rest of the commits were due to tests.

[omry]

Testing these kind of things is always painful.
sorry it had to be your first PR :)

I have a request which will hopefully not be hard to address.

[omry]

shell=True is insecure in general.
Can you switch to keeping the shell script in a simple file and calling with subprocess.check_output() ?

[thoth291]

This is not difficult, but against my habits. In the projects I worked so far we use pytest (and similar things for C/C++/Fortran) and we always make our tests as self contained as possible. So we keep all things in one place - to give ultimate visibility of what is really being tested at particular test.
I would actually suggest you to change some of your expect scripts to my format to make sure that everything in one place.
I have an anecdotal story when push from my colleague changed many files at ones and one of them was part of the test suite - but then at final merge another colleague changed how that test was executed (unintentionally) which caused test to pass for all cases (even where it should have been failed).
If that test would be in the script (like the one above) - this would not happen.

P.S. I have to admit - I never used mypy and nox before - and given what I've seen I will probably not use them any time soon - IMHO - until they will mature a bit more and will make things more automatic.

[omry]

Okay, I accept your reasoning. but do get rid of the shell=True somehow.
For all I care dump the script to a temp file and run that with bash.

As for mypy and nox:

1. nox is fantastic. What issues did you have with it?
2. mypy is industry standard.
   The CI is intentionally testing a lot of things. It does take time to handle all the requirements, as especially given that it's your first PR.

[omry]

As for the expect scripts: I am expecting to have a change to them every 10 years.

[thoth291]

This is your repo - you are the boss here - I'll split it into separate file and will make shell=False.
I said - it's easy - was just sharing some of my personal observations.
P.S. I just take testing easy - nox is like "heavy metal for relaxation" for me (don't take me wrong - I love heavy metal - but consider it to be relaxing only during Armageddons). This is indeed my first PR with mypy - I guess I don't belong to the same industry :-)).

[omry]

I agree with your reasoning to keep it in one place.
what I am suggesting is that your test can dump a temp file from the script and execute it with bash.

[omry]

I am being strict on testing because I hate breaking things unknowingly.
If something breaks, it would better be because I intentionally broke it and not because of poor test coverage.

[thoth291]

Creating files on the fly for the tests is OK if it's data files - but executable scripts - that's something which I try to avoid due to the way different systems handle temporary files and setting permissions - so I decided to split it all together.

[omry]

1. sure.
2. This is not an executable, just a text file that is being executed by bash.
3. This shows you the power of good CI. if it your tests pass in CI you know it works on Linux Mac and Windows.

[omry]

Not that it matters much here, but I would exit with non zero exit code if there is an error. This way you don't need to compare the output.