Skip to content

fix: avoid deprecated os.path.commonprefix() in RelativePath - #365

Merged
gengjiawen merged 1 commit into
mainfrom
cclauss-os.path.commonpath
Oct 20, 2026
Merged

gengjiawen merged 1 commit into
mainfrom
cclauss-os.path.commonpath

Conversation

@cclauss cclauss added the python Pull requests that update Python code label Oct 12, 2026
@cclauss
cclauss force-pushed the cclauss-os.path.commonpath branch from 3a543b0 to f6d72fd Compare October 16, 2026 15:44

@gengjiawen gengjiawen left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for picking this up! Unfortunately os.path.commonpath() isn't a drop-in replacement here. When the only shared ancestor is the filesystem root, it returns the root with a trailing separator (/, C:\, \\srv\share\), so .split(os.path.sep) yields an extra empty component and prefix_len is off by one. The result is missing one .. and drops the first component of path:

path relative_to main this PR
/a/b /c ../a/b b ❌
/a / ../a '' ❌
C:\work\proj C:\Users\me\node-gyp ..\..\..\work\proj ..\..\proj ❌
\\srv\share\a \\srv\share\b ..\a '' ❌
/a/b /a/c ../b ../b ✅

For example, a .gyp file under /tmp with --depth under /home (or C:\work vs C:\Users on Windows) would get wrong relative paths written into the generated build files. CI doesn't catch this because RelativePath has no unit tests and none of the fixtures cross a top-level directory.

path_split and relative_to_split are already lists of components, so the simplest fix is to count the shared leading components directly (see the inline suggestion). I compared it with the old commonprefix() for every pair of paths up to depth 3 under /, C:\ and \\srv\share\, and the results are identical, so behavior is unchanged.

Could you also add a regression test to pylib/gyp/common_test.py? This fails on the current PR and passes on main and with the suggestion:

class TestRelativePath(unittest.TestCase):
    def test_common_parent(self):
        assert gyp.common.RelativePath("/a/b", "/a/c") == os.path.join("..", "b")

    def test_only_root_in_common(self):
        """Paths whose only common ancestor is the filesystem root."""
        assert gyp.common.RelativePath("/a/b", "/c") == os.path.join("..", "a", "b")

    def test_same_path(self):
        assert gyp.common.RelativePath("/a/b", "/a/b") == ""

Nit: please retitle the PR to a Conventional Commits title such as fix: avoid deprecated os.path.commonprefix() in RelativePath. The squash commit uses the commit title only while the PR has a single commit; after more pushes it uses the PR title, and release-please silently drops titles it can't parse. The current title also says commonpath, which won't be accurate after this change.

Comment thread pylib/gyp/common.py Resolved
@cclauss cclauss changed the title Replace deprecated os.path.commonprefix with commonpath fix: avoid deprecated os.path.commonprefix() in RelativePath Oct 20, 2026
@cclauss
cclauss force-pushed the cclauss-os.path.commonpath branch from 7936f51 to 012b7f3 Compare October 20, 2026 04:54
@cclauss
cclauss requested a review from gengjiawen October 20, 2026 05:01

@gengjiawen gengjiawen left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for the update! LGTM — verified locally (pytest + ruff), and the new tests run on all CI platforms.

@gengjiawen
gengjiawen merged commit 428343e into main Oct 20, 2026
92 checks passed
@gengjiawen
gengjiawen deleted the cclauss-os.path.commonpath branch October 20, 2026 06:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

python Pull requests that update Python code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants