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 cache key collisions for paths with separators #12159

Merged
merged 1 commit into from
Jul 3, 2024
Merged

Conversation

zanieb
Copy link
Member

@zanieb zanieb commented Jul 3, 2024

Closes #12158

Hashing Path does not take into account path separators so foo/bar is the same as foobar which is no good for our case. I'm guessing this is an upstream bug, perhaps introduced by rust-lang/rust@45082b0? I'm investigating that further.

@zanieb zanieb added the bug Something isn't working label Jul 3, 2024
Copy link
Contributor

github-actions bot commented Jul 3, 2024

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

ℹ️ ecosystem check encountered linter errors. (no lint changes; 1 project error)

demisto/content (error)

ruff check --no-cache --exit-zero --ignore RUF9 --output-format concise --preview

warning: The top-level linter settings are deprecated in favour of their counterparts in the `lint` section. Please update the following options in `pyproject.toml`:
  - 'ignore' -> 'lint.ignore'
  - 'select' -> 'lint.select'
  - 'unfixable' -> 'lint.unfixable'
  - 'per-file-ignores' -> 'lint.per-file-ignores'
warning: `PGH001` has been remapped to `S307`.
warning: `PGH002` has been remapped to `G010`.
warning: `PLR1701` has been remapped to `SIM101`.
ruff failed
  Cause: Selection of deprecated rule `E999` is not allowed when preview is enabled.

@zanieb
Copy link
Member Author

zanieb commented Jul 3, 2024

Filed a bug report at rust-lang/rust#127254

@zanieb
Copy link
Member Author

zanieb commented Jul 3, 2024

I think this fix is safe because it's what was done upstream until this regression was introduced rust-lang/rust@a083dd6

@zanieb zanieb marked this pull request as ready for review July 3, 2024 04:34
Copy link
Member

@MichaReiser MichaReiser left a comment

Choose a reason for hiding this comment

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

Thank you!

We could also consider to hash the as_os_str because all we really care about is if the path changed and invalidating if the path changed but, technically is still equal, isn't a real concern.

@zanieb zanieb merged commit 47eb6ee into main Jul 3, 2024
20 checks passed
@zanieb zanieb deleted the zb/hash-path branch July 3, 2024 12:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ruff has some cache bug that causes crashes
2 participants