Use entry_points for console scripts

[ubaumann]

Based on #820, I continued the work to remove the python files in the /bin folder and use entry_points for the installation

Moving the scripts brings a couple of advantages and makes installing it on different platforms more accessible. Also, the scripts are now in the library and can be imported from other python scripts. I could also fix all the tests.

• Move bin scripts into the package
• Use pathlib to make regex easier
• Unify some tests

[ubaumann]

@mbj4668 or @fredgan, What do you think about these changes? Anything I should add or change?

[mbj4668]

Why did you move the scripts from bin and removed bin from $PATH ? I usually have the repo checked out and just source env.sh, as described under "Run locally without installing" in README.md.

[ubaumann]

Using entry_points.console_scripts has some benefits.

• it works the same on Windows, Linux, Mac
• The source code is included in the package, and other python tools can include it and run the script. That is, for example, nice if someone wants to write a wrapper around pyang.
• The package can be installed in editable mode with pip install -e .

With entry points, the behaviour is the same as other python packages. as soon the virtual env is activated, the commands are in the $PATH available.

[mbj4668]

so what are the steps to run pyang locally?

[ubaumann]

• If you have it installed:

  pyang

• if you work with the source code:

  pip install -e .
  pyang

You could also do python pyang/scripts/pyang_tool.py or python -m pyang.scripts.pyang_tool should also work

So if someone wants to work on the repo I would recommend:

git clone .... pyang
python -m venv .venv
source .venv/bin/activate
cd pyang
pip install -e .
pyang

[mbj4668]

if you work with the source code: pip install -e .

This doesn't work for me. Where is the script pyang actually installed after this command?

[ubaumann]

The script itself is still at the same place in the script directory but if you are using venv in the bin folder will be a starter script. Setuptools will take care of the creation. So this also works for Windows for example. More information can be found here: https://setuptools.pypa.io/en/latest/userguide/quickstart.html#entry-points-and-automatic-script-creation

Example with a fresh docker container:

urs@DESKTOP-BM35OBL:~$ docker run --rm -it python:3.10 bash
root@2f16e97bfe85:/# git clone https://github.com/ubaumann/pyang.git
Cloning into 'pyang'...
remote: Enumerating objects: 12711, done.
remote: Counting objects: 100% (601/601), done.
remote: Compressing objects: 100% (295/295), done.
remote: Total 12711 (delta 336), reused 513 (delta 280), pack-reused 12110
Receiving objects: 100% (12711/12711), 8.89 MiB | 4.02 MiB/s, done.
Resolving deltas: 100% (8625/8625), done.
root@2f16e97bfe85:/# cd pyang/
root@2f16e97bfe85:/pyang# git checkout windows
Branch 'windows' set up to track remote branch 'windows' from 'origin'.
Switched to a new branch 'windows'
root@2f16e97bfe85:/pyang#
root@2f16e97bfe85:/pyang# python -m venv .venv
root@2f16e97bfe85:/pyang# source .venv/bin/activate
(.venv) root@2f16e97bfe85:/pyang#
(.venv) root@2f16e97bfe85:/pyang# ls .venv/bin/
Activate.ps1  activate  activate.csh  activate.fish  pip  pip3  pip3.10  python  python3  python3.10
(.venv) root@2f16e97bfe85:/pyang#
(.venv) root@2f16e97bfe85:/pyang#
(.venv) root@2f16e97bfe85:/pyang# pip install -e .
Obtaining file:///pyang
  Preparing metadata (setup.py) ... done
Collecting lxml
  Downloading lxml-4.9.1-cp310-cp310-manylinux_2_17_x86_64.manylinux2014_x86_64.manylinux_2_24_x86_64.whl (6.9 MB)
     ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ 6.9/6.9 MB 2.4 MB/s eta 0:00:00
Installing collected packages: lxml, pyang
  Running setup.py develop for pyang
Successfully installed lxml-4.9.1 pyang-2.5.3

[notice] A new release of pip available: 22.2.2 -> 22.3.1
[notice] To update, run: pip install --upgrade pip
(.venv) root@2f16e97bfe85:/pyang#
(.venv) root@2f16e97bfe85:/pyang#
(.venv) root@2f16e97bfe85:/pyang#
(.venv) root@2f16e97bfe85:/pyang# ls .venv/bin/
Activate.ps1  activate.csh   json2xml  pip3     pyang   python3     yang2dsdl
activate      activate.fish  pip       pip3.10  python  python3.10  yang2html
(.venv) root@2f16e97bfe85:/pyang#
(.venv) root@2f16e97bfe85:/pyang#
(.venv) root@2f16e97bfe85:/pyang# pyang --help | head
Usage: pyang [options] [<filename>...]

Validates the YANG module in <filename> (or stdin), and all its dependencies.

Options:
  -h, --help            Show this help message and exit
  -v, --version         Show version number and exit
  -V, --verbose
  -e, --list-errors     Print a listing of all error and warning codes and
                        exit.
