Fix flipped argument state #12005

Rostepher · 2017-09-19T19:51:48Z

Purpose

This PR resolves issue SR-5911. My previous PR #11952 changed the internal 'destinations' for all the --skip-build-* and --skip-test-* flags from the form skip_build_* and skip_test_* to the affirmative build_* and test_*. When swapping those states I failed to account for the more advanced optional boolean behavior on each of these skip flags. It is possible for many flags to accept an optional boolean state to toggle them on or off manually, with skip flags it can be rather confusing since --skip-build-benchmarks True means that we should not build benchmarks. Before this fix the optional boolean was being directly set on these new inverted destinations, thus --skip-build-benchmarks True was setting the destination build_benchmarks to True rather than the expected False.

In addition to fixing the erroneous behavior I've also taken the liberty to revamp the testing suite again making a more clear distinction between flags that accept optional booleans, those that only toggle the state True or False and those that store constant values. The test suite now covers all these cases and has more strict testing for their edge cases.

A small note for reviewers, there's a lot of noise in the testing suite dealing with parsing arguments. To accurately test that each flag accepts the expected argument values and acts accordingly it was necessary to be able to parse flags without applying the defaults. Thus there's now the methods parse_args and parse_default_args on the main test case class.

rdar://34526897

Rostepher · 2017-09-19T21:08:08Z

@gottesmm Would you mind giving this a quick once-over before I merge?

Rostepher · 2017-09-19T21:09:19Z

utils/build_swift/driver_arguments.py

@@ -194,6 +194,7 @@ def _apply_default_arguments(args):

    # If none of tests specified skip swift stdlib test on all platforms
    if not args.test and not args.validation_test and not args.long_test:
+        print('not testing')


Whoops, left in a print statement. I'll remove that.

gottesmm · 2017-09-19T21:35:32Z

