Skip to content

Fixed can't concat str to byte inside move - #4192

Closed
catap wants to merge 1 commit into
beetbox:masterfrom
catap:atomic-followup
Closed

Fixed can't concat str to byte inside move#4192
catap wants to merge 1 commit into
beetbox:masterfrom
catap:atomic-followup

Conversation

@catap

@catap catap commented Dec 8, 2021

Copy link
Copy Markdown
Contributor

Description

os.path.basename returns the same type that was used.

So, I have no guarantee that I needed here: b'.'' or '.'.

Let use something universal ;)

Closes #4168

@catap
catap force-pushed the atomic-followup branch 3 times, most recently from 54cebda to 6bbd2ed Compare December 8, 2021 12:42
`os.path.basename` returns the same type that was used.

So, I have no guarantee that I needed here: `b'.''` or `'.'`.

Let use something universal ;)
@catap

catap commented Dec 8, 2021

Copy link
Copy Markdown
Contributor Author

@LeoSebal this should fix your issue. Because it is a blind shot may I ask you to test it? :)

@dertuxmalwieder

Copy link
Copy Markdown

I tested this fix and it works for me. Windows 11, Python 3.11.

@LeoSebal

Copy link
Copy Markdown

Hi @catap

Sorry for the delayed answer, last week was pretty busy on my end.

That does not seem to do it. Admittedly, I might be the problem as I'm not used to troubleshooting beets.

What I did:

  1. Checked out your branch
  2. Ran python setup.py install from the repo root
  3. Edited my configuration to move instead of copy files
  4. Tried to import

Windows 10, Python 3.9.7 (conda 4.10.3)

Note that I am trying to import from on disk (I:) to another (M:), neither of which is the System disk in which the Temp folder is located.

Full log:

  File "C:\Users\hydro\anaconda3\lib\site-packages\beets\util\__init__.py", line 496, in move
    os.replace(path, dest)
OSError: [WinError 17] The system cannot move the file to a different disk drive: '\\\\?\\C:\\Users\\hydro\\AppData\\Local\\Temp\\tmpcxi4_q0j.flac' -> '\\\\?\\M:\\Musique lossless\\FTP\\2020 - Sacred Tranz Tanz\\01 Thoughts on Time.flac'

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "C:\Users\hydro\anaconda3\Scripts\beet-script.py", line 33, in <module>
    sys.exit(load_entry_point('beets==1.6.1', 'console_scripts', 'beet')())
  File "C:\Users\hydro\anaconda3\lib\site-packages\beets\ui\__init__.py", line 1285, in main

    _raw_main(args)
  File "C:\Users\hydro\anaconda3\lib\site-packages\beets\ui\__init__.py", line 1272, in _raw_main
    subcommand.func(lib, suboptions, subargs)
  File "C:\Users\hydro\anaconda3\lib\site-packages\beets\ui\commands.py", line 973, in import_func
    import_files(lib, paths, query)
  File "C:\Users\hydro\anaconda3\lib\site-packages\beets\ui\commands.py", line 943, in import_files
    session.run()
  File "C:\Users\hydro\anaconda3\lib\site-packages\beets\importer.py", line 340, in run
    pl.run_parallel(QUEUE_SIZE)
  File "C:\Users\hydro\anaconda3\lib\site-packages\beets\util\pipeline.py", line 446, in run_parallel
    raise exc_info[1].with_traceback(exc_info[2])
  File "C:\Users\hydro\anaconda3\lib\site-packages\beets\util\pipeline.py", line 358, in run

    self.coro.send(msg)
  File "C:\Users\hydro\anaconda3\lib\site-packages\beets\util\pipeline.py", line 170, in coro
    task = func(*(args + (task,)))
  File "C:\Users\hydro\anaconda3\lib\site-packages\beets\importer.py", line 1566, in manipulate_files
    task.manipulate_files(
  File "C:\Users\hydro\anaconda3\lib\site-packages\beets\importer.py", line 757, in manipulate_files
    item.move(operation)
  File "C:\Users\hydro\anaconda3\lib\site-packages\beets\library.py", line 922, in move
    self.move_file(dest, operation)
  File "C:\Users\hydro\anaconda3\lib\site-packages\beets\library.py", line 821, in move_file

    util.move(self.path, dest)
  File "C:\Users\hydro\anaconda3\lib\site-packages\beets\util\__init__.py", line 499, in move
    prefix=py3_path(b'.' + os.path.basename(dest)),
TypeError: can't concat str to bytes

@catap

catap commented Dec 14, 2021

Copy link
Copy Markdown
Contributor Author

@LeoSebal are you sure that you have updated the code and used as local root a branch for this PR?

I'm asking because I see in your stack trace at the end: prefix=py3_path(b'.' + os.path.basename(dest)), and in this PR it was changed to prefix = py3_path('.') + os.path.basename(py3_path(dest)).

@LeoSebal

Copy link
Copy Markdown

@catap I believe so: I tried to fetch and checkout again, and I believe I did it correctly. The lines 498...501 from beets/util/__init__.py correspond to your modifications.

I used the command git fetch origin pull/4192/head:atomic-followup and then checked out the branch, but I'm not used to using Github so it might be incorrect?

@catap

catap commented Dec 19, 2021

Copy link
Copy Markdown
Contributor Author

@LeoSebal it is wired.

Anyway, I see that an error happened in file C:\Users\hydro\anaconda3\lib\site-packages\beets\util\__init__.py.

Can you confirm that you run pip install or something similar from branch?

Because right now from provided stacktrace I see that this is the same error that @aibradford had reported in the issue and which I've tried to fix :)

@catap

catap commented Dec 21, 2021

Copy link
Copy Markdown
Contributor Author

@sampsyo ping here! ;)

