[build-script] Move HostSpecificConfiguration out of main script. #23810
Conversation
This will allow using HostSpecificConfiguration from other parts that are not the main script in the future. Additionally, a lot of code that was part of BuildScriptInvocation has been moved into HostSpeficiaConfiguration because nobody else needed it, and HostSpecificConfiguration was highly coupled with the Invocation object (through one of its arguments). This should make the Configuration object a better citizen in other scenarios.
|
@swift-ci please test |
|
Question. Is this just a pure move (ignoring the tests?) |
There was a problem hiding this comment.
@gottesmm: I though Github will draw markings in the gutter to show code movements, but it seems it doesn’t.
The code is mostly unchanged. I have added comments to the main changes I can see, explaining why I did the changes. If there are more changes, I hope they are to please the linter, or minor enough, but if you find one I haven’t commented about, don’t hesitate to ask.
|
|
||
| """Configuration information for an individual host.""" | ||
|
|
||
| def __init__(self, args, host_target): |
There was a problem hiding this comment.
Change: Previously HostSpecificConfiguration was receiving an invocation parameter. Through this parameter, args were accessed, and also calculated values like platforms_to_skip_build, platforms_to_skip_test, platforms_archs_to_skip_test and platforms_to_skip_test_host. Those calculations were only used by HostSpecificConfiguration, but were done by BuildScriptInvocation, and only relied on `args.
The solution is passing args directly into HostSpecificConfiguration and calculate the values from args by itself.
| platforms_to_skip_test = self.__platforms_to_skip_test(args) | ||
| platforms_archs_to_skip_test = \ | ||
| self.__platforms_archs_to_skip_test(args) | ||
| platforms_to_skip_test_host = self.__platforms_to_skip_test_host(args) |
There was a problem hiding this comment.
Instead of calculating the values of those variables inline in the method (like it happens in the previous build-script L320-440), the calculation is done by smaller methods that only deal with one of the variables.
| "check-swift{}-optimize_none_implicit_dynamic-{}" | ||
| .format(subset_suffix, name)) | ||
|
|
||
| def __platforms_to_skip_build(self, args): |
There was a problem hiding this comment.
This method is taken from the previous build-script L327-351. The only change is removing self in front of every platforms_to_skip_build.
| platforms_to_skip_build.add(StdlibDeploymentTarget.Android) | ||
| return platforms_to_skip_build | ||
|
|
||
| def __platforms_to_skip_test(self, args): |
There was a problem hiding this comment.
This method is taken from the previous build-script L355-391. As before, the self. is removed. In the previous code the use of test_ios_32bit_simulator to update platforms_archs_to_skip_test is in the middle of this checks, but in the new version it has moved to its own method to keep calculations clearer.
| if not args.test_ios_host: | ||
| platforms_to_skip_test.add(StdlibDeploymentTarget.iOS) | ||
| else: | ||
| raise ArgumentError(None, |
There was a problem hiding this comment.
Here and in L214 and L222, the original code was calling directly to exit_rejecting_arguments. Since that function is not visible from this module, and I didn’t want to exit directly, the method raises exceptions that can be processed by the user of the code. The build script will do exit_rejecting_arguments in response to those exceptions.
|
|
||
| return platforms_to_skip_test | ||
|
|
||
| def __platforms_archs_to_skip_test(self, args): |
There was a problem hiding this comment.
This method is taken from build-script L371-L373. See my comment above for more information.
| StdlibDeploymentTarget.iOSSimulator.i386) | ||
| return platforms_archs_to_skip_test | ||
|
|
||
| def __platforms_to_skip_test_host(self, args): |
There was a problem hiding this comment.
This method is taken from build-script L394-404. As before, the self. is removed.
| try: | ||
| config = HostSpecificConfiguration(self.args, host_target.name) | ||
| except argparse.ArgumentError as e: | ||
| exit_rejecting_arguments(e.message) |
There was a problem hiding this comment.
Here and in the previous code change: BuildScriptInvocation was calling a function exit_rejecting_arguments in the middle of the calculation of platforms_to_skip_test. Since those variables are only used in HostSpecificConfiguration, I moved them there, and to reproduce the previous behaviour, if HostSpecificConfiguration finds some problem with the passed arguments, it will raise ArgumentError, which will be dealt here. That allows the users of HostSpecificConfiguration decide how to act in the case of an error.
|
I agree that the changes are pretty minor. The problem is entirely in how this diff is rendered. The only difference is that there was a refactoring that moved two inline "functions" into separate methods. But, its mostly just code motion. The changes introduced aid in readability and structure. It looks NFC to me. |
|
@drodriguez next time, when you have a patch with a lot of refactoring/moving, please do it in a different commit to ease reviewing (I am fine if it is in the same PR). It makes it so that I can easily eyeball those parts of the PR. The less I have to think, the less time it takes me to review your PR and the better job I can do. |
|
LGTM. Next time, please split this up. Moving code around like you did here makes it hard for me to review. |
|
*without splitting it into its own commit. |
|
LGTM |
This will allow using
HostSpecificConfigurationfrom other parts thatare not the main script in the future. This is interesting because the information is mostly useful when building Swift. The rest of the products are not really interested in the results of this calculations.
Additionally, a lot of code that was part of
BuildScriptInvocationhasbeen moved into
HostSpecificConfigurationbecause nobody else needed it,and
HostSpecificConfigurationwas highly coupled with the Invocationobject (through one of its arguments). This should make the
HostSpecificConfigurationobject a better citizen in other scenarios.Includes a suite of tests that check the implementation correctly calculates the right targets to build under diverse circumstances.
This was part of #23257. I’m trying to split the PR in smaller ones to make the reviews easier. Other PRs in this group are #23303, and #23803.
/cc @compnerd