[build-script] Presets Module #14422

Rostepher · 2018-02-06T01:28:26Z

Purpose

This PR implements a new presets module with a more robust and easy to understand presets parser compared to the swift_build_support implementation in SwiftBuildSupport.py. The --swift-sdks migration code has also been moved to a new migration module that will house functionality for eventually migrating away from build-script-impl.

There's testing for all the new functionality, both the presets and migration modules have associated test suites.

This furthers the efforts for completing SR-237
rdar://25281853

… understand parser. Moved the swift-sdks migration code to a new migration module and added testing for both the presets and migration modules. Also converted build-script to use the new presets parser.

Rostepher · 2018-02-06T01:28:38Z

@swift-ci please test

Rostepher · 2018-02-06T04:45:55Z

On second thought, I can add testing to compare the results of the previous preset-parsing code to the new PresetParser if that would give everyone more confidence.

rintaro · 2018-02-06T07:41:03Z

utils/build-script

            os.path.join(
                SWIFT_SOURCE_ROOT, SWIFT_REPO_NAME, "utils",
                "build-presets.ini")
        ]

+        user_presets_file = os.path.join(HOME, '.swift-build-presets')
+        if os.path.isfile(user_presets_file):
+            args.preset_file_names.append(user_presets_file)


This is technically a behavior change.
If the both files has presets in a same name, ConfigParser merges those sections. e.g.

# preset1.ini [section] opt1 = True opt2 = foo

# preset2.ini [section] opt1 = False opt3 = bar

parser.read(['preset1.ini', 'preset2.ini']) will be like:

