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

test_exports fails on Python 3.13 because ntpath no longer considers paths starting with / to be absolute, causing a MISSING_INCLUDE_PATH message #110

Closed
AdamWill opened this issue Jun 13, 2024 · 2 comments

Comments

@AdamWill
Copy link

AdamWill commented Jun 13, 2024

When run under Python 3.13, the test suite fails in test_exports. The second cmakelist checked:

        project(mock)
        find_package(catkin REQUIRED)
        find_package(other_catkin REQUIRED)
        find_package(other_system REQUIRED)
        catkin_package(
        INCLUDE_DIRS ${CMAKE_CURRENT_BINARY_DIR}
        CATKIN_DEPENDS other_catkin
        DEPENDS other_system
        )

gets a result list with a MISSING_INCLUDE_PATH message in it, when it expects to get no messages. This is because on Python 3.13, ntpath.isabs no longer considers a path that starts with a single / to be absolute.

When parsing this cmakelist, CMAKE_CURRENT_BINARY_DIR is resolved to something like /e0RDahb2EYle4Jjl. This is because catkin_lint/linter.py sets info.var["CMAKE_CURRENT_BINARY_DIR"] to PathConstants.PACKAGE_BINARY, which is defined as "/%s" % generate_random_id().

Then, in catkin_lint/checks/build.py function exports(linter), we do this in the init hook:

    info.export_includes = set()

Then this in the command hook:

    includes = [info.source_relative_path(d) for d in opts["INCLUDE_DIRS"]]
    ...
    info.export_includes |= set([d for d in includes if not os.path.isabs(d)])

with this cmakelist, with Python up to 3.12, that always results in info.export_includes being an empty set, because there's only CMAKE_CURRENT_BINARY_DIR (i.e. /somerandomstring) in INCLUDE_DIRS, and that will always be considered absolute. With Python 3.13 using posixpath, that continues to be the case, and the test passes. But with Python 3.13 using ntpath - the test has the @posix_and_nt decorator which causes it to be tested both ways - we now have a non-empty info.export_includes which contains a randomly-generated path name that does not exist.

So now we reach the final hook (on_final), which does this:

    for incl in info.export_includes:
        if not info.is_existing_path(incl, check=os.path.isdir, require_source_folder=True):
            info.report(ERROR, "MISSING_INCLUDE_PATH", path=incl, file_location=info.location_of("catkin_package"))

and so of course, because we now have this non-existent path in info.export_includes, we report the MISSING_INCLUDE_PATH error.

I don't know what the appropriate upstream fix for this would be, which is why I'm sending an issue not a patch. Maybe change the implementation of PathConstants to use Windows-y paths on Windows?

AdamWill added a commit to AdamWill/catkin_lint that referenced this issue Jun 13, 2024
ntpath.isabs no longer considers paths that start with "/" to
be absolute. This is correct behaviour on Windows but causes us
problems, because our `PathConstants` use paths that start with
/ and expect these to always be considered absolute. To deal
with this, let's just filter out paths that are absolute by
ntpath *or* posixpath rules.

Signed-off-by: Adam Williamson <awilliam@redhat.com>
@AdamWill
Copy link
Author

well, I sent a possible fix as #111 , but not really sure if it's the best choice.

@AdamWill
Copy link
Author

AdamWill commented Jun 13, 2024

Another option I guess might be to double the slashes in PathConstants - make them "//%s" % generate_random_id():

Python 3.13.0b2 (main, Jun  6 2024, 00:00:00) [GCC 14.1.1 20240522 (Red Hat 14.1.1-4)] on linux
>>> import ntpath
>>> ntpath.isabs("//foo")
True
>>> import posixpath
>>> posixpath.isabs("//foo")
True

edit: sent #112 to do that, I guess pick whichever of #111 or #112 you prefer, or think of something better.

AdamWill added a commit to AdamWill/catkin_lint that referenced this issue Jun 13, 2024
ntpath.isabs no longer considers paths that start with "/" to
be absolute. This is correct behaviour on Windows but causes us
problems, because our `PathConstants` use paths that start with
/ and expect these to always be considered absolute. To deal
with this, let's add an extra slash to the `PathConstants`;
paths starting `//` are considered absolute by both ntpath and
posixpath.

Signed-off-by: Adam Williamson <awilliam@redhat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

1 participant