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.

Sign up for GitHub

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

Jump to bottom

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

Closed
wants to merge 4 commits into from
Closed

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

wants to merge 4 commits into from

Conversation

dothebart
Copy link

  • 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 related to general changes in the lib or src directory. labels Apr 29, 2019
@Trott
Copy link
Member

Trott commented May 2, 2019

@nodejs/build-files @nodejs/gyp

@refack
Copy link
Contributor

refack commented May 2, 2019

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
Copy link
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
Copy link
Contributor

@refack refack left a comment

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 🤔

tools/gyp/gyp_main.py
@@ -10,6 +10,8 @@
# 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))
Copy link
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

configure.py Outdated Show resolved Hide resolved
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
Copy link
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
Copy link
Contributor

refack commented May 2, 2019

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
Copy link
Member

jasnell commented Jul 7, 2020

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

@refack refack refack left review comments

Assignees
No one assigned
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. 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