{ "section": { "opt1": "False", // NOTE: preset2.ini overwrites this "opt2": "foo", "opt3": "bar" } }

I'm not convinced this is a behavior change. The old presets parsing code could read in more than one file at a time as well (namely the _load_preset_files_impl function).

All I've done here is add checking to ensure that the user presets file (~/.swift-build-presets) exists before trying to parse it and failing. The previous implementation would only fail if all the presets files failed to parse, which ignored the failure for a non-existing user presets file. My implementation is more strict and will throw an exception for any missing files.

I meant the order in the preset_file_names might affect the result of the parsing.
In your implementation, the order is ['utils/build-presets.ini', '~/.swift-build-presets'], whereas the current master implementation is ['~/.swift-build-presets', 'utils/build-presets.ini'].
When ~/.swift-build-presets declares an option with the same name in the same section name in utils/build-presets.ini, in the current implementation, it will be ignored because it will be overwritten by the latter file.

To keep the current behavior, the code should insert ~/.swift-build-presets at the top of the list.

if os.path.isfile(user_presets_file): args.preset_file_names.insert(0, user_presets_file)

That's said, I think your implementation is OK because user defined (~/.swift-build-presets) value should be able to override the default value.

I would rather keep this functionality change (however slight) since you make a good point. User presets should be able to override the built-in ones.

I think we should enforce that every file should have unique presets. It could save someone from a really bad day, and is there any real win with the use case of overriding presets?

rintaro · 2018-02-06T07:57:14Z

utils/build_swift/presets.py

+        self._presets = {}
+
+    def _parse_raw_preset(self, section):
+        preset_name = _remove_prefix(section, PresetParser._PRESET_PREFIX)


SwiftBuildSupport.py used to ignore non-prefixed sections.
Are we going to make preset: optional?

For the time-being we are going to maintain feature parity with the old implementation. In the future we might consider changing the format some.

rintaro · 2018-02-06T15:21:21Z

utils/build_swift/presets.py

+        for mixin_name in raw_preset.mixins:
+            mixin = self._get_preset(mixin_name)
+
+            args += mixin.args


[preset: base] opt1 = 0 opt2 = 0 [preset: derived] opt1 = 1 mixin-preset = base opt2 = 1

should emit --opt1=1 --opt1=0 --opt2=0 --opt2=1 so the derived preset can override the options.

This was an oversight on my part. I didn't realize that it was possible to have mixins expanded in-place. I had only ever seen them declared at the beginning of a preset. I'll modify my parser to preserve this behavior and add some testing for it. Thanks!

…nown feature) and also changed the parser to skip all non-preset sections. Tests are included for these two behaviors.

Rostepher · 2018-02-06T22:56:09Z

@swift-ci please test

swift-ci · 2018-02-06T22:56:34Z

Build failed
Swift Test OS X Platform
Git Sha - 4342207

swift-ci · 2018-02-06T22:57:58Z

Build failed
Swift Test Linux Platform
Git Sha - 4342207

Rostepher · 2018-02-07T01:09:46Z

@swift-ci please test Linux

shahmishal · 2018-02-07T04:58:47Z

@swift-ci Build Toolchain

Rostepher · 2018-02-07T19:49:56Z

@swift-ci please Build Toolchain macOS

swift-ci · 2018-02-07T21:18:28Z

macOS Toolchain
Download Toolchain
Git Sha - 009b6aa

Install command
tar -zxf swift-PR-14422-15-osx.tar.gz --directory ~/

Rostepher · 2018-02-08T23:59:37Z

@swift-ci please test

Rostepher · 2018-02-08T23:59:48Z

@swift-ci please build toolchain

swift-ci · 2018-02-08T23:59:57Z

Build failed
Swift Test Linux Platform
Git Sha - 009b6aa

swift-ci · 2018-02-09T00:00:04Z

Build failed
Swift Test OS X Platform
Git Sha - 009b6aa

Rostepher · 2018-02-09T00:33:32Z

@swift-ci please test

Rostepher · 2018-02-09T00:33:41Z

@swift-ci please build toolchain

swift-ci · 2018-02-09T00:33:55Z

Build failed
Swift Test OS X Platform
Git Sha - 74909c4fb1903e281e157b998740570b0f3a3eef

swift-ci · 2018-02-09T00:35:02Z

Build failed
Swift Test Linux Platform
Git Sha - 74909c4fb1903e281e157b998740570b0f3a3eef

Rostepher · 2018-02-09T00:45:11Z

@swift-ci please test

Rostepher · 2018-02-09T00:45:20Z

@swift-ci please build toolchain

swift-ci · 2018-02-09T00:45:36Z

Build failed
Swift Test Linux Platform
Git Sha - e99dc545a9c046df387d50d551bd825c8ae02828

swift-ci · 2018-02-09T00:45:44Z

Build failed
Swift Test OS X Platform
Git Sha - e99dc545a9c046df387d50d551bd825c8ae02828

Rostepher · 2018-02-09T00:50:53Z

@swift-ci please test

Rostepher · 2018-02-09T00:51:00Z

@swift-ci please build toolchain

swift-ci · 2018-02-09T00:52:10Z

Build failed
Swift Test Linux Platform
Git Sha - 95e70c22241d157dc5d8fe897f88467dad15c629

swift-ci · 2018-02-09T00:52:23Z

Build failed
Swift Test OS X Platform
Git Sha - 95e70c22241d157dc5d8fe897f88467dad15c629

Rostepher · 2018-02-09T01:46:47Z

@swift-ci please build toolchain macOS

swift-ci · 2018-02-09T02:16:35Z

macOS Toolchain
Download Toolchain
Git Sha - 6793553acc9570a28a233a1d2e7fc213ef71ef92

Install command
tar -zxf swift-PR-14422-26-osx.tar.gz --directory ~/

…formation packed exception classes. Also re-worked the PresetParser to catch duplicate preset declarations and duplicate options in a single preset. There's some special shim-code to handle the disconnect between the Python 2 ConfigParser module and the Python 3 update which adds DuplicateOptionError.

Rostepher · 2018-02-10T01:42:35Z

@swift-ci please test

swift-ci · 2018-02-10T01:42:58Z

Build failed
Swift Test OS X Platform
Git Sha - 6793553acc9570a28a233a1d2e7fc213ef71ef92

swift-ci · 2018-02-10T01:44:01Z

Build failed
Swift Test Linux Platform
Git Sha - 6793553acc9570a28a233a1d2e7fc213ef71ef92

zisko

LGTM.

zisko · 2018-02-08T21:58:21Z

utils/build-script

            os.path.join(
                SWIFT_SOURCE_ROOT, SWIFT_REPO_NAME, "utils",
                "build-presets.ini")
        ]

+        user_presets_file = os.path.join(HOME, '.swift-build-presets')
+        if os.path.isfile(user_presets_file):
+            args.preset_file_names.append(user_presets_file)


I think we should enforce that every file should have unique presets. It could save someone from a really bad day, and is there any real win with the use case of overriding presets?

Rostepher · 2018-02-26T19:26:12Z

@zisko Ultimately I would like to enforce uniqueness, but at the moment I don't want to modify functionality. There's also some technical limitations with the Python 2 ConfigParser module (which is substantially different from the Python 3 configparser) that will make it more difficult to implement. We'd need to have a separate ConfigParser instance per file read and then validate each preset is not repeated, but in Python 2 it's not possible to validate that sections in the same file (i.e. presets) are unique.

