build: remove requirement to re-run ./configure by addaleax · Pull Request #21371 · nodejs/node · GitHub

Instead of requiring ./configure to be run again after
the file changed, first try to re-run the configure script
with the arguments with which it was originally run.

Usually, those arguments will either contain no flags,
or all flags that were passed are still supported.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

Instead of requiring `./configure` to be run again after the file changed, first try to re-run the configure script with the arguments with which it was originally run. Usually, those arguments will either contain no flags, or all flags that were passed are still supported.

I'd rather have:

$(PYTHON) configure $(something that reads .configure_args as args)

So it will be easier to implement for Windows

btw shouldn't this be dependant on ls -R *.gyp? as well?

So it will be easier to implement for Windows

We could follow the same approach pretty easily, I think

btw shouldn't this be dependant on ls -R *.gyp? as well?

I don’t know, somebody else would have to answer that

Now I understand why your pattern makes perfect sense for autotools, since it only assumes a POSIX compatible shell...

Yeah … I heard escaping parameters for cmd is fun :)

Otherwise we should be able to follow the same format here if we like

shouldn't this be dependant on ls -R *.gyp?

Ideally, yes, but can be done in a separate PR.

Suggesting a .configure_args approach, and .* files are ignored by default.

tl;dr if we simply store the args as .configure_args it could be reused in #21284

Another idea, add a --args=.configure_args to configure, then no file reading is needed.

@refack This approach and filename match what it’s how the autotools configure script works, which is the reason it’s named configure in the first place.

Dismissing for autotools similarity

refack This approach and filename match what it’s how the autotools configure script works, which is the reason it’s named configure in the first place.

But configure is not autotools generated, and we do need to consider cross platform and code reuse. I'm not going to block, but seems like a shame to miss out on this.

My suggestion is:

---
 configure | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/configure b/configure
index 97a75b98569..c8a21c35703 100755
--- a/configure
+++ b/configure
@@ -554,7 +554,13 @@ parser.add_option('-C',
     dest='compile_commands_json',
     help=optparse.SUPPRESS_HELP)
 
-(options, args) = parser.parse_args()
+args = sys.argv[1:]
+if args[0] == '--reuse':
+  with open('.used_configure_flags') as f
+  args = f.readlines()
+else:
+  write('.used_configure_flags', '\n'.join(args)
+(options, args) = parser.parse_args(args)
 
 # Expand ~ in the install prefix now, it gets written to multiple files.
 options.prefix = os.path.expanduser(options.prefix or '')

@refack How would this work on Windows? vcbuild.bat runs configure unconditionally anyway, right?

#21284 is a PR to avoid that (get it to work similar to this after your change, only there it needs to do CMD batch acrobatics).

nit: How about exec ./configure …? This should allow getting rid of the set -e.

Done!

Use pipes.quote(arg) for this (officially deprecated but shlex.quote doesn't exist in python 2.7.)

Done!

shouldn't this be dependant on ls -R *.gyp?

Ideally, yes, but can be done in a separate PR.

CI: https://ci.nodejs.org/job/node-test-pull-request/15617/

Instead of requiring `./configure` to be run again after the file changed, first try to re-run the configure script with the arguments with which it was originally run. Usually, those arguments will either contain no flags, or all flags that were passed are still supported. PR-URL: #21371 Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>

Landed in 074e7f8

Instead of requiring `./configure` to be run again after the file changed, first try to re-run the configure script with the arguments with which it was originally run. Usually, those arguments will either contain no flags, or all flags that were passed are still supported. PR-URL: #21371 Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>

PR-URL: #58603 Refs: #21371 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>

devsnek added a commit to devsnek/zero-archive that referenced this pull request Jun 16, 2018

take inspiration from nodejs/node#21371

1223c25

devsnek approved these changes Jun 16, 2018

View reviewed changes

devsnek added the build Issues and PRs related to build files or the CI. label Jun 16, 2018

iwko approved these changes Jun 16, 2018

View reviewed changes

refack previously requested changes Jun 16, 2018

View reviewed changes

TimothyGu reviewed Jun 17, 2018

View reviewed changes

ryzokuken approved these changes Jun 17, 2018

View reviewed changes

bnoordhuis approved these changes Jun 18, 2018

View reviewed changes

jasnell approved these changes Jun 21, 2018

View reviewed changes

fixup! build: remove requirement to re-run ./configure

bd1c649

addaleax added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Jun 25, 2018

TimothyGu approved these changes Jun 26, 2018

View reviewed changes

jasnell closed this Jun 29, 2018

targos mentioned this pull request Jul 3, 2018

v10.6.0 proposal #21629

Merged

Renegade334 mentioned this pull request Jun 6, 2025

build: remove config.status under make distclean #58603

Merged

nodejs-github-bot pushed a commit that referenced this pull request Jun 12, 2025

tools: remove config.status under make distclean

88cfdb3

PR-URL: #58603 Refs: #21371 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>

targos pushed a commit that referenced this pull request Jun 16, 2025

tools: remove config.status under make distclean

268c8c1

PR-URL: #58603 Refs: #21371 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>

aduh95 pushed a commit that referenced this pull request Jul 21, 2025

tools: remove config.status under make distclean

54cbf7d

PR-URL: #58603 Refs: #21371 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>

aduh95 pushed a commit that referenced this pull request Jul 24, 2025

tools: remove config.status under make distclean

9798511

PR-URL: #58603 Refs: #21371 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>

               config.gypi: configure
-              	$(error Missing or stale $@, please run ./$<)
+              	@if [ -x config.status ]; then \
+              		./config.status; \

               config.gypi: configure
-              	$(error Missing or stale $@, please run ./$<)
+              	@if [ -x config.status ]; then \
+              		./config.status; \

               /config.mk
               /config.gypi
+              /config.status

               write('config.gypi', do_not_edit +
                     pprint.pformat(output, indent=2) + '\n')
+              write('config.status', '#!/bin/sh\nset -ex\n./configure ' +

build: remove requirement to re-run ./configure by addaleax · Pull Request #21371 · nodejs/node · GitHub

Uh oh!

Uh oh!

build: remove requirement to re-run ./configure - #21371

build: remove requirement to re-run ./configure#21371addaleax wants to merge 2 commits intonodejs:masternodejs/node:masterfrom addaleax:config.statusaddaleax/node:config.statusCopy head branch name to clipboard

Conversation

addaleax commented Jun 16, 2018

Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

refack commented Jun 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

addaleax commented Jun 16, 2018

Uh oh!

refack commented Jun 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

addaleax commented Jun 17, 2018

Uh oh!

refack commented Jun 17, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

addaleax commented Jun 25, 2018

Uh oh!

jasnell commented Jun 29, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

build: remove requirement to re-run ./configure#21371
addaleax wants to merge 2 commits into
nodejs:masterfrom
addaleax:config.status

refack commented Jun 16, 2018 •
edited
Loading

refack commented Jun 17, 2018 •
edited
Loading