@LeoSebal

LeoSebal commented Dec 21, 2021

Copy link
Copy Markdown

I used python setup.py install, which seems to work: I reran the install just now, and the executable called by the beet command has been modified a few minutes ago.

@catap

catap commented Dec 21, 2021

Copy link
Copy Markdown
Contributor Author

@LeoSebal have it failed after you last attempt to reinstall it?

@LeoSebal

Copy link
Copy Markdown

(oops, edited my previous comment which contained an error)

Yes, imports with move still seem to be failing.

@catap

catap commented Dec 21, 2021

Copy link
Copy Markdown
Contributor Author

@LeoSebal can you share a new stackrace?

@sampsyo

sampsyo commented Dec 27, 2021

Copy link
Copy Markdown
Member

To eliminate the guesswork, I tried to confirm exactly what the Python 3 standard library wants here. It is of course fine with all Unicode strings:

>>> tempfile.mktemp(suffix='a', prefix='b', dir='.')
'./bdwgl4nk4a'

And it, quite rightly, rejects a mixture of bytess and strs:

>>> tempfile.mktemp(suffix='a', prefix='b', dir=b'.')
[...]
    raise TypeError("Can't mix strings and bytes in path components") from None
TypeError: Can't mix strings and bytes in path components

But the sad part is that this API also rejects all-bytes invocations:

>>> tempfile.mktemp(suffix=b'a', prefix=b'b', dir=b'.')
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/usr/local/Cellar/python@3.9/3.9.9/Frameworks/Python.framework/Versions/3.9/lib/python3.9/tempfile.py", line 403, in mktemp
    file = _os.path.join(dir, prefix + name + suffix)
TypeError: can't concat str to bytes

I despise Python path APIs that insist on Unicode arguments, and I think any API that does this should be considered buggy, so this led me to actually read the docs. mktemp is deprecated and we should not be using it at all! I wish I had looked more closely at this when it was originally proposed. Let's instead use one of the supported interfaces, such as mkstemp or TemporaryFile, which—as an added bonus—actually seem to support a bytes interface:

>>> tempfile.mkstemp(suffix=b'a', prefix=b'b', dir=b'.')
(3, b'/Users/asampson/Desktop/XXX/b_d49w89_a')

So we will not need any of this py3_path hackery at all.

sampsyo added a commit that referenced this pull request Dec 27, 2021
Fixes #4168. Also closes #4192, which it supersedes.

The original problem is that this implementation used bytestrings
incorrectly to invoke `mktemp`. However, `mktemp` is deprecated, so this
PR just avoids it altogether. Fortunately, the non-deprecated APIs in
`tempfile` support all-bytes arguments.
@sampsyo

sampsyo commented Dec 27, 2021

Copy link
Copy Markdown
Member

Thank you for the attempt, @catap! I believe I have a type-correct fix, which also avoids using the deprecated mktemp function, in #4192. Closing this in favor of that.

@sampsyo sampsyo closed this Dec 27, 2021
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.

Unicode error in new atomic move

4 participants