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

[gyp_installer] Usage of GYP's Command Expansions leads to an AttributeError exception #851

Closed
cher-nov opened this issue Jun 30, 2019 · 16 comments
Labels

Comments

@cher-nov
Copy link

cher-nov commented Jun 30, 2019

CC @uilianries @SSE4

Notes

This is seems to be caused by the next piece of code:
https://github.com/bincrafters/gyp/blob/cc91d1b0dc0375a9087974a8fd09fe9539a6ab3e/pylib/gyp/input.py#L898-L905

The problem is universal_newlines=True. The Python3 documentation states next:

If encoding or errors are specified, or text is true, the file objects stdin, stdout and stderr are opened in text mode with the specified encoding and errors, as described above in Frequently Used Arguments. The universal_newlines argument is equivalent to text and is provided for backwards compatibility. By default, file objects are opened in binary mode.

Note that there's also many universal_newlines=True in the other places, introduced in by the bincrafters/gyp@067a33b.

Also note that there's much more Python3 compatibility fixes appeared in the official repo, so it's maybe have sense to rebase onto it (not replacing with it!) the Bincrafters version (moreover, their version seems to retain compatibility with Python2).

Steps to reproduce

Run on Linux: python3 gyp/gyp_main.py --depth=. --format=gypd lemme_die.gyp
Contents of lemme_die.gyp:

{
    'conditions': [
      [ '"<!(uname -m)" == "i686"', {

      }],
    ],
}

Logs

Exception: 'str' object has no attribute 'decode'
Traceback (most recent call last):
  File "share/gyp/pylib/gyp/input.py", line 487, in CallLoadTargetBuildFile
    includes, depth, check, False)
  File "share/gyp/pylib/gyp/input.py", line 408, in LoadTargetBuildFile
    build_file_data, PHASE_EARLY, variables, build_file_path)
  File "share/gyp/pylib/gyp/input.py", line 1286, in ProcessVariablesAndConditionsInDict
    build_file)
  File "share/gyp/pylib/gyp/input.py", line 1301, in ProcessVariablesAndConditionsInList
    ProcessVariablesAndConditionsInDict(item, phase, variables, build_file)
  File "share/gyp/pylib/gyp/input.py", line 1260, in ProcessVariablesAndConditionsInDict
    ProcessConditionsInDict(the_dict, phase, variables, build_file)
  File "share/gyp/pylib/gyp/input.py", line 1139, in ProcessConditionsInDict
    variables, build_file)
  File "share/gyp/pylib/gyp/input.py", line 1260, in ProcessVariablesAndConditionsInDict
    ProcessConditionsInDict(the_dict, phase, variables, build_file)
  File "share/gyp/pylib/gyp/input.py", line 1133, in ProcessConditionsInDict
    build_file)
  File "share/gyp/pylib/gyp/input.py", line 1057, in EvalCondition
    cond_expr, true_dict, false_dict, phase, variables, build_file)
  File "share/gyp/pylib/gyp/input.py", line 1071, in EvalSingleCondition
    build_file)
  File "share/gyp/pylib/gyp/input.py", line 904, in ExpandVariables
    p_stdout = p_stdout.decode()
AttributeError: 'str' object has no attribute 'decode'

Package and Environment Details

  • Package Name/Version: gyp_installer/20190423
  • Operating System+version: Linux Xubuntu 18.04.2 LTS
  • Compiler+version: gcc-7.4
  • Conan version: conan 1.16.1
  • Python version: Python 3.6.8

Conan configuration

[settings]
arch=x86_64
arch_build=x86_64
build_type=Release
compiler=gcc
compiler.libcxx=libstdc++
compiler.version=7
os=Linux
os_build=Linux
@cher-nov cher-nov added the bug label Jun 30, 2019
@Croydon
Copy link
Member

Croydon commented Jun 30, 2019

We certainty shouldn't maintain our own fork of gyp. Node.js relies on gyp heavily, and they are discussing right now if they want to maintain gyp in their org. If they decide for that, we should switch to this. If not, looking for alternatives maintainers or considering freezing it.

