Skip to content

build: refactor configure, add --verbose#22450

Merged
refack merged 3 commits into
nodejs:masterfrom
refack:tweak-configure-5
Sep 7, 2018
Merged

build: refactor configure, add --verbose#22450
refack merged 3 commits into
nodejs:masterfrom
refack:tweak-configure-5

Conversation

@refack

@refack refack commented Aug 21, 2018

Copy link
Copy Markdown
Contributor
  1. renaming so that IDEs can properly detect this as python
  2. move meta-shebang back to ./configure
  3. add --verbose and patch configure.py to use appropriate output function
Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • documentation is changed or added
  • commit message follows commit guidelines

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@nodejs-github-bot nodejs-github-bot added the build Issues and PRs related to build files or the CI. label Aug 21, 2018
@refack refack added the python PRs and issues that require attention from people who are familiar with Python. label Aug 21, 2018
@refack

refack commented Aug 21, 2018

Copy link
Copy Markdown
Contributor Author

/CC @nodejs/build @nodejs/build-files @nodejs/python

@Trott

Trott commented Aug 22, 2018

Copy link
Copy Markdown
Member

I would have thought that changing it to configure.py would have broken the Makefile but Travis passed. What sorcery is this?

@refack

refack commented Aug 22, 2018

Copy link
Copy Markdown
Contributor Author

What sorcery is this?

In the second commit (258d500) I restore the mega-meta-shebang so that ./configure will still work as before.

@refack refack force-pushed the tweak-configure-5 branch from 40648fe to a506eb3 Compare August 22, 2018 15:07
@refack refack closed this Aug 23, 2018
@refack refack deleted the tweak-configure-5 branch August 23, 2018 17:20
@refack refack restored the tweak-configure-5 branch August 24, 2018 19:37
@refack

refack commented Aug 24, 2018

Copy link
Copy Markdown
Contributor Author

Closed by mistake

@refack refack reopened this Aug 24, 2018
@refack

refack commented Aug 24, 2018

Copy link
Copy Markdown
Contributor Author

@nodejs/build @nodejs/build-files @nodejs/python PTAL

@refack

refack commented Aug 29, 2018

Copy link
Copy Markdown
Contributor Author

Could anyone please take a look? up or down...

Comment thread configure 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.

Hmm..can we use another name for the configure.py to avoid confusion? Or maybe a comment here?

@richardlau

Copy link
Copy Markdown
Member

We have a makefile dependency on configure:

node/Makefile

Line 132 in 221df22

config.gypi: configure

With the changes in the PR this dependency will be broken? (As in changes are more likely to be in configure.py as opposed to configure which is now merely a wrapper).

@refack

refack commented Aug 30, 2018

Copy link
Copy Markdown
Contributor Author

With the changes in the PR this dependency will be broken? (As in changes are more likely to be in configure.py as opposed to configure which is now merely a wrapper).

yeah they both should be dependents...

@BridgeAR

BridgeAR commented Sep 5, 2018

Copy link
Copy Markdown
Member

@refack this needs a rebase

@refack

refack commented Sep 5, 2018

Copy link
Copy Markdown
Contributor Author

Rebased and running CI: https://ci.nodejs.org/job/node-test-pull-request/17034/

@refack

refack commented Sep 5, 2018

Copy link
Copy Markdown
Contributor Author

!Should go with next commit!

* renaming so that IDEs can properly detect this as python
* Add dependency to Makefile

PR-URL: nodejs#22450
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
PR-URL: nodejs#22450
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
PR-URL: nodejs#22450
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
@refack

refack commented Sep 7, 2018

Copy link
Copy Markdown
Contributor Author

landed in
d1c5d18
cc9dd0f
62a3f9b

@refack refack merged commit 62a3f9b into nodejs:master Sep 7, 2018
@refack refack deleted the tweak-configure-5 branch September 7, 2018 14:21
@addaleax

addaleax commented Sep 7, 2018

Copy link
Copy Markdown
Member

@refack d1c5d18 is a broken commit for bisecting using ./configure && make test – can you squash those together the next time?

@refack

refack commented Sep 7, 2018

Copy link
Copy Markdown
Contributor Author

@refack d1c5d18 is a broken commit for bisecting using ./configure && make test – can you squash those together the next time?

Indeed... Sorry.

targos pushed a commit that referenced this pull request Sep 7, 2018
!Should go with next commit!

* renaming so that IDEs can properly detect this as python
* Add dependency to Makefile

PR-URL: #22450
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
targos pushed a commit that referenced this pull request Sep 7, 2018
PR-URL: #22450
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
targos pushed a commit that referenced this pull request Sep 7, 2018
PR-URL: #22450
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
@addaleax addaleax mentioned this pull request Sep 9, 2018
4 tasks
@richardlau richardlau added the semver-major PRs that contain breaking changes and should be released in the next major version. label Oct 3, 2018
@richardlau

Copy link
Copy Markdown
Member

Marking semver-major as per #23111 (comment).

targos added a commit to targos/node that referenced this pull request Oct 10, 2018
The change that added the --verbose flag was supposed to be
semver-major but already landed in a 10.x release.

Refs: nodejs#22450
targos added a commit that referenced this pull request Oct 10, 2018
The change that added the --verbose flag was supposed to be
semver-major but already landed in a 10.x release.

Refs: #22450

PR-URL: #23408
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
BaochengSu pushed a commit to BaochengSu/node that referenced this pull request Oct 20, 2020
!Should go with next commit!

* renaming so that IDEs can properly detect this as python
* Add dependency to Makefile

PR-URL: nodejs#22450
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
(cherry picked from commit d1c5d18)
BaochengSu pushed a commit to BaochengSu/node that referenced this pull request Oct 20, 2020
PR-URL: nodejs#22450
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
(cherry picked from commit cc9dd0f)
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. semver-major PRs that contain breaking changes and should be released in the next major version.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants