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

Check for Mach-O using byte strings to allow case of non unicode chars #635

Merged
merged 1 commit into from
Apr 18, 2020
Merged

Check for Mach-O using byte strings to allow case of non unicode chars #635

merged 1 commit into from
Apr 18, 2020

Conversation

QuantumEntangledAndy
Copy link
Contributor

The recent change of not encoding byte strings in bdist_mac is breaking the build. Methods using bytes should not be mixed with str. This patch unifies the relevant code to use all bytes.

NOTE: There is one step where I choose to decode

else:
                        files.append(name.decode())

This is because

filePath = os.path.join(self.binDir, fileName)

The fileName is generated from type str (from os.walk) but when we add extra files in the loop they were added as type bytes.

Currently I decode back to string then append to the list, but alternatively it may be better to encode the results of os.walk so that it is all bytes throughout.

@QuantumEntangledAndy
Copy link
Contributor Author

I have a further patch that gets @loader_path working (see #634) BUT when I came to merge I had conflicts from your changes to not decode. When I pulled your changes I ran into multiple errors related to using str when bytes was expected.

If you want I can roll the addition of @loader_path into this pull request too. Or I can wait until this is merged and pull request of that.

@marcelotduarte
Copy link
Owner

That is my patch. It was untested and I forget to declare this. In the issue #529 I requested tests but no one answered. As you use mac, if you find it better to revert the patch and you fix the issue, I even prefer it.

@QuantumEntangledAndy
Copy link
Contributor Author

QuantumEntangledAndy commented Apr 17, 2020

I actually had #529 and had a different fix. Rather than not decoding I did this instead

try:
    line = p.stdout.readline().decode()
except UnicodeDecodeError:
    continue
if "Mach-O" in line:

Here I assume that if we can't decode it is not Mach-O. Which fixes #529 in my case I am not sure what the original case was.

We have two choice

  1. resolve by changing all to bytes (many changes, but possibly more flexible)

  2. resolve by skipping any files where it is not decodable (less changes, but possibly not as flexible).

Which would you prefer?

@marcelotduarte
Copy link
Owner

I reverted my patch. Please send your patch, your approach is ok to me, the option 2.

@QuantumEntangledAndy
Copy link
Contributor Author

I have made this pull request work as indicated in option 2.

Fixes #529

@QuantumEntangledAndy
Copy link
Contributor Author

Actually I am thinking of an option 3.

Use str everywhere except when testing for Mach-O this will let us test for Mach-O in files with non unicode bytes, while still not requiring many changes

@marcelotduarte
Copy link
Owner

ok, put it in another PR

@QuantumEntangledAndy
Copy link
Contributor Author

A completely separate PR? Or ammend this one?

@marcelotduarte
Copy link
Owner

Can be in this PR, but change the title to not confuse anthony.

@QuantumEntangledAndy QuantumEntangledAndy changed the title Use byte string throughout when not decoding Check for Mach-O using byte strings to allow case of non unicode chars Apr 17, 2020
@QuantumEntangledAndy
Copy link
Contributor Author

Latest commit is using option number 3. This allows searching for Mach-O even if there are non unicode chars but keeps the rest of the code as str

@anthony-tuininga anthony-tuininga merged commit 5372cbf into marcelotduarte:master Apr 18, 2020
@QuantumEntangledAndy QuantumEntangledAndy deleted the QuantumEntangledAndy-patch-2 branch April 19, 2020 06:24
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.

setRelativeReferencePaths() Error when trying to freeze wxPython app
3 participants