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

drop utf-8 encoding to compute file hashes in symlink stage #399

Merged
merged 3 commits into from
Apr 7, 2017

Conversation

rhaschke
Copy link
Contributor

@rhaschke rhaschke commented Sep 5, 2016

This fixes #311 (UnicodeDecodeErrors) as pointed out in #311 (comment). Relates to #368.

@wjwwood
Copy link
Member

wjwwood commented Jan 31, 2017

@rhaschke there are CI failures for this. I've merged #368, but I don't have push permission on this branch so I cannot rebase it (I'm assuming rebasing would address the issue). Can you rebase this or otherwise fix these failures (assuming they're related to the changes). Thanks.

@rhaschke
Copy link
Contributor Author

@wjwwood: I rebased. However, Travis already fails on master. I fixed this in #428.

@wjwwood wjwwood mentioned this pull request Feb 8, 2017
@rhaschke rhaschke force-pushed the fix-#311 branch 2 times, most recently from 2be6658 to 2926bc5 Compare April 7, 2017 08:20
@rhaschke
Copy link
Contributor Author

rhaschke commented Apr 7, 2017

@wjwwood: I finally digged into this and found the underlying issue:
@jbohren's idea was to compute the md5 hashes of files to compare them. However, opening files, opens them in text-mode by default, returning string objects when calling read(), which is not compatible to digest() that expects a byte array. Recoding sometimes failed on binary files. My first fix only removed the recoding to utf8. However it's also import to read the file in binary mode in the first place.
Travis passes now. This resolves #433 too (which is a rebase of #399).

@mikepurvis
Copy link
Member

Nice!

@wjwwood
Copy link
Member

wjwwood commented Apr 7, 2017

Awesome, thanks @rhaschke.

@furushchev
Copy link
Contributor

Hi, is there any plan to release new version including this patch?

@wjwwood
Copy link
Member

wjwwood commented Jul 18, 2017

Hi, is there any plan to release new version including this patch?

Yes, I'm just swamped. But I'm going over the open pr's and issues now, so I think a new release is incoming. Sorry for the long drought of updates.

@nckswt
Copy link

nckswt commented Sep 15, 2017

Any update on the release of 0.4.5? Getting this fix would help out a lot!

@HareshKarnan
Copy link

sudo apt-get install python-catkin-tools fixed things for me !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

UnicodeDecodeError decode error during linking
6 participants