@Rostepher give me a little bit. I am in some meetings today = (.

Rostepher · 2017-09-19T21:41:12Z

@gottesmm No problem, just let me know when you've had a chance to look at it. Thanks!

Rostepher · 2017-09-19T23:54:31Z

@swift-ci please smoke test

gottesmm

A few remarks. Also can you split out the sorting/etc into a different commit. This should be just about fixing the specific bug. If you want to change the order/layout of things please do that in a different commit so that I can review only the important things via the diff.

gottesmm · 2017-09-19T23:50:38Z

utils/build_swift/tests/expected_options.py

+    if parsed and defaulting to False otherwise.
+    """
+
+    pass


Question. Is pass supposed to be on this line according to flake8? I thought it was supposed to be right after the """

gottesmm · 2017-09-19T23:58:37Z

utils/build_swift/tests/expected_options.py

@@ -260,7 +293,7 @@ class UnsupportedOption(_BaseOption):
    """Option that is not supported."""

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


I still hate this. Can you just do this:

if 'dest' not in kwargs: kwargs['dest'] = None

gottesmm · 2017-09-19T23:58:58Z

utils/build_swift/tests/expected_options.py

@@ -270,7 +303,9 @@ class IgnoreOption(_BaseOption):
    generated.
    """

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


gottesmm · 2017-09-20T00:01:13Z

utils/build_swift/tests/expected_options.py

+    pass
+
+
+class EnableOption(_BaseOption):


Can you leave any shuffling to a different commit. This is going to be difficult to review.

gottesmm · 2017-09-20T00:02:53Z

With my review, I stopped due to the moving around of code. Please fix that and then I will relook.

shahmishal · 2017-09-26T19:06:30Z

@swift-ci test

swift-ci · 2017-09-26T19:06:46Z

Build failed
Swift Test Linux Platform
Git Sha - 66ce0dae13ac721b6d6d2af0bb31c8a7c9ab1c90

swift-ci · 2017-09-26T19:10:52Z

Build failed
Swift Test OS X Platform
Git Sha - 66ce0dae13ac721b6d6d2af0bb31c8a7c9ab1c90

shahmishal · 2017-09-26T19:14:27Z

@swift-ci test Linux

swift-ci · 2017-09-26T19:16:42Z

Build failed
Swift Test Linux Platform
Git Sha - 9b672e8516b0438d09b655498d429b6ceebd3fa4

Rostepher · 2017-09-26T21:56:10Z

@swift-ci please smoke test

Rostepher · 2017-09-26T21:56:18Z

@swift-ci please test

…d Disable variants, properly handling optional boolean arguments and inverting the state for disabled options. Previously when an option such as '--skip-build-benchmarks' was passed with a boolean the destination would take on the value without inverting the state, which was causing some build configuration errors.

Rostepher · 2017-09-26T23:22:42Z

@swift-ci please smoke test

Rostepher · 2017-09-27T18:08:38Z

@gottesmm Would you mind taking a look over this fix again. I've removed the test changes for now. I'll add those in a follow-up PR, I'd rather get this merged first to avoid any unexpected regressions.

lplarson · 2017-09-29T18:32:41Z

LGTM

Rostepher added bug A deviation from expected or documented behavior. Also: expected but undesirable behavior. enhancement labels Sep 19, 2017

Rostepher requested review from gottesmm, bob-wilson and najacque September 19, 2017 19:51

Rostepher changed the title ~~Updated argument parsing code to use a new OnOffAction with Enable an…~~ Fix flipped argument state Sep 19, 2017

Rostepher force-pushed the fix-flipped-argument-state branch from 66ce0da to 56c3d8d Compare September 19, 2017 20:21

Rostepher commented Sep 19, 2017

View reviewed changes

gottesmm requested changes Sep 20, 2017

View reviewed changes

Rostepher force-pushed the fix-flipped-argument-state branch from 710a5a2 to 9b672e8 Compare September 25, 2017 02:42

Rostepher force-pushed the fix-flipped-argument-state branch from 9b672e8 to 3d33634 Compare September 26, 2017 21:35

swiftlang deleted a comment from swift-ci Sep 26, 2017

Rostepher force-pushed the fix-flipped-argument-state branch from 3d33634 to edb4daf Compare September 26, 2017 23:22

Rostepher merged commit d1620f5 into swiftlang:master Sep 29, 2017

Rostepher deleted the fix-flipped-argument-state branch September 29, 2017 19:06

swift-ci mentioned this pull request Sep 19, 2017

[SR-5911] utils/build-script regression #48470

Closed

AnthonyLatsis added bug fix improvement and removed bug A deviation from expected or documented behavior. Also: expected but undesirable behavior. enhancement labels Nov 13, 2022

AnthonyLatsis added bug A deviation from expected or documented behavior. Also: expected but undesirable behavior. and removed bug fix labels Apr 21, 2023

Fix flipped argument state #12005

Fix flipped argument state #12005

Uh oh!

Conversation

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

Uh oh!

Purpose

Uh oh!

Rostepher commented Sep 19, 2017

Uh oh!

Rostepher Sep 19, 2017

Choose a reason for hiding this comment

Uh oh!

gottesmm commented Sep 19, 2017

Uh oh!

Rostepher commented Sep 19, 2017

Uh oh!

Rostepher commented Sep 19, 2017

Uh oh!

gottesmm left a comment

Choose a reason for hiding this comment

Uh oh!

gottesmm Sep 19, 2017

Choose a reason for hiding this comment

Uh oh!

gottesmm Sep 19, 2017

Choose a reason for hiding this comment

Uh oh!

gottesmm Sep 19, 2017

Choose a reason for hiding this comment

Uh oh!

gottesmm Sep 20, 2017

Choose a reason for hiding this comment

Uh oh!

gottesmm commented Sep 20, 2017

Uh oh!

shahmishal commented Sep 26, 2017

Uh oh!

swift-ci commented Sep 26, 2017

Uh oh!

swift-ci commented Sep 26, 2017

Uh oh!

shahmishal commented Sep 26, 2017

Uh oh!

swift-ci commented Sep 26, 2017

Uh oh!

Rostepher commented Sep 26, 2017

Uh oh!

Rostepher commented Sep 26, 2017

Uh oh!

Rostepher commented Sep 26, 2017

Uh oh!

Rostepher commented Sep 27, 2017

Uh oh!

lplarson commented Sep 29, 2017

Uh oh!

Uh oh!

Rostepher commented Sep 19, 2017 •

edited

Loading