(.venv) root@2f16e97bfe85:/pyang#
(.venv) root@2f16e97bfe85:/pyang#
(.venv) root@2f16e97bfe85:/pyang# cat .venv/bin/pyang
#!/pyang/.venv/bin/python
# EASY-INSTALL-ENTRY-SCRIPT: 'pyang','console_scripts','pyang'
import re
import sys

# for compatibility with easy_install; see #2198
__requires__ = 'pyang'

try:
    from importlib.metadata import distribution
except ImportError:
    try:
        from importlib_metadata import distribution
    except ImportError:
        from pkg_resources import load_entry_point

def importlib_load_entry_point(spec, group, name):
    dist_name, _, _ = spec.partition('==')
    matches = (
        entry_point
        for entry_point in distribution(dist_name).entry_points
        if entry_point.group == group and entry_point.name == name
    )
    return next(matches).load()

globals().setdefault('load_entry_point', importlib_load_entry_point)

if __name__ == '__main__':
    sys.argv[0] = re.sub(r'(-script\.pyw?|\.exe)?$', '', sys.argv[0])
    sys.exit(load_entry_point('pyang', 'console_scripts', 'pyang')())
(.venv) root@2f16e97bfe85:/pyang#
(.venv) root@2f16e97bfe85:/pyang#

[mbj4668]

Right, that was your third option above, using venv. I got that to work. But I was thinking about the second otpion with just pip install -e .

[ubaumann]

If you run it as a normal user I assume it will be in $HOME/.local/bin but I assume it depends on your operation system

ins@Lab:~$ whereis pip
pip: /usr/bin/pip /usr/share/man/man1/pip.1.gz
ins@Lab:~$ git clone https://github.com/ubaumann/pyang.git
Cloning into 'pyang'...
remote: Enumerating objects: 12711, done.
remote: Counting objects: 100% (601/601), done.
remote: Compressing objects: 100% (295/295), done.
remote: Total 12711 (delta 336), reused 511 (delta 280), pack-reused 12110
Receiving objects: 100% (12711/12711), 8.89 MiB | 24.88 MiB/s, done.
Resolving deltas: 100% (8625/8625), done.
ins@Lab:~$ cd pyang/
ins@Lab:~/pyang$ git checkout windows
Branch 'windows' set up to track remote branch 'windows' from 'origin'.
Switched to a new branch 'windows'
ins@Lab:~/pyang$ pip install -e .
Defaulting to user installation because normal site-packages is not writeable
Obtaining file:///home/ins/pyang
  Preparing metadata (setup.py) ... done
Collecting lxml
  Downloading lxml-4.9.1-cp310-cp310-manylinux_2_17_x86_64.manylinux2014_x86_64.manylinux_2_24_x86_64.whl (6.9 MB)
     ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ 6.9/6.9 MB 45.2 MB/s eta 0:00:00
Installing collected packages: lxml, pyang
  Running setup.py develop for pyang
Successfully installed lxml-4.9.1 pyang-2.5.3
ins@Lab:~/pyang$ pyang
pyang
ins@Lab:~/pyang$ whereis pyang
pyang: /home/ins/.local/bin/pyang

[ubaumann]

If pip installs the script in a location not containing in $PATH, it should get a warning printed: https://pip.pypa.io/en/stable/cli/pip_install/#cmdoption-no-warn-script-location

[mbj4668]

Ok, thanks. Another question, why is the script file called pyang_tool.py instead of just pyang?

[ubaumann]

I renamed pyang.py to pyang_tool.py because of import issues in different python versions. The module is named pyang, and if the file is also named pyang, I could not find a way to import the right pyang in all the supported python versions.

[mbj4668]

Ok. Could you rebase you branch on master and update README.md with new install instructions?

[ubaumann]

I updated the README and added aliases to the env.sh file for people not wanting to install pyang. The master branch is already up to date in the PR

[mbj4668]

Thanks! Github says that the branch cannot be rebased due to conflicts. I haven't checked what the conflicts are yet...

[ubaumann]

I don't see any conflicts

[wlupton]

Have you tried to rebase your branch against the latest mbj4668:master? I tried it locally and hit this conflict in pyang/syntax.py:

# Not part of YANG syntax per se but useful for pyang in several places
re_filename = re.compile(
<<<<<<< HEAD
    r"^(?:.*" + re.escape(os.sep) + r")?" +    # ignore all before os.sep
    r"([^@]*?)" +                              # putative module name
    r"(?:@([^.]*?))?" +                        # putative revision
    r"(?:\.yang|\.yin)*" +                     # foo@bar.yang.yin.yang.yin ?
    r"\.(yang|yin)$")                          # actual final extension
=======
    r"^(?:.*[/\\])?" +              # ignore all before path separator (either / or \)
    r"([^@]*?)" +                   # putative module name
    r"(?:@([^.]*?))?" +             # putative revision
    r"(?:\.yang|\.yin)*" +          # foo@bar.yang.yin.yang.yin ?
    r"\.(yang|yin)$")               # actual final extension
>>>>>>> 23c51f1 (Fix invalid regular expression on Windows)

Having resolved this conflict (taking the 23c51f1 changes) the rebase completed successfully.

[wlupton]

Note: I didn't mean to imply that the right way to resolve the conflict is necessarily just to take the 23c51f1 changes.

It appears that ubaumann:master (from which you presumably branched ubaumann:windows) is three commits behind mbj4668:master. One of these commits (17aa7ad) made these changes to pyang/syntax.py (which might mean that you don't need your changes?):

% git show 17aa7ad
commit 17aa7ad77b00c502afda533fcf7dedd1a9f91930
Author: Fred Gan <ganshaolong@vip.qq.com>
Date:   Sat Nov 5 15:29:18 2022 +0800

    bugfix: os.sep should be escaped because it is "\" in Windows platform

diff --git a/pyang/syntax.py b/pyang/syntax.py
index 41c54a2..7819d2f 100644
--- a/pyang/syntax.py
+++ b/pyang/syntax.py
@@ -131,11 +131,11 @@ re_deviate = re.compile(r"^(add|delete|replace|not-supported)$")

 # Not part of YANG syntax per se but useful for pyang in several places
 re_filename = re.compile(
-    r"^(?:.*" + os.sep + r")?" +    # ignore all before os.sep
-    r"([^@]*?)" +                   # putative module name
-    r"(?:@([^.]*?))?" +             # putative revision
-    r"(?:\.yang|\.yin)*" +          # foo@bar.yang.yin.yang.yin ?
-    r"\.(yang|yin)$")               # actual final extension
+    r"^(?:.*" + re.escape(os.sep) + r")?" +    # ignore all before os.sep
+    r"([^@]*?)" +                              # putative module name
+    r"(?:@([^.]*?))?" +                        # putative revision
+    r"(?:\.yang|\.yin)*" +                     # foo@bar.yang.yin.yang.yin ?
+    r"\.(yang|yin)$")                          # actual final extension

 arg_type_map = {
     "identifier": lambda s: re_identifier.search(s) is not None,

[wlupton]

(I'm not sure why GitHub says This branch has no conflicts with the base branch. @mbj4668 does GitHub still indicate that the PR can't be merged?)

[ubaumann]

I rebased locally and force-pushed the new history. I hope now it works

[wlupton]

@ubaumann, I'd like to request that the bin/pyang script (and probably also all other scripts currently in bin) should continue to work as before. One reason for this is that I commonly use the following idiom in makefiles (thereby avoiding the need to modify PATH or PYTHONPATH). I don't personally ever use env.sh.

PYTHONPATH=$(PYANGDIR) $(PYANGDIR)bin/pyang ...

Looking at your branch I think that it would work to create soft links in bin such as this one:

pyang -> ../pyang/scripts/pyang_tool.py

...but I don't really see why the scripts need to be moved in their entirety to ../pyang/scripts rather than leaving small driver scripts in bin.

What do you think?

[ubaumann]

...but I don't really see why the scripts need to be moved in their entirety to ../pyang/scripts rather than leaving small driver scripts in bin.

What do you think?

@wlupton Sure, it would be possible to add. On the other hand, somewhen it is to stop supporting legacy stuff. This project still supports python 2.7, and it leads to a lot of challenges.

Can a maintainer tell me what I need to do so this PR can be merged?

[mbj4668]

The only reason we still support 2.7 is that we simply didn't have had any reason for not doing so.

You wrote earlier that you renamed the script due to import issues with different versions of python. Would it help if we removed support for 2.7?

People rely on the the script bin/pyang so if we can avoid changing that it would be great.

[ubaumann]

@mbj4668 I am not sure when exactly the import behaviour of absolute and relative changed, but it differs between 2.7 and newer python 3 versions. What is the last version pyang needs to support? 3.7 (end of life: 2023-06-27) or 3.8?

I can remove older versions in the tests and clean up some stuff. I think it should be possible to make the imports work with the name pyang, and I can also put some starting scripts into the bin folder to keep the behaviour the same.

[mbj4668]

Yes I think 3.7 makes sense.

[ubaumann]

@mbj4668 @wlupton

I removed the old python versions and created the endpoint scripts in the /bin directory. Now they are working like before. I could not rename the scripts/pyang_tool.py because otherwise, I would need to change the way how python imports work by default, and that would make it complicated, and it does not really bring a benefit.

Tests are running:

https://github.com/ubaumann/pyang/actions/runs/3907015157

Now the following ways should work.

• install with pip and entry points will be installed
• use env.sh and use scripts
• use bin/* and set pythonpath manually

What do you think?

[mbj4668]

I think it looks really good, thank you for doing this! I'll have a more detailed look later this week. Perhaps this needs to be a new major version (3.0).