Skip to content

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

Closed
christopherfujino wants to merge 1 commit into
nodejs:masterfrom
christopherfujino:docs-edit
Closed

Edited BUILDING.md, removing broken workaround for case where python#8763
christopherfujino wants to merge 1 commit into
nodejs:masterfrom
christopherfujino:docs-edit

Conversation

@christopherfujino
Copy link
Copy Markdown
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
Comment thread BUILDING.md Outdated
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Member

Trott commented Sep 25, 2016

@nodejs/documentation

Comment thread BUILDING.md Outdated
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Member

Trott commented Sep 26, 2016

LGTM

1 similar comment
@bengl
Copy link
Copy Markdown
Member

bengl commented Sep 26, 2016

LGTM

@jasnell
Copy link
Copy Markdown
Member

jasnell commented Sep 27, 2016

LGTM

Comment thread BUILDING.md
$ $PYTHON ./configure
$ make
```
Note that the above requires that `python` resolve to Python 2.6/2.7 and not a newer version.
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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