Move legacy argparser #11872

Rostepher · 2017-09-12T05:17:22Z

Purpose

This PR is part of the staging necessary for #11827 and builds on the staging from #11880 and #11882. Put simply this PR bundles up the current argument parser and the _apply_default_arguments function into an ApplyDefaultsArgumentParser class that will call _apply_default_arguments anytime the parse_args method is called. The new parser bundles up these two interconnected behaviors making it easier to test the expected outputs.

In addition I've also taken the time to implement a new suite of unit-tests to ensure the expected behavior for the argument parser. These tests should make it easier to transition to the new system in #11827, giving reviewers more confidence that it actually works.

The unit-test suite (ab)uses Python meta classes to dynamically generate tests for each preset and argument, ensuring that they all have coverage. There's still a handful of tests types left to implement for the arguments and these tests have uncovered a preset that is broken (which will either be removed or fixed).

rdar://problem/34336890

bob-wilson · 2017-09-12T06:14:08Z

You mentioned a preset that is broken. Can you elaborate?

bob-wilson · 2017-09-12T06:15:03Z

@gottesmm Do you have any opinion about splitting out a build_swift package like this?

Rostepher · 2017-09-12T19:45:02Z

My new test suite uncovered an unsupported flag in the buildbot_iphoneos_arm64_crosscompiler preset, namely the --build-swift-ios-test option. Looks like the preset was added this year, but from my searching that option has never been available. My intuition says that the preset is not used commonly or we would have seen this crop up earlier.

Rostepher · 2017-09-12T19:48:57Z

As for splitting out to a new build_swift package I was planning to make that the defacto home for the Python sources to build-script. We do already have the swift_build_support package, but I would prefer to keep them separate and eventually phase out that package which is structured awkwardly. Any functionality necessary from swift_build_support will eventually be migrated over to the new build_swift.

I'm open to other suggestions or names for the module, I landed on build_swift because it was short and I think more appropriate than a module named build_script.

gottesmm · 2017-09-12T20:09:22Z

I don't have a problem with the refactoring. I would prefer the name swift_build though. We already have precedent in that area with llvm_build and swift_build_support. But let me look at this in more depth.

gottesmm · 2017-09-12T20:31:03Z

utils/build_swift/legacy_driver_arguments.py

+]
+
+
+class ApplyDefaultsArgumentParser(argparse.ArgumentParser):


Just to loop back from our conversation, can you do this in a separate commit. I want to be able to eyeball the move of code and then see the refactoring done upon the nicely refactored code. It will make it easier for me to review.

Sure thing! I'll chunk out the two large code-moves into separate PRs.

Thanks! = )

Rostepher · 2017-09-12T21:13:02Z

Alright, in order we will need to merge #11880 and then #11882 before this one can be merged. Until then the diff for this PR will look much larger since this branch has those changes in it.

Rostepher · 2017-09-14T00:28:37Z

@gottesmm Would you mind taking a look over this PR again? Feel free to ignore the defaults module, that commit will be merged in #11911. The unit-tests and _ApplyDefaultsArgumentParser are what's important here.

Rostepher · 2017-09-14T00:34:41Z

@swift-ci please smoke test

…er (ab)using metaclasses to dynamically generate unit-tests for each valid argument and preset.

Rostepher · 2017-09-14T19:45:46Z

@swift-ci please smoke test

gottesmm

This looks fine to me.

gottesmm · 2017-09-14T21:47:07Z

utils/build_swift/tests/expected_options.py

+    """Option that is not supported."""
+
+    def __init__(self, *args, **kwargs):
+        kwargs['dest'] = kwargs.get('dest')


What are you trying to do here?

That line facilitates allowing instances of UnsupportedOption to skip passing in the dest argument since it doesn't seem necessary for that sort of option. I would be better written as kwards['dest'] = kwargs.pop('dest', None) which pops dest off the input keyword args or resolves to None and sets the value manually when passed into the superclass constructor.

I had originally tried to add dest as positional argument in the UnsupportedOption init function, but I was running into issues with dest being passed twice to the superclass constructor. This was the best way I found after some searching to achieve the desired effect.

gottesmm · 2017-09-14T21:47:37Z

utils/build_swift/tests/expected_options.py

