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

Fix functionality on Windows #820

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Fix functionality on Windows #820

wants to merge 3 commits into from

Conversation

jktjkt
Copy link
Contributor

@jktjkt jktjkt commented Jul 7, 2022

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.

jktjkt added 3 commits July 7, 2022 13:54
The current approach is not recoomended [1], 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 [2]. 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 for reporting a
test failure on Windows.

[1] https://packaging.python.org/en/latest/guides/distributing-packages-using-setuptools/#scripts
[2] https://setuptools.pypa.io/en/latest/userguide/quickstart.html#entry-points-and-automatic-script-creation
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.
The code 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.

Fixes: dad5c68 Fix issue mbj4668#809: revision-date parsed wrong if >1 @ in path
@jktjkt jktjkt changed the title Fix script functionality on Windows Fix functionality on Windows Jul 7, 2022
@fredgan
Copy link
Collaborator

fredgan commented Nov 5, 2022

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

@ubaumann
Copy link
Contributor

ubaumann commented Nov 5, 2022

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.

jktjkt#1

@ubaumann
Copy link
Contributor

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants