Skip to content

[build-script] SKIP_TEST_BENCHMARK => SKIP_TEST_BENCHMARKS #16878

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

Merged
merged 1 commit into from
May 30, 2018
Merged

[build-script] SKIP_TEST_BENCHMARK => SKIP_TEST_BENCHMARKS #16878

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 6 additions & 6 deletions utils/build-script
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -168,17 +168,17 @@ class HostSpecificConfiguration(object):
if build_benchmarks:
self.swift_benchmark_build_targets.append(
"swift-benchmark-" + name)
# FIXME: This probably should respect `args.benchmark`, but
# a typo in build-script-impl meant we always would do this.
self.swift_benchmark_run_targets.append(
"check-swift-benchmark-" + name)
if args.benchmark:
self.swift_benchmark_run_targets.append(
"check-swift-benchmark-" + name)

if build_external_benchmarks:
# Add support for the external benchmarks.
self.swift_benchmark_build_targets.append(
"swift-benchmark-{}-external".format(name))
self.swift_benchmark_run_targets.append(
"check-swift-benchmark-{}-external".format(name))
if args.benchmark:
self.swift_benchmark_run_targets.append(
"check-swift-benchmark-{}-external".format(name))
if test:
if test_host_only:
suffix = "-non-executable"
Expand Down
7 changes: 3 additions & 4 deletions utils/build-script-impl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1389,7 +1389,7 @@ function calculate_targets_for_host() {
fi
if [[ "${build_benchmark_this_target}" ]] && [[ "${is_in_build_list}" ]]; then
SWIFT_BENCHMARK_TARGETS+=("swift-benchmark-${stdlib_deployment_target}")
if [[ $(not ${SKIP_TEST_BENCHMARK}) ]] ; then
if [[ $(not ${SKIP_TEST_BENCHMARKS}) ]] ; then
SWIFT_RUN_BENCHMARK_TARGETS+=("check-swift-benchmark-${stdlib_deployment_target}")
fi
fi
Expand All @@ -1398,7 +1398,7 @@ function calculate_targets_for_host() {
[[ "${build_external_benchmark_this_target}" ]] &&
[[ "${is_in_build_list}" ]] ; then
SWIFT_BENCHMARK_TARGETS+=("swift-benchmark-${stdlib_deployment_target}-external")
if [[ $(not ${SKIP_TEST_BENCHMARK}) ]] ; then
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this would have been expanding to just not, which is equivalent to false, and then trying to substitute the result, which is if [[ "" ]], which is false. @gparker42, does my shell analysis make sense?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand the relevance.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're changing this code from always producing false to actually trying to execute something. That's not your fault because the old code is very wrong, but it explains the failure.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry in my head, I had already gone beyond that. It is actually sort of funny, the reason that today we do not add something here is b/c of another hack in build-script that explicitly states that this typo is why we are doing a workaround... too bad we didn't just fix it = p.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're going to have to fix this to get anything to work. LLVM not takes a command name, not a value. (I think the right code here is if [[ ! "${SKIP_TEST_BENCHMARKS}" ]] but I haven't tried it.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not invoking LLVM not. This is invoking a bash function called not defined in build-script-impl that has exactly what you described as its implementation. See:

https://github.com/apple/swift/blob/9aca727949f1338cb04855bca32e540acf998cf0/utils/build-script-impl#L962

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Beyond that do you have anything else?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh wow that's confusing. Then my earlier diagnosis of why things were failing was incorrect. Sorry for wasting your time!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You didn't waste my time! This is just confusing = p.

if [[ $(not ${SKIP_TEST_BENCHMARKS}) ]] ; then
SWIFT_RUN_BENCHMARK_TARGETS+=("check-swift-benchmark-${stdlib_deployment_target}-external")
fi
fi
Expand Down Expand Up @@ -1811,8 +1811,7 @@ for host in "${ALL_HOSTS[@]}"; do
echo "Running Swift tests for: ${SWIFT_TEST_TARGETS[@]}"
fi
if ! [[ "${SKIP_TEST_BENCHMARKS}" ]] &&
[[ "${SWIFT_RUN_BENCHMARK_TARGETS[@]}" ]] &&
! [[ "${SKIP_TEST_BENCHMARK}" ]]; then
[[ "${SWIFT_RUN_BENCHMARK_TARGETS[@]}" ]]; then
echo "Running Swift benchmarks for: ${SWIFT_RUN_BENCHMARK_TARGETS[@]}"
fi
fi
Expand Down