+
+EXPECTED_OPTIONS = [
+    # Ignore the help options since they always call sys.exit(0)
+    _BaseOption('-h', dest='help', default=argparse.SUPPRESS),


Why not make a special thing for help rather than using _BaseOption.

That would probably clean this up a bit. I'll make that change.

gottesmm · 2017-09-14T21:48:05Z

utils/build_swift/tests/expected_options.py

+
+    # NOTE: We'll need to manually test the behavior of these since they
+    # validate compiler version strings.
+    _BaseOption('--clang-compiler-version',


Same here. Can you make a wrapper rather than using an _ class? Looks weird to me.

Rostepher · 2017-09-15T01:56:15Z

@swift-ci please smoke test

…ould give better output in case the default tests fail.

Rostepher · 2017-09-15T04:48:37Z

@swift-ci please smoke test

gottesmm

Some post commit review.

gottesmm · 2017-09-15T16:52:11Z

utils/build_swift/tests/expected_options.py

@@ -252,16 +260,25 @@ class UnsupportedOption(_BaseOption):
    """Option that is not supported."""

    def __init__(self, *args, **kwargs):
-        kwargs['dest'] = kwargs.get('dest')
+        kwargs['dest'] = kwargs.pop('dest', None)


This is still really weird to me. What are you trying to do here?

I'm injecting my own definition for dest such that the UnsupportedOption class does not need to be instantiated with it manually passed. Here I'm passing through any manually set dest or setting it by default to None. In all cases it's defined in the kwargs and satisfies the _BaseOption constructor.

gottesmm · 2017-09-15T16:52:32Z

utils/build_swift/tests/test_driver_arguments.py

@@ -220,6 +227,8 @@ def generate_option_test(cls, option):
            return cls._generate_append_option_test(option)
        elif option.__class__ is expected_options.ChoicesOption:
            return cls._generate_choices_option_test(option)
+        elif option.__class__ is expected_options.HelpOption:
+            return cls._generate_help_option_test(option)


This can be done cleaner via a dictionary.

When you say 'via a dictionary' do you mean I should have a dictionary from class to function? I'm not sure I'm following along.

gottesmm · 2017-09-15T16:53:14Z

utils/build_swift/tests/test_driver_arguments.py

@@ -143,6 +143,13 @@ def test(self):
        return test

    @classmethod
+    def _generate_help_option_test(cls, option):
+        def test(self):


This doesn't do anything, no? You aren't invoking test. What is the purpose of this?

The meta-class here is used to generate unit-test cases dynamically. When Python is invoked with the unittest argument it will execute all methods on classes subclassing unittest.TestCase that begin with the word test. It's not required in any Python unittest code to invoke the test cases yourself.

On a second look you're right, I forgot to return the actual test here. I'll fix this in the next PR

Rostepher requested review from zisko, bob-wilson and najacque September 12, 2017 05:17

Rostepher added the enhancement label Sep 12, 2017

gottesmm requested changes Sep 12, 2017

View reviewed changes

This was referenced Sep 12, 2017

Move build-script argument parser to separate module #11880

Merged

Move build-script apply default arguments into separate module #11882

Merged

Rostepher force-pushed the move-legacy-argparser branch from 01a8e72 to a5d453f Compare September 12, 2017 21:08

Rostepher force-pushed the move-legacy-argparser branch 5 times, most recently from 2544c33 to 0c8ac1f Compare September 14, 2017 00:14

Rostepher mentioned this pull request Sep 14, 2017

New defaults module for build_swift argument parser #11911

Merged

Rostepher force-pushed the move-legacy-argparser branch 2 times, most recently from f82d4fc to bc4f87b Compare September 14, 2017 19:03

Implemented a (mostly) comprehensive test suite for the argument pars…

1749f98

…er (ab)using metaclasses to dynamically generate unit-tests for each valid argument and preset.

Rostepher force-pushed the move-legacy-argparser branch from bc4f87b to 1749f98 Compare September 14, 2017 19:44

gottesmm approved these changes Sep 14, 2017

View reviewed changes

Rostepher added 2 commits September 14, 2017 16:54

Fixed failing defaults test for build_ninja.

13890e4

Added new HelpOption and IgnoreOption classes for generating tests.

8820bcf

Converted default value test into individual generated tests which sh…

dc4f901

…ould give better output in case the default tests fail.

Rostepher merged commit 695ded2 into swiftlang:master Sep 15, 2017

Rostepher deleted the move-legacy-argparser branch September 15, 2017 08:05

gottesmm reviewed Sep 15, 2017

View reviewed changes

AnthonyLatsis added improvement and removed enhancement labels Nov 13, 2022

		]


		class ApplyDefaultsArgumentParser(argparse.ArgumentParser):

Move legacy argparser #11872

Move legacy argparser #11872

Uh oh!

Conversation

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

Uh oh!

Purpose

Uh oh!

bob-wilson commented Sep 12, 2017

Uh oh!

bob-wilson commented Sep 12, 2017

Uh oh!

Rostepher commented Sep 12, 2017

Uh oh!

Rostepher commented Sep 12, 2017

Uh oh!

gottesmm commented Sep 12, 2017

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!

Rostepher commented Sep 12, 2017

Uh oh!

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

Uh oh!

Uh oh!

Rostepher commented Sep 14, 2017

Uh oh!

Rostepher commented Sep 14, 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!

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

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!

Rostepher commented Sep 15, 2017

Uh oh!

Rostepher commented Sep 15, 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!

Uh oh!

Rostepher commented Sep 12, 2017 •

edited

Loading

Rostepher commented Sep 14, 2017 •

edited

Loading

Rostepher Sep 15, 2017 •

edited

Loading