mbj4668 / pyang

An extensible YANG validator and converter in python
ISC License
530 stars 342 forks source link

Fix functionality on Windows #820

Open jktjkt opened 1 year ago

jktjkt commented 1 year ago

We've started using pyang in a test suite of our project for linting the YANG files. Our test suite simply tried to execute pyang something something, which failed on Windows. Upon investigating, it turned out that Wheel-based installation won't include the .bat files, and upon investigating this further, it seems that the code tried to wrap Bash shell script with a Python shebang. Hence:

scripts: port to entry_points.console_scripts

The current approach is not recoomended, and it has not worked on Windows since the distribution switched to Python wheels. With wheels, no code from setup.py runs at the installation time, which means that platform detection logic which tried to prepare BAT files on Windows was non-effective. The wheels are marked using the any tag which means "pure Python code only", which is a correct marking, but then the actual content of wheels would differ depending on whether they were built on Windows vs. Unix.

The modern alternative is to let setuptools create these scripts automatically. That thing apparently puts, e.g., pyang into $PATH on Windows, possibly generating all the requires wrappers and what not. That sounds like a best thing since sliced bread, and indeed it is.

Discovered via GNPy, thanks to Stefan Melin from Telia (@Sme791) for reporting a test failure on Windows.

Do not wrap a shell script with Python on Windows

This code attempted to produce Python wrappers around native Python code and creating some .bat files on Windows. The previous commit ported all Python scripts to a native equivalent from setuptools.

Since the only remaining script is a bash one, not a Python code, let's remove that special casing. Those on Windows need Bash for this script anyway, so let them invoke bash path/to/yang2dsdl directly.

Fix invalid regular expression on Windows

The code (Cc: @mlittlej, #809) was trying to be portable by using the OS-specific path separator in a regular expression. However, on Windows the path separator is a backslash which is a special character in a regular expression.

However, simply wrapping os.pathsep in re.compile() would not be the right thing to do here. It is very common to also use paths with Unix-style forward slashes on Windows, especially with portable projects which want to use the same scripts on Unix. Since forward slashes are not allowed in file names on Windows (and they can be used "instead of" backslashes in a lot of contexts), using both is OK on Windows. Anyone using backslashes in, say, Linux, will see a change of behavior, but come on, that would not exactly be the sanest thing to do. Also, YANG disallows a lot of funny characters in module names, so let's be reasonable here.

Of course the best fix here would be to use something like pathlib for path handling, and only apply the regex on actual file names, but I would prefer to use pyang in Windows CI for our project without doing a major refactoring here.

fredgan commented 1 year ago

For the "Fix invalid regular expression on Windows" part, I fixed it by re.escape(os.sep)

ubaumann commented 1 year ago

Hi @jktjkt, hi @fredgan, I fixed some tests, and I added pathlib. What do you think? I am at the IETF Hackathon in London and thought I could do some pyang work.

https://github.com/jktjkt/pyang/pull/1

ubaumann commented 1 year ago

As I just got informed, @jktjkt only cares about the invalid regular expression on windows which is fixed now. I am interested in moving the scrips into the repository and using entry_points. I guess you can close this PR, and I will create a new one with working tests.