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.

Sign up for GitHub

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

Jump to bottom

Windows: accept backslashes in file names #2093

Merged
merged 2 commits into from
Sep 5, 2023
Merged

Windows: accept backslashes in file names #2093

merged 2 commits into from
Sep 5, 2023

Conversation

jktjkt
Copy link
Contributor

@jktjkt jktjkt commented Aug 31, 2023

Without this patch, a warning gets printed for any .yang files that are read using native path names:

libyang[1]: File name "D:\a\oopt-gnpy-libyang\oopt-gnpy-libyang\tests\yang\ietf-network@2018-02-26.yang" does not match module name "ietf-network".

jktjkt added a commit to Telecominfraproject/oopt-gnpy-libyang that referenced this pull request Aug 31, 2023
@jktjkt
libyang: support backslashes in file names on Windows
afe6e38 
There are some failures with `gnpy` on Windows:

 ================================== FAILURES ===================================
 ____________ test_lint_yang[ietf-network-topology@2018-02-26.yang] ____________

 yang_model = WindowsPath('D:/a/oopt-gnpy/oopt-gnpy/gnpy/yang/ext/ietf-network-topology@2018-02-26.yang')

     @pytest.mark.parametrize("yang_model", [x for x in external_path().glob('*.yang')] + [x for x in model_path().glob('*.yang')], ids=_get_basename)
     def test_lint_yang(yang_model):
         '''Run a linter on each YANG model'''
         c = ly.Context(str(external_path()) + os.pathsep + str(model_path()),
                        ly.ContextOptions.NoYangLibrary | ly.ContextOptions.DisableSearchCwd)
         assert c.parse_module(yang_model, ly.SchemaFormat.YANG) is not None
 >       assert c.errors() == []
 E       assert <[TypeError("unhashable type: 'instancemethod'") raised in repr()] list object at 0x1880e5958c0> == []
 E         (pytest_assertion plugin: representation of details failed: C:\hostedtoolcache\windows\Python\3.11.4\x64\Lib\pprint.py:178: TypeError: unhashable type: 'instancemethod'.
 E          Probably an object has a faulty __repr__.)

 tests\test_yang_lint.py:30: AssertionError
 ---------------------------- Captured stderr call -----------------------------
 libyang[1]: File name "D:\a\oopt-gnpy\oopt-gnpy\gnpy\yang\ext\ietf-network-topology@2018-02-26.yang" does not match module name "ietf-network-topology".
 ________________ test_lint_yang[ietf-network@2018-02-26.yang] _________________

That complaint about __repr__ is an impedance mismatch between pybind11
and Python (pybind/pybind11#2722), but in this case that whole thing is
triggered through a bug in libyang which assumes that the directory
separator is always `/`.

Bug: CESNET/libyang#2093
@jktjkt jktjkt force-pushed the windows-backslash-paths branch from 2dc8e20 to 41f2002 Compare August 31, 2023 19:46
jktjkt added a commit to Telecominfraproject/oopt-gnpy-libyang that referenced this pull request Aug 31, 2023
@jktjkt
libyang: support backslashes in file names on Windows
7fb9771 
There are some failures with `gnpy` on Windows:

 ================================== FAILURES ===================================
 ____________ test_lint_yang[ietf-network-topology@2018-02-26.yang] ____________

 yang_model = WindowsPath('D:/a/oopt-gnpy/oopt-gnpy/gnpy/yang/ext/ietf-network-topology@2018-02-26.yang')

     @pytest.mark.parametrize("yang_model", [x for x in external_path().glob('*.yang')] + [x for x in model_path().glob('*.yang')], ids=_get_basename)
     def test_lint_yang(yang_model):
         '''Run a linter on each YANG model'''
         c = ly.Context(str(external_path()) + os.pathsep + str(model_path()),
                        ly.ContextOptions.NoYangLibrary | ly.ContextOptions.DisableSearchCwd)
         assert c.parse_module(yang_model, ly.SchemaFormat.YANG) is not None
 >       assert c.errors() == []
 E       assert <[TypeError("unhashable type: 'instancemethod'") raised in repr()] list object at 0x1880e5958c0> == []
 E         (pytest_assertion plugin: representation of details failed: C:\hostedtoolcache\windows\Python\3.11.4\x64\Lib\pprint.py:178: TypeError: unhashable type: 'instancemethod'.
 E          Probably an object has a faulty __repr__.)

 tests\test_yang_lint.py:30: AssertionError
 ---------------------------- Captured stderr call -----------------------------
 libyang[1]: File name "D:\a\oopt-gnpy\oopt-gnpy\gnpy\yang\ext\ietf-network-topology@2018-02-26.yang" does not match module name "ietf-network-topology".
 ________________ test_lint_yang[ietf-network@2018-02-26.yang] _________________

That complaint about __repr__ is an impedance mismatch between pybind11
and Python (pybind/pybind11#2722), but in this case that whole thing is
triggered through a bug in libyang which assumes that the directory
separator is always `/`.

Bug: CESNET/libyang#2093
@jktjkt jktjkt force-pushed the windows-backslash-paths branch from 41f2002 to a90d575 Compare August 31, 2023 19:56
jktjkt added a commit to Telecominfraproject/oopt-gnpy-libyang that referenced this pull request Aug 31, 2023
@jktjkt
libyang: support backslashes in file names on Windows
52d2272 
There are some failures with `gnpy` on Windows:

 ================================== FAILURES ===================================
 ____________ test_lint_yang[ietf-network-topology@2018-02-26.yang] ____________

 yang_model = WindowsPath('D:/a/oopt-gnpy/oopt-gnpy/gnpy/yang/ext/ietf-network-topology@2018-02-26.yang')

     @pytest.mark.parametrize("yang_model", [x for x in external_path().glob('*.yang')] + [x for x in model_path().glob('*.yang')], ids=_get_basename)
     def test_lint_yang(yang_model):
         '''Run a linter on each YANG model'''
         c = ly.Context(str(external_path()) + os.pathsep + str(model_path()),
                        ly.ContextOptions.NoYangLibrary | ly.ContextOptions.DisableSearchCwd)
         assert c.parse_module(yang_model, ly.SchemaFormat.YANG) is not None
 >       assert c.errors() == []
 E       assert <[TypeError("unhashable type: 'instancemethod'") raised in repr()] list object at 0x1880e5958c0> == []
 E         (pytest_assertion plugin: representation of details failed: C:\hostedtoolcache\windows\Python\3.11.4\x64\Lib\pprint.py:178: TypeError: unhashable type: 'instancemethod'.
 E          Probably an object has a faulty __repr__.)

 tests\test_yang_lint.py:30: AssertionError
 ---------------------------- Captured stderr call -----------------------------
 libyang[1]: File name "D:\a\oopt-gnpy\oopt-gnpy\gnpy\yang\ext\ietf-network-topology@2018-02-26.yang" does not match module name "ietf-network-topology".
 ________________ test_lint_yang[ietf-network@2018-02-26.yang] _________________

That complaint about __repr__ is an impedance mismatch between pybind11
and Python (pybind/pybind11#2722), but in this case that whole thing is
triggered through a bug in libyang which assumes that the directory
separator is always `/`.

Bug: CESNET/libyang#2093
michalvasko
michalvasko reviewed Sep 4, 2023
View reviewed changes
Copy link
Member

@michalvasko michalvasko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Paths on Windows can have either / or \ as the separator? That is what the code seems to indicate. In any case, I would prefer to not change compat and just put something like FILENAME_SLASH macro into config.h.in that would change based on the platform. Then, use it in a dedicated function for getting the file name from a path.

@jktjkt
Copy link
Contributor Author

jktjkt commented Sep 4, 2023

Paths on Windows can have either / or \ as the separator?

Yes. Users can (and do) invoke this function either with paths which went through some canonicalization code (like std::filesystem::path in C++, or pathlib.Path on Python, etc), which would typically get you backslashes, but it's also very common to have these paths "hardcoded" to use forward slashes only, which Windows support just fine.

That is what the code seems to indicate. In any case, I would prefer to not change compat and just put something like FILENAME_SLASH macro into config.h.in that would change based on the platform. Then, use it in a dedicated function for getting the file name from a path.

That would work if that thing was a regular expression, but because it's a simple character search and Windows can have both / and \, this approach won't work. I don't see any other way than accepting both, and for that a new function is needed.

@michalvasko
Copy link
Member

I see. What I would do no matter what is to write a dedicated function for getting all the path components, it seems most of the code is duplicated. And why not just put a single #ifdef there for optional backslash support?

@jktjkt
Copy link
Contributor Author

jktjkt commented Sep 4, 2023

I see. What I would do no matter what is to write a dedicated function for getting all the path components, it seems most of the code is duplicated. And why not just put a single #ifdef there for optional backslash support?

So you'd like me to change these three places which perform some checks on the filename. OK, later today.

@jktjkt jktjkt closed this Sep 4, 2023
@jktjkt
Copy link
Contributor Author

jktjkt commented Sep 4, 2023

(looks like I need more coffee so as not to confuse these two buttons, yay)

@jktjkt jktjkt reopened this Sep 4, 2023
@jktjkt jktjkt force-pushed the windows-backslash-paths branch from a90d575 to 1265f6b Compare September 5, 2023 11:14
github-advanced-security[bot]
github-advanced-security bot found potential problems Sep 5, 2023
View reviewed changes
tools/lint/common.c
@@ -54,6 +54,7 @@

/* split the path to dirname and basename for further work */
*dir = strdup(path);
/* FIXME: this is broken on Windows */

Check notice

Code scanning / CodeQL

FIXME comment

FIXME comment: this is broken on Windows
jktjkt added a commit to Telecominfraproject/oopt-gnpy-libyang that referenced this pull request Sep 5, 2023
@jktjkt
libyang: support backslashes in file names on Windows (again)
1a40787 
There are some failures with `gnpy` on Windows:

 ================================== FAILURES ===================================
 ____________ test_lint_yang[ietf-network-topology@2018-02-26.yang] ____________

 yang_model = WindowsPath('D:/a/oopt-gnpy/oopt-gnpy/gnpy/yang/ext/ietf-network-topology@2018-02-26.yang')

     @pytest.mark.parametrize("yang_model", [x for x in external_path().glob('*.yang')] + [x for x in model_path().glob('*.yang')], ids=_get_basename)
     def test_lint_yang(yang_model):
         '''Run a linter on each YANG model'''
         c = ly.Context(str(external_path()) + os.pathsep + str(model_path()),
                        ly.ContextOptions.NoYangLibrary | ly.ContextOptions.DisableSearchCwd)
         assert c.parse_module(yang_model, ly.SchemaFormat.YANG) is not None
 >       assert c.errors() == []
 E       assert <[TypeError("unhashable type: 'instancemethod'") raised in repr()] list object at 0x1880e5958c0> == []
 E         (pytest_assertion plugin: representation of details failed: C:\hostedtoolcache\windows\Python\3.11.4\x64\Lib\pprint.py:178: TypeError: unhashable type: 'instancemethod'.
 E          Probably an object has a faulty __repr__.)

 tests\test_yang_lint.py:30: AssertionError
 ---------------------------- Captured stderr call -----------------------------
 libyang[1]: File name "D:\a\oopt-gnpy\oopt-gnpy\gnpy\yang\ext\ietf-network-topology@2018-02-26.yang" does not match module name "ietf-network-topology".
 ________________ test_lint_yang[ietf-network@2018-02-26.yang] _________________

That complaint about __repr__ is an impedance mismatch between pybind11
and Python (pybind/pybind11#2722), but in this case that whole thing is
triggered through a bug in libyang which assumes that the directory
separator is always `/`.

Bug: CESNET/libyang#2093
@jktjkt jktjkt force-pushed the windows-backslash-paths branch from 1265f6b to e6233b9 Compare September 5, 2023 12:18
@jktjkt jktjkt marked this pull request as ready for review September 5, 2023 12:18
@jktjkt

This comment was marked as resolved.

Sign in to view
@michalvasko
Copy link
Member

I am guessing you are missing parentheses on line 2604 on the comparison. But printing diff seems like a good idea for an improvement.

@jktjkt jktjkt force-pushed the windows-backslash-paths branch from e6233b9 to f0c235c Compare September 5, 2023 12:33
jktjkt and others added 2 commits September 5, 2023 14:36
@jktjkt
refactor: one place to verify correct schema file name
9739f38
@jktjkt
Windows: accept backslashes in file names
ca6c841 
Without this patch, a warning gets printed for any .yang files that are
read using native path names:

 libyang[1]: File name "D:\a\oopt-gnpy-libyang\oopt-gnpy-libyang\tests\yang\ietf-network@2018-02-26.yang" does not match module name "ietf-network".
@jktjkt jktjkt force-pushed the windows-backslash-paths branch from f0c235c to ca6c841 Compare September 5, 2023 12:37
@jktjkt
Copy link
Contributor Author

jktjkt commented Sep 5, 2023

I am guessing you are missing parentheses on line 2604 on the comparison.

Fixed.

@michalvasko michalvasko merged commit 35c145a into CESNET:devel Sep 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@michalvasko michalvasko michalvasko approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@jktjkt @michalvasko