I believe gyp_installer got only added because it is a dependency of another package?

@cher-nov
Copy link
Author

cher-nov commented Jul 1, 2019

I believe gyp_installer got only added because it is a dependency of another package?

I don't know actually, but this was extremely useful at least for me to install it from Conan.

@uilianries
Copy link
Member

Yeah, it would be nice having another alternative. @SSE4 spent a lot of time fixing our fork. If we could use node version I would be happy, at least they are updating more frequently than us.

@SSE4
Copy link
Member

SSE4 commented Jul 2, 2019

if they have added python3 compatibility to the official repo, we may try to use that, that's great news

@cher-nov
Copy link
Author

cher-nov commented Jul 2, 2019

if they have added python3 compatibility to the official repo, we may try to use that, that's great news

They have (if we're about Google), but its Ninja generator doesn't work properly on Windows and fails on msvs_emulation.py:985 (_ExtractImportantEnvironment) with the exception TypeError: argument should be integer or bytes-like object, not 'str'. This also fails on POSIX systems, reporting gyp: Variable uname -m must expand to a string or list of strings; found a bytes in case of the aforementioned example. But I think that their Python3 port could be merged with Bincrafters one.

@SSE4
Copy link
Member

SSE4 commented Jul 2, 2019

okay, and what about nodejs/npm, do they use their own fork of gyp or just Google's repository?

@cher-nov
Copy link
Author

cher-nov commented Jul 2, 2019

It seems that they use a their own fork:
https://github.com/nodejs/node-gyp/commits/master/gyp
It also seems that it's Python3-compatible and updated regularly.

@SSE4
Copy link
Member

SSE4 commented Jul 2, 2019

okay then, let's try to migrate to node-gyp, for me it looks like the best option for now

@SSE4
Copy link
Member

SSE4 commented Jul 2, 2019

for @Croydon : gyp installer was added because some package use it as build system, for instance libuv, may be few others.

@Croydon
Copy link
Member

Croydon commented Jul 2, 2019

Google is barely maintaining GYP, it's pretty much dead for them as they have moved on to other build systems.

The Node.js community has still several discussion ongoing about the future of their build system, but in the short term they are likely going to migrate to another fork which has Python 3 support, see: nodejs/node#26620

So we could use https://github.com/refack/GYP

Croydon added a commit to bincrafters/conan-gyp_installer that referenced this issue Jul 2, 2019
@Croydon
Copy link
Member

Croydon commented Jul 2, 2019

Python 2 works with this GYP fork. Python 3 fails currently with this error https://ci.appveyor.com/project/bincrafters/conan-gyp-installer/build/job/2pin2muby1qphq74#L412

even though they are supposed to have fixed that https://github.com/refack/GYP/blame/master/gyp/generator/xcodeproj_file.py#L160

maybe we should try to push @uilianries patch upstream as he removed the cmp parameter completely, which seems to be necessary bincrafters/gyp@c606b59#diff-b71148dfd080925ebc604279f4d3fe23R1416

@Croydon
Copy link
Member

Croydon commented Jul 2, 2019

@Croydon
Copy link
Member

Croydon commented Jul 2, 2019

Created issue: refack/GYP3#48

@Croydon
Copy link
Member

Croydon commented Jul 3, 2019

@cher-nov You spoke out against replacing our fork with the upstream version, why?

@cher-nov
Copy link
Author

cher-nov commented Jul 3, 2019

@cher-nov You spoke out against replacing our fork with the upstream version, why?

Because of this: #851 (comment).

@Croydon
Copy link
Member

Croydon commented Oct 22, 2020

We decided a while ago to archive our gyp recipe.

https://github.com/bincrafters/conan-gyp_installer

Unmaintained recipe.
If you desire a Conan GYP package, I recommend to package https://github.com/nodejs/gyp-next which is an actively maintained,
backwards compatible fork of the abandoned original with Python 3 support

@Croydon Croydon closed this as completed Oct 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants