Skip to content

[build-script] Forward --skip-local-build to build-script-impl #27890

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.

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
Oct 28, 2019

Conversation

vedantk
Copy link
Contributor

@vedantk vedantk commented Oct 25, 2019

Otherwise, build-script-impl ignores --skip-local-build.

I would really appreciate advice on how to write a test for this.

Conceptually, I think I just need to do a dry run of build-script and verify that the arguments forwarded to build-script-impl look good. However, I can't work out how to fit this into the existing set of unit tests.

There's a BuildScriptImplOption expected option type I thought I could use for this, but it appears to have something to do with "migration.py", and I'm not sure how/where that fits into the whole picture.

There's an existing test that defines "ExpectedOption('--skip-local-build')", but that passes even though the argument isn't forwarded.

@davezarzycki davezarzycki removed their request for review October 26, 2019 10:29
Otherwise, build-script-impl ignores --skip-local-build.
@vedantk
Copy link
Contributor Author

vedantk commented Oct 28, 2019

I've added a dry-run validation test. Let's see what swiftci says.

@vedantk
Copy link
Contributor Author

vedantk commented Oct 28, 2019

@swift-ci smoke test and merge

@swift-ci swift-ci merged commit 3971c0a into swiftlang:master Oct 28, 2019
@davezarzycki
Copy link
Contributor

Hi @vedantk – I thought this was just a build-script change but apparently it broke unified (i.e. pure CMake / not build-script) builds. What's the best fix for the following?

FAIL: Swift(linux-x86_64) :: BuildSystem/skip-local-build.test-sh (512 of 12248)
******************** TEST 'Swift(linux-x86_64) :: BuildSystem/skip-local-build.test-sh' FAILED ********************
Script:
--
: 'RUN: at line 1';   /home/dave/s/u/swift/utils/build-script --dump-config --skip-local-build 2>&1 | '/usr/bin/python2.7' '/home/dave/s/u/swift/utils/PathSanitizingFileCheck' --sanitize BUILD_DIR='/home/dave/b/u/t/tools/swift' --sanitize SOURCE_DIR='/home/dave/s/u/swift' --use-filecheck '/home/dave/b/u/t/bin/FileCheck'  /home/dave/s/u/swift/validation-test/BuildSystem/skip-local-build.test-sh -check-prefix=CONFIG
: 'RUN: at line 4';   /home/dave/s/u/swift/utils/build-script --dry-run --verbose-build --skip-local-build 2>&1 | '/usr/bin/python2.7' '/home/dave/s/u/swift/utils/PathSanitizingFileCheck' --sanitize BUILD_DIR='/home/dave/b/u/t/tools/swift' --sanitize SOURCE_DIR='/home/dave/s/u/swift' --use-filecheck '/home/dave/b/u/t/bin/FileCheck'  /home/dave/s/u/swift/validation-test/BuildSystem/skip-local-build.test-sh -check-prefix=DRY
--
Exit Code: 1

Command Output (stderr):
--
/home/dave/s/u/swift/validation-test/BuildSystem/skip-local-build.test-sh:2:11: error: CONFIG: expected string not found in input
# CONFIG: "skip_local_build": true
          ^
<stdin>:1:1: note: scanning from here
Traceback (most recent call last):
^
<stdin>:2:12: note: possible intended match here
 File "SOURCE_DIR/utils/build-script", line 45, in <module>
           ^

--

********************
Testing: 0 .. 10.. 20.. 30.. 40.. 50.. 60.. 70.. 80.. 90..

8 warning(s) in tests.
Testing Time: 112.89s
********************
Failing Tests (1):
    Swift(linux-x86_64) :: BuildSystem/skip-local-build.test-sh

  Expected Passes    : 10602
  Expected Failures  : 24
  Unsupported Tests  : 1621
  Unexpected Failures: 1

@vedantk
Copy link
Contributor Author

vedantk commented Oct 29, 2019

It didn't occur to me that build-script could not be run in dry-run mode in a pure CMake build. Taking a look now.

@vedantk
Copy link
Contributor Author

vedantk commented Oct 29, 2019

@davezarzycki I think a clean fix would be to add 'UNSUPPORTED: unified-build' to the test (although, I really don't know why build-script's dry-run modes would throw exceptions in a unified build setting, perhaps that's what needs fixing?).

I can't find any documentation for unified builds and am not sure where they are implemented. If you know where the cmake logic lives, it should be sufficient to set(SWIFT_UNIFIED_BUILD ...) there and patch the test:

diff --git a/test/lit.site.cfg.in b/test/lit.site.cfg.in
index 8733e1f7b8..44438f8c83 100644
--- a/test/lit.site.cfg.in
+++ b/test/lit.site.cfg.in
@@ -89,10 +89,13 @@ if "@SWIFT_HAVE_WORKING_STD_REGEX@" == "FALSE":
     config.available_features.add('broken_std_regex')
 
 if "@SWIFT_ENABLE_RUNTIME_FUNCTION_COUNTERS@" == "TRUE":
     config.available_features.add('runtime_function_counters')
 
+if "@SWIFT_UNIFIED_BUILD@" == "TRUE":
+    config.available_features.add('swift_unified_build')
+
 if "@CMAKE_GENERATOR@" == "Xcode":
     xcode_bin_dir = os.path.join(config.llvm_obj_root, "@LLVM_BUILD_TYPE@",
                                  'bin')
     lit_config.note('Adding to path: ' + xcode_bin_dir)
     config.environment['PATH'] = \
diff --git a/validation-test/BuildSystem/skip-local-build.test-sh b/validation-test/BuildSystem/skip-local-build.test-sh
index 0897d19a24..9897dab7a3 100644
--- a/validation-test/BuildSystem/skip-local-build.test-sh
+++ b/validation-test/BuildSystem/skip-local-build.test-sh
@@ -1,5 +1,7 @@
+# UNSUPPORTED: swift_unified_build
+#
 # RUN: %swift_src_root/utils/build-script --dump-config --skip-local-build 2>&1 | %FileCheck %s -check-prefix=CONFIG
 # CONFIG: "skip_local_build": true
 
 # RUN: %swift_src_root/utils/build-script --dry-run --verbose-build --skip-local-build 2>&1 | %FileCheck %s -check-prefix=DRY
 # DRY: build-script-impl

@davezarzycki
Copy link
Contributor

davezarzycki commented Oct 30, 2019

Unified builds are just a shorthand way of saying "not standalone builds". See #27952

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants