[WIP] SR-237 Refactor `build-script` argument parsing logic into separate module #11827

Rostepher · 2017-09-09T00:42:05Z

Purpose

This PR is part of my ongoing effort to refactor build-script and build-script-impl into a single script. Here I have re-used part of @rintaro's argparser_builder module from PR #2190 and then implemented a new argument parser for the main script mode.

This PR is still a WIP, I still want to add testing for the argparser_builder module and then integrate it with the main script. There's still some other groundwork to finish before that can be accomplished.

rdar://problem/34336890

Rostepher · 2017-09-09T01:40:46Z

I'm going to add tests that parse in the preset file and ensure that all the presets still work under this new parser. If there's any conflicts I'll consider that a bug in the new implementation. In addition I have planned to manually get all the flags accepted by the current parser implementation and have a static test against that list to ensure all flags are supported.

gottesmm

Some quick comments.

gottesmm · 2017-09-12T19:02:06Z

utils/build_swift/defaults.py

+# These options are not exposed as command line options on purpose.  If you
+# need to change any of these, you should do so on trunk or in a branch.
+
+LLVM_TARGETS_TO_BUILD = 'X86;ARM;AArch64;PowerPC'


No. We want these to be configurable from the command line. For our smoke tests (where we only test X86), we only want to build the X86 LLVM target.

For the purpose of this PR I don't think we should change this value. I don't want to change too much in a single PR. I copied this value from the previous default. It won't be too late to change it in a follow-up PR.

gottesmm · 2017-09-12T19:04:22Z

utils/build_swift/driver_arguments.py

