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

Bug fix/fix error handling #1925

Closed
wants to merge 7 commits into from

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

@@ -911,9 +911,7 @@ def ExpandVariables(input, phase, variables, 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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants