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

addaleax · 2018-06-16T19:36:07Z

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.

refack · 2018-06-16T23:36:40Z

Makefile

@@ -98,7 +98,12 @@ out/Makefile: common.gypi deps/uv/uv.gyp deps/http_parser/http_parser.gyp \
 	$(PYTHON) tools/gyp_node.py -f make

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


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.

refack · 2018-06-16T23:39:28Z

.gitignore

@@ -69,6 +69,7 @@ ipch/

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


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

refack · 2018-06-16T23:44:37Z

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.

addaleax · 2018-06-16T23:48:46Z

@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 · 2018-06-17T00:15:48Z

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 '')

addaleax · 2018-06-17T00:20:12Z

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

refack · 2018-06-17T00:27:14Z

#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).

TimothyGu · 2018-06-16T22:06:59Z

configure

@@ -1513,6 +1518,10 @@ pprint.pprint(output, indent=2)
 write('config.gypi', do_not_edit +
      pprint.pformat(output, indent=2) + '\n')

+write('config.status', '#!/bin/sh\nset -ex\n./configure ' +


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

bnoordhuis · 2018-06-18T17:30:49Z

configure

@@ -1513,6 +1518,10 @@ pprint.pprint(output, indent=2)
 write('config.gypi', do_not_edit +
      pprint.pformat(output, indent=2) + '\n')

+write('config.status', '#!/bin/sh\nset -ex\n./configure ' +
+      ' '.join([shell_quote(arg) for arg in original_argv]) + '\n')


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

bnoordhuis · 2018-06-18T17:33:20Z

Makefile

@@ -98,7 +98,12 @@ out/Makefile: common.gypi deps/uv/uv.gyp deps/http_parser/http_parser.gyp \
 	$(PYTHON) tools/gyp_node.py -f make

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


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

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

addaleax · 2018-06-25T22:41:41Z

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 <[email protected]> Reviewed-By: Ujjwal Sharma <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>

jasnell · 2018-06-29T20:11:21Z

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 <[email protected]> Reviewed-By: Ujjwal Sharma <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>

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 pending requests for changes, 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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

addaleax commented Jun 16, 2018

refack Jun 16, 2018

refack Jun 16, 2018

addaleax Jun 16, 2018

refack Jun 17, 2018

addaleax Jun 17, 2018

bnoordhuis Jun 18, 2018

refack Jun 16, 2018

refack commented Jun 16, 2018 •

edited

Loading

addaleax commented Jun 16, 2018

refack commented Jun 17, 2018 •

edited

Loading

addaleax commented Jun 17, 2018

refack commented Jun 17, 2018

TimothyGu Jun 16, 2018

addaleax Jun 25, 2018

bnoordhuis Jun 18, 2018

addaleax Jun 25, 2018

bnoordhuis Jun 18, 2018

addaleax commented Jun 25, 2018

jasnell commented Jun 29, 2018

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

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

Conversation

addaleax commented Jun 16, 2018

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

refack commented Jun 16, 2018 • edited Loading

addaleax commented Jun 16, 2018

refack commented Jun 17, 2018 • edited Loading

addaleax commented Jun 17, 2018

refack commented Jun 17, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

addaleax commented Jun 25, 2018

jasnell commented Jun 29, 2018

refack commented Jun 16, 2018 •

edited

Loading

refack commented Jun 17, 2018 •

edited

Loading