Skip to content

Fix cache key collisions for paths with separators#12159

Merged
zanieb merged 1 commit intomainfrom
zb/hash-path
Jul 3, 2024
Merged

Fix cache key collisions for paths with separators#12159
zanieb merged 1 commit intomainfrom
zb/hash-path

Conversation

@zanieb
Copy link
Copy Markdown
Member

@zanieb zanieb commented Jul 3, 2024 •
edited
Loading

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
@github-actions
Copy link
Copy Markdown
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 zanieb mentioned this pull request Jul 3, 2024
Closed
@zanieb
Copy link
Copy Markdown
Member Author

zanieb commented Jul 3, 2024

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

@zanieb
Copy link
Copy Markdown
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
MichaReiser
MichaReiser approved these changes Jul 3, 2024
View reviewed changes
Copy link
Copy Markdown
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
@zanieb zanieb deleted the zb/hash-path branch July 3, 2024 12:36
@BrewTestBot BrewTestBot mentioned this pull request Jul 5, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@MichaReiser MichaReiser MichaReiser approved these changes

@charliermarsh charliermarsh Awaiting requested review from charliermarsh

Assignees

No one assigned

Labels

bug Something isn't working

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

ruff has some cache bug that causes crashes

2 participants

@zanieb @MichaReiser