Skip to content

tools,build: use the python interpreter running gyp througout the whole build process - #27476

Closed
dothebart wants to merge 4 commits into
nodejs:masterfrom
dothebart:use_python_executeable_of_gyp
Closed

tools,build: use the python interpreter running gyp througout the whole build process#27476
dothebart wants to merge 4 commits into
nodejs:masterfrom
dothebart:use_python_executeable_of_gyp

Conversation

@dothebart

@dothebart dothebart commented Apr 29, 2019

Copy link
Copy Markdown
  • make -j4 test (UNIX), or vcbuild test (Windows) passes

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs involving general changes in the lib/ or src/ directories. labels Apr 29, 2019
@Trott

Trott commented May 2, 2019

Copy link
Copy Markdown
Member

@nodejs/build-files @nodejs/gyp

@refack

refack commented May 2, 2019

Copy link
Copy Markdown
Contributor

Hello @dothebart and thank you for the contribution!

I think I understand what you are proposing, but I'd like to ask what was the specific motivation for this PR at this time? Did you experience problems with the current set-up?

WRT the changes in .gyp? files, I was thinking about a more generic feature in GYP3. Something like allowing explicit self-hosting of python code in the runtime that is running GYP. Something similar to <!pymod_do_main():

node/tools/gyp/pylib/gyp/input.py

Lines 876 to 882 in 6f02f15

# <!pymod_do_main(modulename param eters) loads |modulename| as a
# python module and then calls that module's DoMain() function,
# passing ["param", "eters"] as a single list argument. For modules
# that don't load quickly, this can be faster than
# <!(python modulename param eters). Do this in |build_file_dir|.
oldwd = os.getcwd() # Python doesn't like os.open('.'): no fchdir.
if build_file_dir: # build_file_dir may be None (see above).

@dothebart

dothebart commented May 2, 2019

Copy link
Copy Markdown
Author

hi,
we have been using this in arangodb for a while now; upstream V8 has a pretty complicated PR procedure. And since we decided to stay with gyp as well for the time being, I thought I'd PR this instead of continuing to patch it.
I also still have a PR in preparation that allows msbuild-out-of-source builds.

refack
refack reviewed May 2, 2019
View reviewed changes

@refack refack left a comment

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.

Left a few comments.
I'm still considering the impact of this change 🤔

Comment thread tools/gyp/gyp_main.py
# Make sure we're using the version of pylib in this repo, not one installed
# elsewhere on the system.
sys.path.insert(0, os.path.join(os.path.dirname(sys.argv[0]), 'pylib'))
sys.argv.append("-DPYTHON_EXECUTABLE=" + os.path.realpath(sys.executable))

@refack refack May 2, 2019

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.

This change should be upstreamed to https://github.com/refack/GYP

In the context of this PR it's not necessary as there are should no be any direct calls to GYP outside of through ./configure

Comment thread configure.py Outdated
Comment thread configure.py
bin_override = None if sys.platform == 'win32' else make_bin_override()
if bin_override:
config = 'export PATH:=' + bin_override + ':$(PATH)\n' + config
os.environ['PYTHON_EXECUTABLE'] = sys.executable

@refack refack May 2, 2019

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.

I believe this is not used in the context of this PR.

@refack

refack commented May 2, 2019

Copy link
Copy Markdown
Contributor

upstream V8 has a pretty complicated PR procedure.

I've forked GYP as GYP3 in https://github.com/refack/GYP, and the plan is to migrate node to use that #26620. Hopefully the fork's PR process will be more agile.

@dothebart @refack
Update configure.py
158b735 
Co-Authored-By: Refael Ackermann <refack@gmail.com>
@cclauss cclauss added the python PRs and issues that require attention from people who are familiar with Python. label Sep 11, 2019
@BridgeAR
BridgeAR force-pushed the master branch 2 times, most recently from 8ae28ff to 2935f72 Compare May 31, 2020 12:19
@jasnell

jasnell commented Jul 7, 2020

Copy link
Copy Markdown
Member

Closing as this is out of date and hasn't had any activity in over a year

@jasnell jasnell closed this Jul 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer
@refack refack refack left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs involving general changes in the lib/ or src/ directories. python PRs and issues that require attention from people who are familiar with Python.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@dothebart @Trott @refack @jasnell @cclauss @nodejs-github-bot