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

Fix action functions with unicode constants py2 #3139

Merged

Conversation

bdbaddog
Copy link
Contributor

Fix for bug reported on mailing list by Eugene Leskinen
https://pairlist4.pair.net/pipermail/scons-users/2018-June/007067.html

Here's a small SConstruct to exercise. A unit test has been added to cover the core issue which was that to_Bytes wasn't converting unicode strings to bytearray on python 2.7. This was then getting joined with bytes and throwing a stack trace.

'''
SConstruct file to re-produce the issue
both commands
    scons hello.txt
and
    scons hello.txt WORK=1
should create the same contents of 'hello.txt' file
'''
import os

if ARGUMENTS.get('WORK'):
    def build_it(target, source, env):
        with open(target[0].abspath, "w") as w:
            os.write(w.fileno(), '\xef\xbb\xbf')
            w.write("Hello world")
else:
    def build_it(target, source, env):
        with open(target[0].abspath, "w") as w:
            os.write(w.fileno(), '\xef\xbb\xbf')
            w.write(u"Hello world")
Builder(
    action=Action(
        build_it
    )
)(Environment(), 'hello.txt', '')

Contributor Checklist:

  • I have created a new test or updated the unit tests to cover the new/changed functionality.
  • I have updated master/src/CHANGES.txt directory (and read the README.txt in that directory)
  • I have updated the appropriate documentation

… an array of items one of which was unicode.
…ting it to a bytearray utf-8 encoded. This was breaking python action functions with unicode constant strings
@bdbaddog bdbaddog requested a review from garyo June 23, 2018 19:05
@garyo
Copy link
Contributor

garyo commented Jun 24, 2018

Fixes in 783a97 and 97f9431 look good to me.

@bdbaddog bdbaddog merged commit 68865f9 into SCons:master Jun 24, 2018
@bdbaddog bdbaddog deleted the fix_action_functions_with_unicode_constants_py2 branch June 24, 2018 16:30
@lightmare
Copy link

This change looks unnecessarily complicated. The culprit is this condition:

    if isinstance (s, (bytes, bytearray)) or bytes is str:
        return s

It always passes on python2, regardless of what s is. Dodging this with another condition makes the code more confusing.

Anything that's not None, bytes or bytearray should be utf-8-encoded, on both py2 and py3, right? Then what about this:

def to_bytes(s):
    if s is None:
        return b'None'
    if isinstance(s, (bytes, bytearray)):
        return s
    return bytearray(s, 'utf-8')

@bdbaddog
Copy link
Contributor Author

On py2 we want a string an not bytearray().. So not really.
In essence that's say in if it's a byte or bytearray or we're on py2 then return s.
So I guess it could be simplified to if not PY3..

if isinstance (s, (bytes, bytearray)) or not PY3

@lightmare
Copy link

not PY3 is basically the same thing as bytes is str. The error in that part of the condition is that it doesn't take the type of s into account -- it can be str (i.e. bytes) or unicode on py2 (or maybe even UserString, not sure) -- and you don't want to return s when it's unicode.

On py2 we want a string an not bytearray().. So not really.

It was changed to bytearray for py2 in master so I assumed that's desired. bytearray can be used in the same way on both py2 and py3 (unlike bytes whose constructor doesn't accept encoding on py2).

@bdbaddog
Copy link
Contributor Author

bdbaddog commented Jun 28, 2018 via email

@lightmare
Copy link

That's what the function I posted above does. Returns py2.str as is; py2.unicode as bytearray; py3.str as bytearray.

@bdbaddog
Copy link
Contributor Author

bdbaddog commented Jun 30, 2018 via email

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.

3 participants