+                    'Swift host tools, target Swift standard libraries, LLDB '
+                    '(if enabled) (default)')
+
+        option(['-r', '--release-debug'],


Can you keep this as --release-debuginfo. Otherwise, it is ambiguous against --debug.

gottesmm · 2017-09-12T19:05:42Z

utils/build_swift/driver_arguments.py

+    option('--no-legacy-impl', disable('lecacy_impl'), default=True, nargs=0,
+           help='avoid legacy build-script-impl implementation')
+
+    option('--export-compile-commands', enable, default=False, nargs=0,


The default for this should be true. It is not expensive and can be useful for tooling purposes.

For the purpose of this PR I am hesitant to change any default values since I'm already replacing the whole argument parser. I'd rather hold off on reworking the defaults to be more sensible until we have this merged.

I thought the default was to have this enabled.

I can double check as I make a cleanup pass over this again to make sure all the defaults are preserved (in combination with the new unit-tests).

gottesmm · 2017-09-12T19:07:08Z

utils/build_swift/driver_arguments.py

+    with mutually_exclusive_group():
+        option('--debug-swift',
+               set_('swift_build_variant', const='Debug'),
+               help='build the Debug variant of Swift')


Can you make it clear that this only makes the swift compiler, not the stdlib in debug?

gottesmm · 2017-09-12T19:07:47Z

utils/build_swift/driver_arguments.py

+               help='CMake build variant for Swift')
+
+    with mutually_exclusive_group():
+        option('--debug-swift-stdlib',


Can you make it clearer here that "the swift compiler" is not controlled by this option?

gottesmm · 2017-09-12T19:08:05Z

utils/build_swift/driver_arguments.py

+
+    option('--stdlib-deployment-targets', append(separator=' '),
+           metavar='TARGET',
+           help='space-separated ist of targets to compile or cross-compile '


"ist" => "list"

gottesmm · 2017-09-12T19:08:32Z

utils/build_swift/driver_arguments.py

+    option('--stdlib-deployment-targets', append(separator=' '),
+           metavar='TARGET',
+           help='space-separated ist of targets to compile or cross-compile '
+                'the Swift standard library for. %(default)s by default')


Whats up with the format string here?

The magical %(default)s format string inputs the default value when the argument parser outputs the help message.

gottesmm · 2017-09-12T19:10:11Z

utils/build_swift/driver_arguments.py

+
+    with mutually_exclusive_group():
+        option('--build-llvm',
+               enable('build_llvm'),


Just to double check (I didn't look), did you preserve the behavior where --skip-build-llvm builds enough of LLVM to compile Swift archives? (I think it tablegens some things and produces a few utilities, but thats it).

gottesmm · 2017-09-12T20:28:08Z

I am going to do another pass over this after Ross pings me. He said he is going to make some other changes.

…larative DSL.

…ParserBuilder DSL based on Rintaro's work last year.

…s and fixed one failing test for stdlib-deployment-targets which I couldn't preserve the old behavior with 1:1.

…type for the time being.

swift-ci · 2017-10-25T07:45:24Z

Build failed
Swift Test Linux Platform
Git Sha - 5fb944c

Rostepher · 2017-10-25T07:47:52Z

@swift-ci please test

Rostepher · 2017-10-25T07:47:59Z

@swift-ci please smoke test

swift-ci · 2017-10-25T08:52:10Z

Build failed
Swift Test Linux Platform
Git Sha - 3d6883d

Rostepher · 2017-10-26T18:48:50Z

@swift-ci please smoke test

Rostepher · 2017-11-15T23:28:02Z

Closing in favor #12873 and a handful of follow-up PRs which should make reviews much easier.

Rostepher requested review from rintaro, zisko, bob-wilson and najacque September 9, 2017 00:42

Rostepher force-pushed the build-me-a-swift branch from c25def9 to d3e2fd5 Compare September 9, 2017 00:48

Rostepher mentioned this pull request Sep 9, 2017

Disable benchmark builds by default in build-script #11717

Closed

Rostepher force-pushed the build-me-a-swift branch from d3e2fd5 to e38dc1f Compare September 9, 2017 05:13

Rostepher mentioned this pull request Sep 12, 2017

Move legacy argparser #11872

Merged

gottesmm reviewed Sep 12, 2017

View reviewed changes

Rostepher force-pushed the build-me-a-swift branch 3 times, most recently from 22d2836 to 27783ab Compare September 14, 2017 00:20

This was referenced Sep 14, 2017

New defaults module for build_swift argument parser #11911

Merged

Affirmative names for argument parsing namespace destinations. #11952

Merged

Rostepher added 4 commits October 24, 2017 22:14

Added defaults for a few hard-coded android values.

ad4ae6f

Implemented a builder class for creating argument parsers using a dec…

eefec0c

…larative DSL.

Re-implemented the main driver argument parser using the new Argument…

80485ad

…ParserBuilder DSL based on Rintaro's work last year.

Updated the argument-parsing test suite to handle the new parser clas…

5fb944c

…s and fixed one failing test for stdlib-deployment-targets which I couldn't preserve the old behavior with 1:1.

Rostepher force-pushed the build-me-a-swift branch from 27783ab to 5fb944c Compare October 25, 2017 05:19

Converted Clang and Swift compiler options to legacy copiler version …

3d6883d

…type for the time being.

swiftlang deleted a comment from swift-ci Oct 25, 2017

Rostepher removed the request for review from bob-wilson October 25, 2017 19:10

Rostepher mentioned this pull request Nov 11, 2017

Argparse "Overlay" Module #12873

Merged

Rostepher closed this Nov 15, 2017

Rostepher deleted the build-me-a-swift branch November 15, 2017 23:28

[WIP] SR-237 Refactor build-script argument parsing logic into separate module #11827

[WIP] SR-237 Refactor build-script argument parsing logic into separate module #11827

Uh oh!

Conversation

Rostepher commented Sep 9, 2017 • edited by najacque Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Purpose

Uh oh!

Rostepher commented Sep 9, 2017

Uh oh!

gottesmm left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gottesmm commented Sep 12, 2017

Uh oh!

swift-ci commented Oct 25, 2017

Uh oh!

Rostepher commented Oct 25, 2017

Uh oh!

Rostepher commented Oct 25, 2017

Uh oh!

swift-ci commented Oct 25, 2017

Uh oh!

Rostepher commented Oct 26, 2017

Uh oh!

Rostepher commented Nov 15, 2017

Uh oh!

Uh oh!

[WIP] SR-237 Refactor `build-script` argument parsing logic into separate module #11827

[WIP] SR-237 Refactor `build-script` argument parsing logic into separate module #11827

Rostepher commented Sep 9, 2017 •

edited by najacque

Loading