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

Force the python env version to 2.7 #246

Merged
merged 1 commit into from
Nov 21, 2019
Merged

Force the python env version to 2.7 #246

merged 1 commit into from
Nov 21, 2019

Conversation

LMS007
Copy link
Contributor

@LMS007 LMS007 commented Nov 15, 2019

Ran into an error when running this tool due to it wanting an older version of python (I have 3.6 in my global env), seems like the logical solution is to force an older version.

OR

Do we want to change the tool so it uses a newer version?

@LMS007 LMS007 requested a review from robertknight November 15, 2019 18:27
@hmstepanek
Copy link

Python 2.7 is being EOL so we should upgrade this to use >= Python3.6.

@robertknight
Copy link
Member

I wrote up an analysis of the problem in this Slack thread.

The basic question I asked is: Why on earth is there a Python dependency in the first place? Nothing in this repository uses Python directly!

To quote my rant at the end:

The web-ext tool for building Firefox extensions needed some logging capability, so they turned to node-bunyan which is a popular library describes itself as “a simple and fast JSON logging module for Node.js services” … seems sensible enough. Except it has this optional feature which integrates with the system process tracing facilities on macOS (DTrace) that requires compiling native code using Python. (Specifically Python 2.x)
Now every app that uses this tool will spew a ton of scary looking errors to the console if python happens to be Python 3.
Arrgghh!

The dependency in question (dtrace-provider) is optional, so everything should still work if it fails to run. From what I can see, the web-ext tool doesn't need it at all.

Ideally, we would prevent this dependency from being installed in the first place. I haven't yet found a way of disabling specific optional dependencies in npm.

@robertknight
Copy link
Member

I spent some more time looking into this. The saga is in the next paragraph, feel free to skip to the conclusion:

Looking into this further, the lack of Python 3 support is not an issue with dtrace-provider but node-gyp (see this issue) which ships with npm itself. After installing the latest version of npm with npm install -g npm@latest, removing node_modules and re-running npm install I ran into this issue. I followed a workaround in a comment on that thread to update the version of node-gyp that npm uses to the latest (v6.0.1) which resolved that issue. The dtrace-provider build still failed. Some other packages failed to build as well. I then removed package-lock.json and re-ran npm install in order to update the transitive packages to their latest versions. That resolved most of the issues. dtrace-provider failed to build but it does so much more quietly and uses a stub implementation which is fine for us.

So in summary, the error is also resolved if you:

  1. Upgrade to the latest version of npm with npm install -g npm@latest, which upgrades the tool that npm uses to build native extensions, node-gyp, to a version that supports Python 3
  2. Apply a workaround to update the version of node-gyp that npm uses to the very latest version (v6.0.1). I assume this will be shipped as part of the next update to npm itself.
  3. On the super-new Node release I'm using (v13.1.0) regenerating package-lock.json was necessary to fix a build issue with fsevents

I also looked into ways to just not install the dependencies that we don't need. I tried npm install --no-optional, but it still installs optional dependencies if package-lock.json has already been generated. It turns out that web-ext depends on an old version of bunyan which lists dtrace-provider as a required rather than optional dependency anyway.

This left me 😢 and 🤦‍♂ at the chaos of it all.

The one-line fix in this PR is frankly probably a better solution for the moment until a newer npm version is released. I wanted to add a comment to explain why it is needed but the .python-version file format doesn't support them. I will update the commit message with an explanation and then merge it.

The `web-ext` npm dependency installs `dtrace-provider` via the `web-ext >
bunyan > dtrace-provider` transitive dependency. dtrace-provider
requries node-gyp to build and, except for the very latest version of
node-gyp, this fails to build with Python 3 and produces a ton of
confusing debug output when running `npm install` if the `python` binary
is Python 3.

dtrace-provider uses a stub implementation if it fails to build, so the
issue isn't fatal but it is very confusing. We don't even need this
package, but npm doesn't provide a convenient way to override it AFAIK.

Since we use pyenv for all of Hypothesis' projects to manage the Python
version, use a .python-version file to force the use of Python 2.7 here
as a workaround.

See #246 for
the full saga.
Copy link
Member

@robertknight robertknight left a comment

Choose a reason for hiding this comment

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

😢 at the JS ecosystem. Approved as the easiest workaround for the time being.

@robertknight robertknight merged commit 6697ebd into master Nov 21, 2019
@robertknight robertknight deleted the add-py-version branch November 21, 2019 09:57
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