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 error while reading directory with shift_jis encoded name #124

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Darkhood148
Copy link

This PR aims to fix the issue described here: #109. There seems to be a default encoding of utf-8 at many places which causes an error if shift_jis is required. I have fixed it by adding another parameter for encoding while keeping its default value to utf-8 to avoid changing the definition everywhere.

@Darkhood148 Darkhood148 force-pushed the develop branch 4 times, most recently from 3694362 to 43b0ca1 Compare March 5, 2024 14:31
@Darkhood148
Copy link
Author

@clalancette Can you look into it please?

@clalancette
Copy link
Owner

@clalancette Can you look into it please?

Sorry for the delay here.

The idea in general here is fine. But this ends up failing many of the builtin tests as-is; you can see that both in the CI jobs, and if you run make tests locally. Once all of the tests are passing, we can consider putting this in.

@Darkhood148
Copy link
Author

@clalancette I have made the changes. Some tests are still failing but those tests are also failing in the master branch which makes me believe that those might be related to another issue unrelated to my PR. With that in mind, could you please review it again?

@clalancette
Copy link
Owner

@clalancette I have made the changes. Some tests are still failing but those tests are also failing in the master branch which makes me believe that those might be related to another issue unrelated to my PR. With that in mind, could you please review it again?

As far as I know, there are no tests failing in the master branch currently, at least on Fedora. Also if you rebase this onto the latest, we'll be able to run full CI on Ubuntu 22.04, which is also passing.

@Darkhood148 Darkhood148 force-pushed the develop branch 2 times, most recently from 7fe3925 to 08f3e03 Compare March 24, 2024 18:28
@Darkhood148
Copy link
Author

Darkhood148 commented Mar 24, 2024

@clalancette I have rebased my commit on the latest commits. Although, still the exactly 8 tests are failing on my machine (Ubuntu 22.04).
Tests that fail on master branch:

FAILED tests/integration/test_hybrid.py::test_hybrid_sevendeepdirs - assert 2 == 3
FAILED tests/integration/test_parse.py::test_parse_rr_symlink_broken - FileNotFoundError: [Errno 2] No such file or directory: '/tmp/pytest-of-darkhood148/pytest-3/test_parse_rr_symlink_broken0/...
FAILED tests/integration/test_parse.py::test_parse_rr_deep_dir - assert 3 == 4
FAILED tests/integration/test_parse.py::test_parse_rr_deep - assert 3 == 4
FAILED tests/integration/test_parse.py::test_parse_rr_deep2 - assert 3 == 4
FAILED tests/integration/test_parse.py::test_parse_rr_joliet_deep - assert 3 == 4
FAILED tests/integration/test_parse.py::test_parse_rr_deeper_dir - assert 3 == 5
FAILED tests/integration/test_parse.py::test_parse_rr_hidden_relocated - assert 3 == 4

Tests that fail on my branch:

FAILED tests/integration/test_hybrid.py::test_hybrid_sevendeepdirs - assert 2 == 3
FAILED tests/integration/test_parse.py::test_parse_rr_symlink_broken - FileNotFoundError: [Errno 2] No such file or directory: '/tmp/pytest-of-darkhood148/pytest-3/test_parse_rr_symlink_broken0/...
FAILED tests/integration/test_parse.py::test_parse_rr_deep_dir - assert 3 == 4
FAILED tests/integration/test_parse.py::test_parse_rr_deep - assert 3 == 4
FAILED tests/integration/test_parse.py::test_parse_rr_deep2 - assert 3 == 4
FAILED tests/integration/test_parse.py::test_parse_rr_joliet_deep - assert 3 == 4
FAILED tests/integration/test_parse.py::test_parse_rr_deeper_dir - assert 3 == 5
FAILED tests/integration/test_parse.py::test_parse_rr_hidden_relocated - assert 3 == 4

No new tests are failing on my branch

The code was always hardcoding utf-8 as an encoding which
was producing wrong results for SHIFT-JIS (Japanese) file names. Thus I have added an optional argument for encoding which by default is set to utf-8 but can be specified to any other value if encoding is not utf-8
Darkhood148 added a commit to Darkhood148/scummvm that referenced this pull request Apr 11, 2024
The pycdlib module does not implement the encoding parameter that is required for decoding Japanese filenames. A PR has been made to acknowledge this issue: clalancette/pycdlib#124 . For the time, the PR is not merged, we need a temporary pycdlib version checker. Once the pycdlib PR has been merged, this commit can be removed
Darkhood148 added a commit to Darkhood148/scummvm that referenced this pull request Apr 11, 2024
The pycdlib module does not implement the encoding parameter that is required for decoding Japanese filenames. A PR has been made to acknowledge this issue: clalancette/pycdlib#124 . For the time, the PR is not merged, we need a temporary pycdlib version checker. Once the pycdlib PR has been merged, this commit can be removed
@Darkhood148
Copy link
Author

Heyy @clalancette . Could you look into this?

sev- pushed a commit to scummvm/scummvm that referenced this pull request Apr 24, 2024
The pycdlib module does not implement the encoding parameter that is required for decoding Japanese filenames. A PR has been made to acknowledge this issue: clalancette/pycdlib#124 . For the time, the PR is not merged, we need a temporary pycdlib version checker. Once the pycdlib PR has been merged, this commit can be removed
TiCoKH pushed a commit to TiCoKH/scummvm that referenced this pull request Apr 26, 2024
The pycdlib module does not implement the encoding parameter that is required for decoding Japanese filenames. A PR has been made to acknowledge this issue: clalancette/pycdlib#124 . For the time, the PR is not merged, we need a temporary pycdlib version checker. Once the pycdlib PR has been merged, this commit can be removed
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.

2 participants