cdist may execute the wrong version if executed from a git repository #21

New Issue

ungleich-gitea · 2021-11-20T11:24:52Z

ungleich-gitea commented

2021-11-20 11:24:52 +00:00

The MR !942 removed the script bin/cdist and replaced it with scripts/cdist. As an effect of this, cdist will import the cdist module from the system if it exists. It was tried to bypass the system module if it doesn't exist, but it only happens if it doesn't exist in the system path:

# try to import cdist and if that fails,
# then add this file's parent dir to
# module search path and try again.
try:
    import cdist
except ModuleNotFoundError:
    cdist_dir = os.path.realpath(
        os.path.join(
            os.path.dirname(os.path.realpath(__file__)),
            os.pardir))
    sys.path.insert(0, cdist_dir)
    import cdist

Where before, the bootstrapping shell script executed the following to guarantee that the checked out worktree will be used:

# Wrapper for real script to allow execution from checkout
# [...]
libdir=$(cd "${dir}/../" && pwd -P)
export PYTHONPATH="${libdir}"

TL;DR the MR !942 removed the capability to execute cdist in-place of a checked out git repository if cdist is already installed system-wide. I think the old behaviour should be kept to keep cdist portable, even between versions. Because you want to execute the cdist version you checked out in the git repository (where you executed the executeable).

If yes, we need to add/expand the module detection to load from the correct path. I think cdist isn't the only project that have this problem ;-) Else, you need to make it clear for the user that he might executed an other cdist version than he thought.

It's related to the error I'm talked in the chat yesterday (12.11.2020).

ping @ander

The MR !942 removed the script `bin/cdist` and replaced it with `scripts/cdist`. As an effect of this, cdist will import the `cdist` module from the system if it exists. It was tried to bypass the system module if it doesn't exist, but it only happens if it doesn't exist in the system path: ```py # try to import cdist and if that fails, # then add this file's parent dir to # module search path and try again. try: import cdist except ModuleNotFoundError: cdist_dir = os.path.realpath( os.path.join( os.path.dirname(os.path.realpath(__file__)), os.pardir)) sys.path.insert(0, cdist_dir) import cdist ``` Where before, the bootstrapping shell script executed the following to guarantee that the checked out worktree will be used: ```sh # Wrapper for real script to allow execution from checkout # [...] libdir=$(cd "${dir}/../" && pwd -P) export PYTHONPATH="${libdir}" ``` **TL;DR** the MR !942 removed the capability to execute cdist in-place of a checked out git repository if cdist is already installed system-wide. I think the old behaviour should be kept to keep cdist portable, even between versions. Because you want to execute the cdist version you checked out in the git repository (where you executed the executeable). If yes, we need to add/expand the module detection to load from the correct path. I think cdist isn't the only project that have this problem ;-) Else, you need to make it clear for the user that he might executed an other cdist version than he thought. --- It's related to the error I'm talked in the chat yesterday (*12.11.2020*). ping @ander

ungleich-gitea closed this issue

2021-11-20 11:24:52 +00:00

ungleich-gitea commented

2021-11-20 15:26:12 +00:00

mentioned in commit d30cd5c2b2

mentioned in commit d30cd5c2b2640cbb9ea5f2adc8763ce77edc844b

ungleich-gitea commented

2021-11-20 15:26:13 +00:00

mentioned in commit 76aa00b12e

mentioned in commit 76aa00b12e9b675c797e997a06ec7597c53a85f3

ungleich-gitea commented

2021-11-20 15:26:15 +00:00

mentioned in merge request !957

ungleich-gitea commented

2021-11-20 15:26:16 +00:00

mentioned in commit 156d4470130454e86c6a71327056f5d511f6e882

ungleich-gitea commented

2021-11-20 15:26:17 +00:00

What's about to check if we need to insert this path instead of doing it every time? It's not too big to check if .git or Makefile exists, or pragmatically, check if the cdist/ directory (or cdist/__init__.py) exists ...

cdist may also be installed system-wide. Insert a python path of a system-wide cdist installation like /usr/bin/.. won't throw an error, but there's no reason to not do a check.

What's about to check if we need to insert this path instead of doing it every time? It's not too big to check if `.git` or `Makefile` exists, or pragmatically, check if the `cdist/` directory (or `cdist/__init__.py`) exists ... cdist may also be installed system-wide. Insert a python path of a system-wide cdist installation like `/usr/bin/..` won't throw an error, but there's no reason to not do a check.

ungleich-gitea commented

2021-11-20 15:26:19 +00:00

@ander What was your real intention with

try:
    import cdist
except ModuleNotFoundError:
    cdist_dir = os.path.realpath(
        os.path.join(
            os.path.dirname(os.path.realpath(__file__)),
            os.pardir))
    sys.path.insert(0, cdist_dir)
    import cdist

What if we just do what is in the expect body?

    cdist_dir = os.path.realpath(
        os.path.join(
            os.path.dirname(os.path.realpath(__file__)),
            os.pardir))
    sys.path.insert(0, cdist_dir)
    import cdist

@ander What was your real intention with ``` try: import cdist except ModuleNotFoundError: cdist_dir = os.path.realpath( os.path.join( os.path.dirname(os.path.realpath(__file__)), os.pardir)) sys.path.insert(0, cdist_dir) import cdist ``` What if we just do what is in the `expect` body? ``` cdist_dir = os.path.realpath( os.path.join( os.path.dirname(os.path.realpath(__file__)), os.pardir)) sys.path.insert(0, cdist_dir) import cdist ```

Sign in to join this conversation.