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 UnicodeEncodeError when reading .count-file #3816

Merged
merged 1 commit into from Oct 24, 2018

Conversation

Projects
None yet
3 participants
@memsharded
Copy link
Contributor

commented Oct 21, 2018

Changelog: Fix: Handling corrupted lock files in cache

  • Refer to the issue that supports this Pull Request.
    Fix #3815

@ghost ghost assigned memsharded Oct 21, 2018

@ghost ghost added the stage: review label Oct 21, 2018

@CLAassistant

This comment has been minimized.

Copy link

commented Oct 21, 2018

CLA assistant check
All committers have signed the CLA.

@memsharded memsharded added this to the 1.9 milestone Oct 21, 2018

@pianoslum pianoslum force-pushed the pianoslum:develop branch 2 times, most recently from bf9d8b6 to 0c9005b Oct 21, 2018

@pianoslum

This comment has been minimized.

Copy link
Contributor

commented Oct 21, 2018

It was not too clever to read the file again if an IOException occurred... I updated the PR!

@pianoslum pianoslum force-pushed the pianoslum:develop branch from 0c9005b to 69b4de5 Oct 21, 2018

@@ -61,6 +61,10 @@ def _readers(self):
try:
return int(load(self._count_file))
except IOError:
self._output.info("Could not open %s!" % self._count_file)
return 0
except UnicodeEncodeError:

This comment has been minimized.

Copy link
@memsharded

memsharded Oct 21, 2018

Author Contributor

Are you sure about this? The problem when reading something different than an int is a ValueError. I wouldn't try to load() again the contents of the file, if it failed the first time, it will fail again. The UnicodeEncodeError can come from the load() not the int().

I would be fine with doing an except IOError, UnicodeEncodeError, ValueError to cover all possible cases.

This comment has been minimized.

Copy link
@pianoslum

pianoslum Oct 22, 2018

Contributor

ok, I updated the PR!

This comment has been minimized.

Copy link
@pianoslum

pianoslum Oct 22, 2018

Contributor

ok, I updated the PR!

This comment has been minimized.

Copy link
@pianoslum

pianoslum Oct 22, 2018

Contributor

I updated my commit, but the PR didn't update as well. Can you do it or should I open a new one?
Cheers!

This comment has been minimized.

Copy link
@pianoslum

pianoslum Oct 22, 2018

Contributor

I updated my commit, but the PR didn't update as well. Can you do it or should I open a new one?
Cheers!

This comment has been minimized.

Copy link
@pianoslum

pianoslum Oct 22, 2018

Contributor

I updated my commit, but the PR didn't update as well. Can you do it or should I open a new one?
Cheers!

@pianoslum pianoslum force-pushed the pianoslum:develop branch from 69b4de5 to 79146da Oct 22, 2018

@pianoslum
Copy link
Contributor

left a comment

I updated my commit, but the PR didn't update as well. Can you do it or should I open a new one?
Cheers!

@memsharded

This comment has been minimized.

Copy link
Contributor Author

commented Oct 22, 2018

@pianoslum it seems Github is having lots of issues, better wait until they solve them and everything goes back to normal.

@pianoslum

This comment has been minimized.

Copy link
Contributor

commented Oct 24, 2018

@memsharded The windows build fails but I can't see the problem and whether my commit is really the cause of it.
Could you maybe give me a hint if there is something wrong?

@memsharded

This comment has been minimized.

Copy link
Contributor Author

commented Oct 24, 2018

It doesn't seem related, re-launched CI.

@memsharded memsharded merged commit 4b4d48a into conan-io:develop Oct 24, 2018

1 check passed

continuous-integration/jenkins/pr-head This commit looks good
Details

@ghost ghost removed the stage: review label Oct 24, 2018

@memsharded

This comment has been minimized.

Copy link
Contributor Author

commented Oct 24, 2018

Merged!

I have run manually in the command line, and there is a small issue. Now it prints warnings like:

conan install zlib/1.2.11@conan/stable
******************************
    Running from source
******************************
Configuration:
[settings]
os=Windows
os_build=Windows
arch=x86_64
arch_build=x86_64
compiler=Visual Studio
compiler.version=15
build_type=Release
[options]
[build_requires]
[env]

WARN: C:\Users\memsharded\.conan\data\zlib\1.2.11\conan\stable.count does not contain a number!
zlib/1.2.11@conan/stable: Not found in local cache, looking in remotes...
zlib/1.2.11@conan/stable: Trying with 'conan-center'...
Downloading conanmanifest.txt
[==================================================] 121B/121B
Downloading conanfile.py

The case of IOError is also managing the file creation, which is not a warning, but normal thing. Will submit a PR right now :)

Thanks again for contributing this!

@pianoslum

This comment has been minimized.

Copy link
Contributor

commented Oct 24, 2018

Ok, sorry for that and thanks for the help :-)

grisumbras pushed a commit to grisumbras/conan that referenced this pull request Dec 27, 2018

Merge pull request conan-io#3816 from pianoslum/develop
Check for UnicodeEncodeError when reading .count-file
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.