* Implemented a presets module which includes a more robust and easy to understand parser. Moved the swift-sdks migration code to a new migration module and added testing for both the presets and migration modules. Also converted build-script to use the new presets parser. * Switched the expansion algorithm to resolve mixins in-place (little known feature) and also changed the parser to skip all non-preset sections. Tests are included for these two behaviors. * Re-worked the presets error hierarchy to have more descriptive and information packed exception classes. Also re-worked the PresetParser to catch duplicate preset declarations and duplicate options in a single preset. There's some special shim-code to handle the disconnect between the Python 2 ConfigParser module and the Python 3 update which adds DuplicateOptionError.

Implemented a presets module which includes a more robust and easy to…

4342207

… understand parser. Moved the swift-sdks migration code to a new migration module and added testing for both the presets and migration modules. Also converted build-script to use the new presets parser.

Rostepher self-assigned this Feb 6, 2018

Rostepher added the enhancement label Feb 6, 2018

Rostepher requested review from zisko and rintaro February 6, 2018 04:42

rintaro reviewed Feb 6, 2018

View reviewed changes

Switched the expansion algorithm to resolve mixins in-place (little k…

009b6aa

…nown feature) and also changed the parser to skip all non-preset sections. Tests are included for these two behaviors.

Rostepher requested a review from shahmishal February 7, 2018 23:01

Rostepher mentioned this pull request Feb 8, 2018

[build-script] Cleanup arg-parsing tests #14488

Closed

Rostepher force-pushed the presets-module branch from 74909c4 to e99dc54 Compare February 9, 2018 00:33

Rostepher force-pushed the presets-module branch from e99dc54 to 95e70c2 Compare February 9, 2018 00:45

Rostepher force-pushed the presets-module branch from 95e70c2 to 6793553 Compare February 9, 2018 00:50

Rostepher force-pushed the presets-module branch from 6793553 to 0113466 Compare February 10, 2018 01:42

zisko approved these changes Feb 26, 2018

View reviewed changes

Rostepher merged commit 4edb8d9 into swiftlang:master Feb 26, 2018

Rostepher mentioned this pull request Feb 26, 2018

[build-script] Presets module 5.0 #14834

Merged

Rostepher deleted the presets-module branch January 19, 2020 20:31

AnthonyLatsis added build-script Area → utils: The build script improvement and removed enhancement labels Nov 13, 2022

[build-script] Presets Module #14422

[build-script] Presets Module #14422

Uh oh!

Conversation

Rostepher commented Feb 6, 2018

Purpose

Uh oh!

Rostepher commented Feb 6, 2018

Uh oh!

Rostepher commented Feb 6, 2018

Uh oh!

rintaro Feb 6, 2018 • 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!

Rostepher Feb 6, 2018 • 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!

rintaro Feb 6, 2018 • 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!

Rostepher Feb 7, 2018

Choose a reason for hiding this comment

Uh oh!

zisko Feb 8, 2018

Choose a reason for hiding this comment

Uh oh!

rintaro Feb 6, 2018

Choose a reason for hiding this comment

Uh oh!

Rostepher Feb 6, 2018

Choose a reason for hiding this comment

Uh oh!

rintaro Feb 6, 2018

Choose a reason for hiding this comment

Uh oh!

Rostepher Feb 6, 2018

Choose a reason for hiding this comment

Uh oh!

Rostepher commented Feb 6, 2018

Uh oh!

swift-ci commented Feb 6, 2018

Uh oh!

swift-ci commented Feb 6, 2018

Uh oh!

Rostepher commented Feb 7, 2018

Uh oh!

shahmishal commented Feb 7, 2018

Uh oh!

Rostepher commented Feb 7, 2018

Uh oh!

swift-ci commented Feb 7, 2018

Uh oh!

Rostepher commented Feb 8, 2018

Uh oh!

Rostepher commented Feb 8, 2018

Uh oh!

swift-ci commented Feb 8, 2018

Uh oh!

swift-ci commented Feb 9, 2018

Uh oh!

Rostepher commented Feb 9, 2018

Uh oh!

Rostepher commented Feb 9, 2018

Uh oh!

swift-ci commented Feb 9, 2018

Uh oh!

swift-ci commented Feb 9, 2018

Uh oh!

Rostepher commented Feb 9, 2018

Uh oh!

Rostepher commented Feb 9, 2018

Uh oh!

swift-ci commented Feb 9, 2018

Uh oh!

swift-ci commented Feb 9, 2018

Uh oh!

Rostepher commented Feb 9, 2018

rintaro Feb 6, 2018 •

edited

Loading

Rostepher Feb 6, 2018 •

edited

Loading

rintaro Feb 6, 2018 •

edited

Loading