Skip to content

Use "/" as the only glob separator, on every platform - #2112

Merged
martindurant merged 5 commits into
fsspec:masterfrom
Sreekant13:fix/glob-separator-is-always-forward-slash
Sep 14, 2026
Merged

martindurant merged 5 commits into
fsspec:masterfrom
Sreekant13:fix/glob-separator-is-always-forward-slash

Conversation

@Sreekant13

Copy link
Copy Markdown
Contributor

glob_translate takes its separators from os.path.sep and os.path.altsep, so on Windows a backslash becomes a path separator. fsspec paths always use /, as AbstractFileSystem.sep declares, and a backslash is an ordinary character in an object store key. The result is that glob returns different things on different hosts.

With a key named /data/we\ird.txt in a MemoryFileSystem:

pattern posix windows
/data/* ['/data/plain.txt', '/data/we\\ird.txt'] ['/data/plain.txt']
/data/we* ['/data/we\\ird.txt'] []
/data/*.txt ['/data/plain.txt', '/data/we\\ird.txt'] ['/data/plain.txt']

find() lists the key on both platforms. Only glob drops it, and it does so silently.

The pattern side is affected in the same way. On Windows glob_translate("we\\ird.txt") compiles to we[\\/]ird\.txt, so a pattern meant to name one key also matches we/ird.txt.

Fix

seps is fixed to /. The rest of the function is unchanged, so the compiled pattern is now the same on every platform. CPython's glob.translate takes seps as a parameter for exactly this reason, and seps=None (use the host's separators) is the right default for glob.glob over a local filesystem but not for fsspec's abstract paths.

Why local paths are not affected

glob() puts the pattern through _strip_protocol before calling glob_translate, and LocalFileSystem._strip_protocol calls make_path_posix, which replaces \ with / in every one of its NT branches. So no local pattern can reach glob_translate with a backslash in it, and this change cannot alter LocalFileSystem behaviour. test_local.py passing unchanged on Windows agrees.

glob() also derives separators from os.path for its own ends_with_sep and append_slash_to_dirname checks. Those run on the raw argument, before _strip_protocol, where a native Windows path is still a legitimate input, so I have deliberately left them alone. Happy to revisit that separately if you would rather they were consistent.

Tests

  • test_glob_translate_does_not_depend_on_the_host_os monkeypatches os.path.sep and os.path.altsep to both the posix and the Windows values and asserts the compiled pattern is identical. This one fails on Linux too without the fix, so the regression is caught wherever CI runs.
  • test_glob_matches_a_name_containing_a_backslash covers the behaviour through fs.glob on a MemoryFileSystem.

Commands run, on Windows with Python 3.12:

  • new tests against unpatched fsspec/utils.py: 2 failed, 1 passed, so they do cover the reported behaviour
  • pytest fsspec/tests/test_utils.py: 101 passed
  • pytest fsspec/tests/test_spec.py: 127 passed, 123 skipped, 1 xfailed (the skips are the bash-based posix glob comparisons, which skip on Windows regardless of this change)
  • pytest fsspec/implementations/tests/test_local.py: 152 passed, 5 skipped, 12 xfailed
  • pytest fsspec/implementations/tests/test_memory.py: 37 passed
  • ruff check and ruff format --check with the pinned v0.14.3 from .pre-commit-config.yaml: clean

Disclosure: I used AI assistance while investigating this and preparing the patch. I have reviewed every changed line and ran all of the commands above myself.

glob_translate took its separators from os.path.sep and os.path.altsep, so
on Windows a backslash became a path separator. fsspec paths always use "/",
as AbstractFileSystem.sep declares, and a backslash is an ordinary character
in an object store key, so the same call returned different results
depending on the host.

With a key named "/data/we\ird.txt" in a MemoryFileSystem:

    glob("/data/*")      posix ['/data/plain.txt', '/data/we\ird.txt']
                       windows ['/data/plain.txt']
    glob("/data/we*")    posix ['/data/we\ird.txt']
                       windows []

The key is listed by find() on both, only glob misses it. The pattern was
affected too: "we\ird.txt" compiled to we[\/]ird\.txt on Windows, so it
also matched "we/ird.txt".

Local paths are unaffected. glob() runs the pattern through
_strip_protocol first, and LocalFileSystem._strip_protocol calls
make_path_posix, which replaces every backslash with "/", so no local
pattern reaches glob_translate with one.

glob() also derives separators from os.path for its ends_with_sep check.
That runs on the raw argument before _strip_protocol, where a native path
is still possible, so it is left alone.

@fallenmi fallenmi left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This fixes the cross-platform mismatch cleanly. fsspec normalizes abstract paths to /, but the glob translator previously inherited host separators, so a literal backslash in an object-store key became a separator only on Windows. Fixing seps="/" aligns the translator with AbstractFileSystem.sep; LocalFileSystem._strip_protocol() already normalizes native Windows paths before translation.

The new tests cover both the host-independent regex and the public MemoryFileSystem.glob() behavior. I checked exact head e6b2ef54d7b8768601bff77211216beae8888009 and current merge 82f5f301a52dff60fc56ecf7077ec73677606f23; their trees are identical, and all 11 check runs plus the pull-request workflow are successful, including pytest-win, downstream, gcsfs, s3fs, and lint. I do not see a blocker.

Disclosure: I used OpenAI Codex and Anthropic Claude to assist with the exact ref/merge/policy/CI verification and semantic red-team; I verified the source paths and conclusion before submission.

@martindurant

Copy link
Copy Markdown
Member

You mention that none of this affects local glob functionality - it would be good to have a test of this, something like the reverse of test_glob_translate_does_not_depend_on_the_host_os. You would need to explicitly monkeypatch os.sep or have separate win/posix tests that skip appropriately.

Review follow-up on fsspec#2112. The PR argued that local paths cannot be affected
because glob() runs _strip_protocol first and LocalFileSystem._strip_protocol
calls make_path_posix, which replaces every backslash. That was an argument in
the description with nothing holding it.

Two tests, split by platform as suggested rather than by monkeypatching os.sep,
which would have to patch both os.sep and os.path.sep to be honest and would
send make_path_posix down its NT branch on a posix host:

- test_local_glob_is_unaffected_by_the_forward_slash_translator runs
  everywhere: a real LocalFileSystem glob over a tmp tree, with *, a bare *
  and **, and an assertion that nothing coming out carries a backslash. It is
  the counterpart named in its docstring to
  test_glob_translate_does_not_depend_on_the_host_os.
- test_a_native_windows_pattern_is_normalised_before_the_translator_sees_it is
  Windows-only: a pattern full of native backslashes still globs, and
  make_path_posix leaves none of them, which is the mechanism the whole
  argument rests on.

Neither is a stash-prove, deliberately. Local globbing was already correct
before this change, so a test that failed without it would be testing the
wrong thing; these are regression guards for the half the PR claims is
untouched.
Formatting only; ruff format --check flagged the two new test bodies and I
pushed before running it. No behaviour change.
@Sreekant13

Copy link
Copy Markdown
Contributor Author

Thanks @martindurant, and apologies for the slow reply. Added in c3771eb, plus a formatting-only follow-up.

You were right that the claim had nothing holding it. The PR argued in prose that local paths cannot be affected, because glob() runs _strip_protocol first and LocalFileSystem._strip_protocol calls make_path_posix, which replaces every backslash. That was an assertion in a description, not a test.

I went with the second option you offered, separate win/posix tests, rather than monkeypatching. To simulate a Windows host honestly I would have had to patch both os.sep and os.path.sep, since make_path_posix reads the former and glob() reads the latter, and patching os.sep on a posix host sends make_path_posix down its NT branch, which makes the test about the patch rather than about globbing.

  • test_local_glob_is_unaffected_by_the_forward_slash_translator runs everywhere: a real LocalFileSystem glob over a tmp tree with *.txt, a bare * and **, and an assertion that nothing coming out carries a backslash. Its docstring names your test as its counterpart, since that is the pairing you asked for.
  • test_a_native_windows_pattern_is_normalised_before_the_translator_sees_it is Windows-only: a pattern full of native backslashes still globs, and make_path_posix leaves none of them. That last assertion is the mechanism the whole argument rests on, so it is now pinned rather than reasoned about.

One thing worth saying plainly: neither is a stash-prove, deliberately. Local globbing was already correct before this change, so a test that failed without it would be testing the wrong thing. These are regression guards for the half the PR claims is untouched, which I think is what you were after.

$ pytest -q fsspec/implementations/tests/test_local.py fsspec/tests/test_utils.py
255 passed, 5 skipped, 12 xfailed

$ pytest -v ...::test_local_glob_is_unaffected_by_the_forward_slash_translator ...::test_a_native_windows_pattern_is_normalised_before_the_translator_sees_it
2 passed

Both confirmed as actually running rather than skipped, since I am on Windows here; the second one will skip on the Linux and macOS jobs by design.

Also merged master, and thanks @fallenmi for the review.

@Sreekant13

Copy link
Copy Markdown
Contributor Author

Hi @martindurant, a gentle note on this one.

Since the last update it has picked up an approving review from @fallenmi, who independently checked that the head and the merge trees are identical and that all 11 check runs pass, including pytest-win, downstream, gcsfs and s3fs.

The change itself is a single word, with tests behind it. The glob translator inherited host separators, so a literal backslash in a key became a path separator on Windows and stayed literal on POSIX. Setting seps="/" aligns the translator with AbstractFileSystem.sep, and LocalFileSystem._strip_protocol() already normalises native Windows paths before translation, so local behaviour is unchanged.

The tests you asked for are in c3771eb. No rush at all, and happy to rebase if it has gone stale.

Resolved one conflict in fsspec/tests/test_utils.py by keeping BOTH sides.
Upstream added `test_check_contained` and this branch added the two glob
tests, both immediately after `test_stringify_path` and both importing `os`,
so git could not tell the additions were independent. Nothing was chosen
between: upstream's cases and this branch's cases all survive, and the
decorator the conflict split is re-opened for upstream's parametrize.

`import os` is taken over `import os.path` because it covers what this branch
uses.

The fix itself is unchanged and still needed: upstream/master still derives
the glob separators from `os.path.altsep`, so a backslash remains a separator
on Windows only.

262 passed across test_utils.py and implementations/tests/test_local.py.
@Sreekant13

Copy link
Copy Markdown
Contributor Author

Rebased onto master — this had gone conflicting, so it was not mergeable regardless of review state.

The conflict was test-only and I resolved it by keeping both sides: upstream's test_check_contained (from #2127) and this branch's two glob tests all landed after test_stringify_path, and both added an os import, so git could not tell the additions were independent. Nothing was dropped from either side.

11 checks, 0 failing
262 passed across test_utils.py and implementations/tests/test_local.py

The fix itself is unchanged and still applies: master still derives the glob separators from os.path.altsep, so a backslash is a separator on Windows only, and an object-store key containing one stops matching there.

No action needed from me that I can see — happy to rebase again if it drifts.

@martindurant
martindurant merged commit 72837e0 into fsspec:master Sep 14, 2026
11 checks passed
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