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

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

Closed
wants to merge 1 commit into from

Conversation

catap
Copy link
Contributor

@catap catap commented Dec 8, 2021

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
Copy link
Contributor Author

catap commented Dec 8, 2021

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

@dertuxmalwieder
Copy link

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

@LeoSebal
Copy link

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
Copy link
Contributor Author

catap commented Dec 14, 2021

@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

@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
Copy link
Contributor Author

catap commented Dec 19, 2021

@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
Copy link
Contributor Author

catap commented Dec 21, 2021

@sampsyo ping here! ;)

@LeoSebal
Copy link

LeoSebal commented Dec 21, 2021

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
Copy link
Contributor Author

catap commented Dec 21, 2021

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

@LeoSebal
Copy link

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

Yes, imports with move still seem to be failing.

@catap
Copy link
Contributor Author

catap commented Dec 21, 2021

@LeoSebal can you share a new stackrace?

@sampsyo
Copy link
Member

sampsyo commented Dec 27, 2021

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
Copy link
Member

sampsyo commented Dec 27, 2021

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