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

tools: use is None consistently in Python #36606

Merged
merged 3 commits into from
Dec 25, 2020
Merged

tools: use is None consistently in Python #36606

merged 3 commits into from
Dec 25, 2020

Conversation

Trott
Copy link
Member

@Trott Trott commented Dec 22, 2020

We use is None instead of == None everywhere (which mostly just
means test.py) except in one place in genv8constants.py. Switch to is None in genv8constants.py. This is slightly more efficient, although I
can't imagine that makes a measurable difference here.

Related Issues

Fixes: https://github.com/nodejs/node/issues/<issue_number>

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added the build Issues and PRs related to build files or the CI. label Dec 22, 2020
@Trott
Copy link
Member Author

Trott commented Dec 22, 2020

I added two more minor changes: An explicit file close at the end of the script, and correcting the usage message.

@Trott Trott added the python PRs and issues that require attention from people who are familiar with Python. label Dec 22, 2020
@Trott
Copy link
Member Author

Trott commented Dec 22, 2020

I'm trying to test to make sure the script still works. Does it require a debug build? Do we not use this anymore and the right thing to do is delete it?

@richardlau
Copy link
Member

I'm trying to test to make sure the script still works. Does it require a debug build? Do we not use this anymore and the right thing to do is delete it?

It's used in

node/node.gyp

Lines 1160 to 1178 in 5bd1eec

'target_name': 'node_dtrace_ustack',
'type': 'none',
'conditions': [
[ 'node_use_dtrace=="true" and OS!="mac" and OS!="linux"', {
'actions': [
{
'action_name': 'node_dtrace_ustack_constants',
'inputs': [
'<(obj_dir)/tools/v8_gypfiles/<(STATIC_LIB_PREFIX)v8_base_without_compiler<(STATIC_LIB_SUFFIX)'
],
'outputs': [
'<(SHARED_INTERMEDIATE_DIR)/v8constants.h'
],
'action': [
'tools/genv8constants.py',
'<@(_outputs)',
'<@(_inputs)'
]
},
which requires dtrace to be enabled via configure. This looks to be the default for macOS and smartOS (according to configure):
help='build with DTrace (default is true on sunos and darwin)')

@Trott
Copy link
Member Author

Trott commented Dec 22, 2020

This looks to be the default for macOS and smartOS

And because line 1163 excludes Mac and Linux unconditionally, this is probably only used on SmartOS?

node/node.gyp

Line 1163 in 5bd1eec

[ 'node_use_dtrace=="true" and OS!="mac" and OS!="linux"', {

@Trott Trott added the request-ci Add this label to start a Jenkins CI on a PR. label Dec 24, 2020
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Dec 24, 2020
@nodejs-github-bot
Copy link
Collaborator

We use `is None` instead of `== None` everywhere (which mostly just
means test.py) except in one place in genv8constants.py. Switch to `is
None` in genv8constants.py. This is slightly more efficient, although I
can't imagine that makes a measurable difference here.

PR-URL: nodejs#36606
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
PR-URL: nodejs#36606
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
PR-URL: nodejs#36606
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@Trott
Copy link
Member Author

Trott commented Dec 25, 2020

Landed in 73a21e4...e068eec

@Trott Trott merged commit e068eec into nodejs:master Dec 25, 2020
@Trott Trott deleted the is-none branch December 25, 2020 12:29
danielleadams pushed a commit that referenced this pull request Jan 12, 2021
We use `is None` instead of `== None` everywhere (which mostly just
means test.py) except in one place in genv8constants.py. Switch to `is
None` in genv8constants.py. This is slightly more efficient, although I
can't imagine that makes a measurable difference here.

PR-URL: #36606
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
danielleadams pushed a commit that referenced this pull request Jan 12, 2021
PR-URL: #36606
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
danielleadams pushed a commit that referenced this pull request Jan 12, 2021
PR-URL: #36606
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@danielleadams danielleadams mentioned this pull request Jan 12, 2021
targos pushed a commit that referenced this pull request May 1, 2021
We use `is None` instead of `== None` everywhere (which mostly just
means test.py) except in one place in genv8constants.py. Switch to `is
None` in genv8constants.py. This is slightly more efficient, although I
can't imagine that makes a measurable difference here.

PR-URL: #36606
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
targos pushed a commit that referenced this pull request May 1, 2021
PR-URL: #36606
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
targos pushed a commit that referenced this pull request May 1, 2021
PR-URL: #36606
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@danielleadams danielleadams mentioned this pull request May 3, 2021
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. python PRs and issues that require attention from people who are familiar with Python.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants