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

Edited BUILDING.md, removing broken workaround for case where python #8763

Closed
wants to merge 1 commit into from

Conversation

christopherfujino
Copy link
Contributor

Checklist
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)
Description of change

Updated BUILDING.md, removing workaround for Python conflicts that didn't work.

@nodejs-github-bot nodejs-github-bot added the doc Issues and PRs related to the documentations. label Sep 24, 2016
@mscdex mscdex added the build Issues and PRs related to build files or the CI. label Sep 24, 2016
@@ -27,22 +27,14 @@ On OS X, you will also need:
On FreeBSD and OpenBSD, you may also need:
* libexecinfo

To build Node.js:
In order to build Node.js, the first listing of `python` in your path must
Copy link
Contributor

Choose a reason for hiding this comment

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

PATH environment variable

Maybe better? or just PATH

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, made the change to

PATH environment variable

@christopherfujino
Copy link
Contributor Author

This issue shows why simply passing the configure script to the Python 2 binary won't work.

FYI, the way I got it to work was creating a temp directory, where I symlinked the python2 & python2-config to be called python & python-config, then temporarily prepended that to my PATH variable.

@Trott
Copy link
Member

Trott commented Sep 25, 2016

@nodejs/documentation

@@ -27,22 +27,14 @@ On OS X, you will also need:
On FreeBSD and OpenBSD, you may also need:
* libexecinfo

To build Node.js:
In order to build Node.js, the first listing of `python` in your `PATH`
environment variable must point to the Python 2.6/2.7 binary. To build:
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this text should replace To build Node.js:.

I'd rather see something like:

To build Node.js:

$ ./configure
$ make

Note that the above requires that python resolve to Python 2.6/2.7 and not a newer version.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay thanks, I made this change.

Copy link
Member

@imyller imyller left a comment

Choose a reason for hiding this comment

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

LGTM

@Trott
Copy link
Member

Trott commented Sep 26, 2016

LGTM

1 similar comment
@bengl
Copy link
Member

bengl commented Sep 26, 2016

LGTM

@jasnell
Copy link
Member

jasnell commented Sep 27, 2016

LGTM

$ $PYTHON ./configure
$ make
```
Note that the above requires that `python` resolve to Python 2.6/2.7 and not a newer version.
Copy link
Contributor

Choose a reason for hiding this comment

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

Would this be better?

Note that the above requirespythonresolve to either Python 2.6 or 2.7 and not a newer version.

Copy link
Member

Choose a reason for hiding this comment

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

Landing, but doing the 2.6/2.7 -> 2.6 or 2.7 per @thefourtheye's suggestion.

Trott pushed a commit to Trott/io.js that referenced this pull request Sep 28, 2016
Updated BUILDING.md, removing workaround for Python conflicts that
didn't work.

PR-URL: nodejs#8763
Reviewed-By: Ilkka Myller <ilkka.myller@nodefield.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Bryan English <bryan@bryanenglish.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Refs: nodejs#8763
@Trott
Copy link
Member

Trott commented Sep 28, 2016

Landed in 7f7502d

@Trott Trott closed this Sep 28, 2016
jasnell pushed a commit that referenced this pull request Sep 29, 2016
Updated BUILDING.md, removing workaround for Python conflicts that
didn't work.

PR-URL: #8763
Reviewed-By: Ilkka Myller <ilkka.myller@nodefield.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Bryan English <bryan@bryanenglish.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Refs: #8763
Fishrock123 pushed a commit that referenced this pull request Oct 11, 2016
Updated BUILDING.md, removing workaround for Python conflicts that
didn't work.

PR-URL: #8763
Reviewed-By: Ilkka Myller <ilkka.myller@nodefield.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Bryan English <bryan@bryanenglish.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Refs: #8763
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Issues and PRs related to build files or the CI. doc Issues and PRs related to the documentations.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants