Skip to content

Conversation

dothebart
Copy link
Contributor

@dothebart dothebart commented Oct 16, 2019

these 3 changes seem to be related to the python 3 migration, the parameter types aren't compatible.

sys.stderr.write(p_stderr) will throw with: TypeError: write() argument must be str, not bytes ; the followup comment references python 2.5, raising the error gives the correct behaviour.

I ran into this error by a gyp line spawning a process exiting errnously.

The None as a parameter to eval __builtins__ leads to the exception Exception: 'NoneType' object is not subscriptable.

… write() argument must be str, not bytes`. It seems to me that its unneccesary, and it should simply throw the exception instead.
…hrow: `Exception: 'NoneType' object is not subscriptable`.
@dothebart
Copy link
Contributor Author

dothebart commented Oct 16, 2019

 cmddigest = hashlib.sha1(command if command else self.target).hexdigest() TypeError: Unicode-objects must be encoded before hashing 

fixed by the 3rd commit.

@cclauss
Copy link
Contributor

cclauss commented Oct 17, 2019

I am +1 if we are sure these changes are compatible with both Py2 and Py3.

@dothebart
Copy link
Contributor Author

doesn't error out with python 2 on debian testing.

@rvagg
Copy link
Member

rvagg commented Oct 17, 2019

@dothebart what's the situation that's causing this error to be experienced? how could we go about reproducing it?

@dothebart
Copy link
Contributor Author

dothebart commented Oct 18, 2019

@rvagg I was putting in the complete directory as V8_ROOT from the outside in, and it seems as if gyp can only handle relative paths for source files - at least with the makefile generator. I reverted to relative paths, and this is working now.

But if its not supported to have absolute paths on filenames, gyp should rather error out; If I get it correctly it needs them relative to the location of the gyp-file - so 'stat' ing files, and ruling out absolute paths could keep people from running into misery.

[edit - wrong issue, will create an issue for this one]

@dothebart
Copy link
Contributor Author

dothebart commented Oct 18, 2019

one of these issues was caused by a sub process erroring out - here due to non-python3-ness:

'<!<(PYTHON_EXECUTABLE) -c "import sys; print sys.byteorder ")', 

so probably invoking /bin/false could reproduce it?

the other one was caused by this section in features.gypi for me:

 [ 'component and "library" in component', { 'is_component_build': 1, }, { 'is_component_build': 0, }], 

in this context:
https://github.com/arangodb/arangodb/blob/feature/upgrade-v8/3rdParty/V8/v7.1.302.28/gypfiles/features.gypi

@rvagg
Copy link
Member

rvagg commented Oct 20, 2019

sounds reasonable, @cclauss over to you I think

raise GypError("%s while executing command '%s' in %s" %
(e, contents, build_file))

p_stdout, p_stderr = p.communicate('')
Copy link
Contributor

Choose a reason for hiding this comment

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

For consistency, my preference would be the solution we use elsewhere:

if bytes != str: # Python 3 p_stdout = p_stdout.decode('utf-8') p_stderr = p_stderr.decode('utf-8') 

And then revert the change to line 914.

Copy link
Contributor

Choose a reason for hiding this comment

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

@rvagg
Copy link
Member

rvagg commented Oct 30, 2019

@cclauss if you think this is important enough please take over the PR, submit a new one or whatever needs to be done so we can progress and get 6.0.1 out. If it's not that important then we'll just hold off on it.

@dothebart
Copy link
Contributor Author

Sorry bit under stress @cclauss sent you an invite to my fork so you can make yourselves home ;)

Copy link
Contributor

@cclauss cclauss left a comment

Choose a reason for hiding this comment

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

I made two reverts to this PR and now I fully support the remaining modifications. Thanks @dothebart for your patience and persistence.

@dothebart
Copy link
Contributor Author

thanks for finishing where I left it ;)

@cclauss
Copy link
Contributor

cclauss commented Oct 30, 2019

@rvagg Can you please rerun the one Travis CI job that timed out?

@richardlau
Copy link
Member

@rvagg Can you please rerun the one Travis CI job that timed out?

I've restarted it.

rvagg pushed a commit that referenced this pull request Oct 31, 2019
PR-URL: #1925 Reviewed-By: Christian Clauss <cclauss@me.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com>
@rvagg
Copy link
Member

rvagg commented Oct 31, 2019

landed in 1b11be6. I hope the commit msg is reasonable: gyp: python3 fixes: utf8 decode, use of 'None' in eval

@dothebart
Copy link
Contributor Author

thank you!

@cclauss cclauss added the Python label Nov 7, 2019
rvagg pushed a commit that referenced this pull request Nov 18, 2019
PR-URL: #1925 Reviewed-By: Christian Clauss <cclauss@me.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

4 participants