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

build: refactor configure, add --verbose #22450

Merged
merged 3 commits into from
Sep 7, 2018
Merged

build: refactor configure, add --verbose #22450

merged 3 commits into from
Sep 7, 2018

Conversation

refack
Copy link
Contributor

@refack refack commented Aug 21, 2018 •
edited
Loading

  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
Collaborator

@refack build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/622/pipeline

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

refack commented Aug 21, 2018

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

@Trott
Copy link
Member

Trott commented Aug 22, 2018

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

@refack
Copy link
Contributor Author

refack commented Aug 22, 2018 •
edited
Loading

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

refack commented Aug 22, 2018 •
edited
Loading

Lite-CI: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/627/

https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/629/pipeline

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

refack commented Aug 24, 2018

Closed by mistake

@refack refack reopened this Aug 24, 2018
@refack
Copy link
Contributor Author

refack commented Aug 24, 2018

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

@refack
Copy link
Contributor Author

refack commented Aug 29, 2018

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

joyeecheung
joyeecheung approved these changes Aug 30, 2018
View reviewed changes
configure Outdated
sys.stderr.write('\n')
sys.exit(1)

import configure
Copy link
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
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
Copy link
Contributor Author

refack commented Aug 30, 2018

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

BridgeAR commented Sep 5, 2018

@refack this needs a rebase

@refack
Copy link
Contributor Author

refack commented Sep 5, 2018

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

@refack
Copy link
Contributor Author

refack commented Sep 5, 2018

Resume: https://ci.nodejs.org/job/node-test-commit/21256/

@refack
build: rename configure to configure.py
d1c5d18 
!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 <[email protected]>
@refack
build: move meta-shebang back to configure
cc9dd0f 
PR-URL: nodejs#22450
Reviewed-By: Joyee Cheung <[email protected]>
@refack
build: add --verbose to ./configure
62a3f9b 
PR-URL: nodejs#22450
Reviewed-By: Joyee Cheung <[email protected]>
@refack
Copy link
Contributor Author

refack commented Sep 7, 2018

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

addaleax commented Sep 7, 2018

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

@refack
Copy link
Contributor Author

refack commented Sep 7, 2018

@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
@refack @targos
build: rename configure to configure.py
db52f8f 
!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 <[email protected]>
targos pushed a commit that referenced this pull request Sep 7, 2018
@refack @targos
build: move meta-shebang back to configure
db10db9 
PR-URL: #22450
Reviewed-By: Joyee Cheung <[email protected]>
targos pushed a commit that referenced this pull request Sep 7, 2018
@refack @targos
build: add --verbose to ./configure
5bae8b9 
PR-URL: #22450
Reviewed-By: Joyee Cheung <[email protected]>
@addaleax addaleax mentioned this pull request Sep 9, 2018
Merged
4 tasks
@targos targos mentioned this pull request Sep 18, 2018
Merged
@richardlau richardlau mentioned this pull request Sep 18, 2018
Merged
2 tasks
@richardlau richardlau mentioned this pull request Sep 27, 2018
Closed
@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
Member

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

@srl295 srl295 mentioned this pull request Oct 3, 2018
Closed
@richardlau richardlau mentioned this pull request Oct 8, 2018
Merged
targos added a commit to targos/node that referenced this pull request Oct 10, 2018
@targos
build: make configure script verbose by default
0f0ec4c 
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 targos mentioned this pull request Oct 10, 2018
Closed
targos added a commit that referenced this pull request Oct 10, 2018
@targos
build: make configure script verbose by default
c4aa033 
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 <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
@anmonteiro anmonteiro mentioned this pull request Oct 20, 2018
Closed
BaochengSu pushed a commit to BaochengSu/node that referenced this pull request Oct 20, 2020
@refack @BaochengSu
build: rename configure to configure.py
17d981e 
!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 <[email protected]>
(cherry picked from commit d1c5d18)
BaochengSu pushed a commit to BaochengSu/node that referenced this pull request Oct 20, 2020
@refack @BaochengSu
build: move meta-shebang back to configure
8423f97 
PR-URL: nodejs#22450
Reviewed-By: Joyee Cheung <[email protected]>
(cherry picked from commit cc9dd0f)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joyeecheung joyeecheung joyeecheung approved these changes

Assignees
No one assigned
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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@refack @nodejs-github-bot @Trott @richardlau @BridgeAR @addaleax @joyeecheung