Skip to content

build-script fix for --extra-swift-args. #23608

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
Mar 28, 2019
Merged

build-script fix for --extra-swift-args. #23608

merged 1 commit into from
Mar 28, 2019

Conversation

atrick
Copy link
Contributor

@atrick atrick commented Mar 27, 2019

Passing an empty string to this option either as
"--extra-swift-args=''" or "--extra-swift-args ''" would cause
build-script-impl to fail to parse some arbitrary number of other
options.

This is impossible to debug from the build log because all of the
build-script-impl options still show up in the log. So, cutting and
pasting the build-script-impl command actually works.

Turns out this is why build-script build and install stages never
worked for me, beyond running cmake.

Passing an empty string to this option either as
"--extra-swift-args=''" or "--extra-swift-args ''" would cause
build-script-impl to fail to parse some arbitrary number of other
options.

This is impossible to debug from the build log because all of the
build-script-impl options still show up in the log. So, cutting and
pasting the build-script-impl command actually works.

Turns out this is why build-script build and install stages never
worked for me, beyond running cmake.
@atrick
Copy link
Contributor Author

atrick commented Mar 27, 2019

@Rostepher please review.

@atrick
Copy link
Contributor Author

atrick commented Mar 27, 2019

@swift-ci test.

@atrick
Copy link
Contributor Author

atrick commented Mar 27, 2019

@swift-ci Please Build Toolchain

@@ -763,8 +763,8 @@ class BuildScriptInvocation(object):
# add them as one command.
if args.extra_swift_args:
impl_args += [
"--extra-swift-args",
";".join(args.extra_swift_args)]
"--extra-swift-args=%s" % ';'.join(args.extra_swift_args)
Copy link
Contributor

Choose a reason for hiding this comment

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

We should probably use the more modern .format() in this case, but it's not entirely necessary as the rest of the file doesn't.

Copy link
Contributor

@Rostepher Rostepher left a comment

Choose a reason for hiding this comment

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

Looks good to me, apart from a minor nit-pick. Feel free to merge once testing passes.

@swift-ci
Copy link
Contributor

Linux Toolchain (Ubuntu 16.04)
Download Toolchain
Git Sha - 266365a

Install command
tar zxf swift-PR-23608-194-ubuntu16.04.tar.gz
More info

@swift-ci
Copy link
Contributor

macOS Toolchain
Download Toolchain
Git Sha - 266365a

Install command
tar -zxf swift-PR-23608-249-osx.tar.gz --directory ~/

@atrick atrick merged commit 7e88424 into swiftlang:master Mar 28, 2019
@atrick atrick deleted the fix-build-script branch May 8, 2019 21:36
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.

3 participants