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

Feature/fix os walk py2 #3505

Merged
merged 14 commits into from Oct 7, 2018
Merged

Conversation

lasote
Copy link
Contributor

@lasote lasote commented Sep 7, 2018

Based on the fact: When py2 uses os.walk and it is passed a directory to iterate as "unicode", if iterating it finds some filename it can't decode, it fails. But if you pass a simple str() it works.

Changelog: BugFix: Fixed failures when Conan walk directories with files containing not ASCCI characters in the file name.

@ghost ghost assigned lasote Sep 7, 2018
@ghost ghost added the stage: review label Sep 7, 2018
@lasote lasote added this to the 1.8 milestone Sep 21, 2018
jgsogo
jgsogo previously requested changes Sep 21, 2018
Copy link
Contributor

@jgsogo jgsogo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Accept lasote#17
Wait for #3601

if six.PY2:
folder = unicode(folder)
save(to_file_bytes(filepath), "contents")
file = [f[0] for _, _, f in walk(folder)][0]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

file is a reserved word

folder = temp_folder()
filepath = os.path.join(folder, badfilename)
if six.PY2:
folder = unicode(folder)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe use six.u(folder) in line 46 and avoid this if clause?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting this six.u, I didn't now about it.

@lasote lasote dismissed jgsogo’s stale review September 24, 2018 08:50

Please, review again

@jgsogo
Copy link
Contributor

jgsogo commented Sep 24, 2018

I'm trying to make a snippet that fails to check that this PR is solving an issue and everything I tried runs ok (MacOS, py 2.7.15). Following the test you provided I'm making some changes to check that the issue actually exists and I thought that these lines below should fail:

        badfilename = "\xE3\x81\x82badfile.txt"
        folder = temp_folder()
        filepath = os.path.join(folder, badfilename)
        save(to_file_bytes(filepath), "contents")
        a_file = [f[0] for _, _, f in os.walk(unicode(folder))][0]

Maybe, does it depend on the locale?

…ed) and broken walk on purpose to see what happend in CI
@lasote
Copy link
Contributor Author

lasote commented Sep 24, 2018

I've pushed the test with os.walk directly, let's see what happens.
Also, I've tricked the other test failing because of the SCM, I don't want to test here other features.

@lasote
Copy link
Contributor Author

lasote commented Sep 24, 2018

@jgsogo Look at the py2 builds using directly os.walk at: https://conan-ci.jfrog.info/blue/organizations/jenkins/ConanTestSuite/detail/PR-3505/8/pipeline/10

Pushing again with the correct code.

@jgsogo
Copy link
Contributor

jgsogo commented Sep 24, 2018

I've replayed builds (MacOS failed because of a CI issue): https://conan-ci.jfrog.info/blue/organizations/jenkins/ConanTestSuite/detail/PR-3505/10/pipeline I want to see what happens with py2.

Anyway, it may depend on locale, but these changes are necessary.

@danimtb danimtb assigned memsharded and unassigned lasote Sep 26, 2018
@memsharded memsharded merged commit 183ac3f into conan-io:develop Oct 7, 2018
@ghost ghost removed the stage: review label Oct 7, 2018
grisumbras pushed a commit to grisumbras/conan that referenced this pull request Dec 27, 2018
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.

None yet

4 participants