[build-script] Introduce --dry-run mode #2241
Conversation
|
This is a useful feature but it looks awfully fragile. It seems like it would decay quickly when changes to build-script or build-script-impl accidentally fail to use the correct dry-run wrappers. Note that the failure mode is especially bad (it runs some random command). Can this feature wait until after we rewrite build-script in Python? I think it could be more supportable there. We ought to be able to do something in Python to disallow un-wrapped command execution directly, or write a test that uses the Python parser to check for for un-wrapped command execution calls and syntax in build-script itself. |
|
@gparker42 FWIW, after libdispatch was introduced in 06860a4 on Jan. 21, |
|
As for "after migrating to Python", |
|
Good point. --dry-run would also make it easier to write tests of build-script itself, which are desperately needed. I now think this is a good feature to add now, on the condition that the shell script implementation be replaced soon. |
|
While I agree this is a useful mode, I have some doubts that we can make it 100% accurate. The assumption is that build commands don't change the state that further build logic inspects and runs different commands depending on what it found. It could also violate some of the implicit preconditions that we have. For example (not a real example) that after calling These might be corner cases in abstract, but it only takes on of them to come up in practice to make a difference between an actual and a simulated run. Do we feel that this mode is useful nevertheless? (An honest question, not implying in any way that it is not.) |
In fact, it never can be 100% accurate. For example |
|
Right, that's a good example. Nevertheless, what do you think about this imprecision? Is the mode still useful despite it? |
|
I think I'm not the one to answer that question :) One useful use case I can imagine is...
|
rintaro
force-pushed
the
build-script-dryrun
branch
6 times, most recently
from
ef46862 to
c239802
Compare
rintaro
force-pushed
the
build-script-dryrun
branch
4 times, most recently
from
ce7a795 to
4c9dc52
Compare
rintaro
force-pushed
the
build-script-dryrun
branch
from
4c9dc52 to
483a081
Compare
rintaro
force-pushed
the
build-script-dryrun
branch
from
483a081 to
815ac8c
Compare
rintaro
force-pushed
the
build-script-dryrun
branch
from
815ac8c to
d5fdeb3
Compare
|
Will rebase again after #2497 land. |
|
To get some early feedback: @swift-ci Please test |
| @@ -1041,6 +1056,9 @@ details of the setups of other systems or automated environments.""") | |||
| print_with_argv0("Unknown operating system.") | |||
| return 1 | |||
|
|
|||
| # Set dry_run flag | |||
| shell.dry_run = args.dry_run | |||
There was a problem hiding this comment.
This part should be reordered.
|
@swift-ci Please test |
rintaro
force-pushed
the
build-script-dryrun
branch
2 times, most recently
from
de8f464 to
3785ef3
Compare
|
#2497 has landed, could you rebase please? |
rintaro
force-pushed
the
build-script-dryrun
branch
from
3785ef3 to
fcbc908
Compare
|
Rebased, still testing locally. |
rintaro
force-pushed
the
build-script-dryrun
branch
from
fcbc908 to
cc69cdd
Compare
|
Rebased. This is caused by Foundation build system where concatenating Still running Mac toolchain build. |
|
@swift-ci Please test |
|
@swift-ci Please smoke test |
| @@ -968,6 +983,9 @@ details of the setups of other systems or automated environments.""") | |||
| if '--check-args-only' in args.build_script_impl_args: | |||
| return 0 | |||
|
|
|||
| # Set dry_run flag | |||
There was a problem hiding this comment.
This comment is redundant with the code; I don't think it adds much.
|
The changes LGTM. |
|
Hm, Linux tests timed out. Let's try again. @swift-ci Please smoke test Linux platform |
| { set +x; } 2>/dev/null | ||
| call pushd "${FOUNDATION_SOURCE_DIR}" | ||
| call ${NINJA_BIN} TestFoundation | ||
| call env LD_LIBRARY_PATH="$(get_host_install_destdir ${host})$(get_host_install_prefix ${host})"/lib/swift/:"${build_dir}/Foundation":"${XCTEST_BUILD_DIR}":${LD_LIBRARY_PATH} "${build_dir}"/TestFoundation/TestFoundation |
There was a problem hiding this comment.
Same here, but I don't think test command itself doesn't necessarily expect ${PWD} == ${FOUNDATION_SOURCE_DIR}.
Can I:
with_pushd "${FOUNDATION_SOURCE_DIR}" \
call ${NINJA_BIN} TestFoundation
call env LD_LIBRARY_PATH=...
Of course I will test it.
Tested, it seems to be OK.
There was a problem hiding this comment.
Absolutely -- as long as it works. Feel free to do a conservative with_pushd for now.
… build-script-impl
Abstract `set -x; ... { set +x; }` block.
As a side effect, prevent displaying confusing trace like:
```
++ cmake_config_opt cmark
++ product=cmark
++ [[ Ninja == \X\c\o\d\e ]]
```
If -n or --dry-run is specified in command line arguments, print the commands that would be executed to stdout, but do not execute them. Supported in build-script and build-toolchain. utils/build-script -n -Rt utils/build-script -n --preset=buildbot_incremental,tools=RA,stdlib=RA utils/build-toolchain -n local.swift
rintaro
force-pushed
the
build-script-dryrun
branch
from
cc69cdd to
552590e
Compare
|
Fixed, sorry for wasting CI resource! |
|
@swift-ci Please test |
|
@swift-ci Please smoke test |
[pull] swiftwasm from main
What's in this pull request?
If
-nor--dry-runis specified in command line arguments, print the commands that would be executed to stdout, but do not execute them.Using this, you can safely know such as exact cmake options, build targets, or directories to be created, without actual build.
Supported in
build-script(both preset and normal) andbuild-toolchain.Example dry-run output by
utils/build-toolchain -n local.swiftMac OS X
Linux
anymore.
And we don't have to write
set -x;and{ set +x; } 2>/dev/nullblock manually.As above, this
build-script-imploutputs relativelysimpleclean trace than current one. If it lacks any necessary information, please tell me.callcan only support simple command and arguments construct. To support commands that includes pipes(|) or redirects(<,>,>>, etc.), we have to do something special like:Because we need special care about complicated quoting for this, it's not included in this PR.
Luckly, we have only one block using pipes.
To set environment variables in child process, you need
call env FOO=VAL commandinstead ofFOO=VAL commandorcall FOO=VAL command.To simulate
set -xtrace as exact as possible, command dump in dry-run mode uses Pythonpipes.quote(). Since it significantly slows down serialization, please someone tell me better way!Moreover,
pipes.quote()emits inaccurate result for argument like"foo 'bar'".pipes.quote()emits'foo '"'"'bar'"'"'', whereasset -xemits'foo '\''bar'\'''Before merging this pull request to apple/swift repository: