[build-script] Determine HOST_CC in build-script #761

modocache · 2015-12-24T04:51:58Z

SR-237 calls for build-script and build-script-impl to be merged. This commit takes another step towards that goal by moving the logic that finds the path to the clang and clang++ executables up into Python-land.

Rather than simply moving all of the logic into utils/build-script, this commit moves relevant functions into a new Python module, named buildcli. This has several benefits:

The logic can be tested. Whereas build-script-impl needed to be run in order to verify its behavior, the logic extracted out of it into buildcli can be tested in isolation.
The logic can be split up into several files without polluting the utils directory, which now contains many different files that are unrelated to build-script.

/cc @gottesmm and @jrose-apple, since they've commented on SR-237 in the past.

gribozavr · 2015-12-24T04:57:38Z

utils/buildcli/buildcli/clang.py

+def _freebsd_release_date():
+    """
+    Return the release date for FreeBSD operating system on this host.
+    If the release date cannot be ascertained, return None.


I think it will return 0 instead, or sometimes throw an error, if sysctl returns a non-integer.

I'd just halt the execution at that point. There is no reasonable way a caller could recover from an error in this function. They either need the version and they call this function, or they don't, and they shouldn't be calling it.

gribozavr · 2015-12-24T05:16:20Z

@modocache Awesome improvements!

modocache · 2015-12-24T18:21:14Z

@gribozavr Addressed your comments, except for the one about adding the unit tests to validation-test. That change may end up being a little more involved, so I think it's best to send it in another PR.

gribozavr · 2015-12-24T20:52:05Z

utils/swift_build_support/README.md

@@ -0,0 +1,10 @@
+# builcli


Stale module name.

Good catch!

gribozavr · 2015-12-24T21:06:50Z

@modocache Thank you! Unfortunately, one more issue with argument handling came up in my testing...

modocache · 2015-12-24T21:17:01Z

@gribozavr Good point, I'll update utils/build-script as per your suggestions.

I also realized I forgot to add the code headers you mentioned! I added the ones from #762, since it looks like that was just merged. Thanks!

modocache · 2015-12-25T19:28:43Z

@gribozavr I've added some logic to the argument parser that enables the backwards-compatibility you described. I wanted to make sure we could test the new behavior in isolation, which meant moving the argument parser itself. Hopefully the commits aren't so big that they're unreviewable... 😟

Let me know if you have any additional thoughts--and thanks for all the reviews!

modocache · 2015-12-26T01:32:23Z

A proposal to add Python unit tests to the validation tests is up at #778.

gribozavr · 2015-12-26T15:12:02Z

@modocache Thanks! I think your patch will work, but... Sorry for sounding picky, but I think we can use a simpler approach. Subclassing of argparse.ArgumentParser seems strange to me (I don't feel like it was meant for subclassing), and _move_impl_args mutating the parsed arguments data structure after the fact seems error-prone (for example, because the information about the key in is duplicated).

What do you think about the following approach? Add a function in the module (so that it is testable) that accepts a command line and a list of options to migrate, and returns the rewritten command line, which is then passed to parse_args. Rewriting algorithm would be:

Split the argument list on --, to get two lists, one is the build-script arguments, another is the build-script-impl arguments.
Iterate over the build-script-impl argument list, and check if an option is marked for migration. If it is, append it to the build-script argument list verbatim.
Concatenate two lists with -- in between.

modocache · 2015-12-26T17:31:46Z

@gribozavr Updated as per your suggestions! 👍

I think I'd still like to move the argument parser out into its own module someday, though. We could probably discuss this on the mailing lists, in the comments on SR-237, or in another pull request. My reasoning is that it's pretty cool to be able to call parser.parse_args() from within a unit test to verify what values are set. Even with the test_migration.py tests from this pull request, I still had to manually call build-script--the unit tests showed the migrate_impl_args() function was working, but I couldn't be sure the argument parsing was the same as before.

Anyway, thanks for all the dedicated reviews!

gribozavr · 2015-12-26T23:48:48Z

@modocache Yes, that would be definitely useful -- I was thinking about a feature in build-script that would validate all presets, for example. I wouldn't mind if you move it to a module in this PR, but it is the subclassing that looks strange -- I could imagine a function that accepts a list of command line arguments and returns a parsed representation as the API that the module should expose for this purpose.

modocache · 2015-12-27T00:32:33Z

Ah, gotcha. Well, definitely expect more PRs on this topic, then! 😉

I feel like this one is in a pretty good state, though. I'm eager to merge it so I can add its tests to the validation tests from #778.

gribozavr · 2015-12-27T06:54:58Z

utils/swift_build_support/swift_build_support/clang.py

+    """
+    for suffix in suffixes:
+        cc_path = which('clang{}'.format(suffix))
+        cxx_path = which('clang{}'.format(suffix))


This should say clang++ -- I'm getting strange linker errors because CMake is trying to link C++ programs with the C compiler.

Oops! Good catch, thanks--this was a typo. 😊

gribozavr · 2015-12-27T06:55:43Z

@modocache Thank you! This patch is much simpler and I like it. There's one issue though, the C++ compiler name is not correct, see above.

modocache · 2015-12-27T07:02:03Z

@gribozavr Updated! Thanks.

https://bugs.swift.org/browse/SR-237 calls for `build-script` and `build-script-impl` to be merged. This commit takes another step towards that goal by moving the logic that finds the path to the `clang` and `clang++` executables up into Python-land. Rather than simply moving all of the logic into `utils/build-script`, this commit moves relevant functions into a new Python module, named `swift_build_support`. This has several benefits: - The logic can be tested. Whereas `build-script-impl` needed to be run in order to verify its behavior, the logic extracted out of it into `swift_build_support` can be tested in isolation. - The logic can be split up into several files without polluting the `utils` directory, which now contains many different files that are unrelated to `build-script`.

modocache · 2015-12-27T07:37:01Z

Also went ahead and added the tests to validation-test. 👍

gribozavr · 2015-12-27T16:47:35Z

@modocache Thank you! Your PR passes all tests I did. Merging.

…-host-cc [build-script] Determine HOST_CC in build-script

modocache force-pushed the sr-237-build-script-impl-merge-host-cc branch from c804cc9 to 00b9f7d Compare December 24, 2015 04:54

gribozavr reviewed Dec 24, 2015
View reviewed changes

modocache force-pushed the sr-237-build-script-impl-merge-host-cc branch 2 times, most recently from 9043cf5 to a7e10b6 Compare December 24, 2015 18:19

gribozavr reviewed Dec 24, 2015
View reviewed changes

modocache force-pushed the sr-237-build-script-impl-merge-host-cc branch from a7e10b6 to 547b26c Compare December 24, 2015 21:13

gribozavr self-assigned this Dec 25, 2015

modocache force-pushed the sr-237-build-script-impl-merge-host-cc branch from 67629c6 to 71deb46 Compare December 25, 2015 19:42

modocache mentioned this pull request Dec 26, 2015

[test] Run Python unit tests in validation tests #778

Merged

modocache force-pushed the sr-237-build-script-impl-merge-host-cc branch from 71deb46 to e382cbe Compare December 26, 2015 17:24

gribozavr reviewed Dec 27, 2015
View reviewed changes

modocache force-pushed the sr-237-build-script-impl-merge-host-cc branch from e382cbe to 7e6dfe2 Compare December 27, 2015 07:01

modocache force-pushed the sr-237-build-script-impl-merge-host-cc branch from 7e6dfe2 to ca3e11b Compare December 27, 2015 07:36

trentxintong assigned andreslp and unassigned gribozavr and andreslp Dec 27, 2015

gribozavr self-assigned this Dec 27, 2015

gribozavr added a commit that referenced this pull request Dec 27, 2015

Merge pull request #761 from modocache/sr-237-build-script-impl-merge…

afde6d3

…-host-cc [build-script] Determine HOST_CC in build-script

gribozavr merged commit afde6d3 into swiftlang:master Dec 27, 2015

modocache deleted the sr-237-build-script-impl-merge-host-cc branch December 27, 2015 17:32

modocache mentioned this pull request Feb 11, 2016

[SR-237][build-script] Migrate more of build-script-impl to Python #1267

Merged

modocache mannequin mentioned this pull request Dec 24, 2015

[SR-237] Merge build-script-impl into build-script #42859

Open

[build-script] Determine HOST_CC in build-script #761

[build-script] Determine HOST_CC in build-script #761

Uh oh!

Conversation

modocache commented Dec 24, 2015

Uh oh!

gribozavr Dec 24, 2015

Choose a reason for hiding this comment

Uh oh!

gribozavr commented Dec 24, 2015

Uh oh!

modocache commented Dec 24, 2015

Uh oh!

gribozavr Dec 24, 2015

Choose a reason for hiding this comment

Uh oh!

modocache Dec 24, 2015

Choose a reason for hiding this comment

Uh oh!

gribozavr commented Dec 24, 2015

Uh oh!

modocache commented Dec 24, 2015

Uh oh!

modocache commented Dec 25, 2015

Uh oh!

modocache commented Dec 26, 2015

Uh oh!

gribozavr commented Dec 26, 2015

Uh oh!

modocache commented Dec 26, 2015

Uh oh!

gribozavr commented Dec 26, 2015

Uh oh!

modocache commented Dec 27, 2015

Uh oh!

gribozavr Dec 27, 2015

Choose a reason for hiding this comment

Uh oh!

modocache Dec 27, 2015

Choose a reason for hiding this comment

Uh oh!

gribozavr commented Dec 27, 2015

Uh oh!

modocache commented Dec 27, 2015

Uh oh!

modocache commented Dec 27, 2015

Uh oh!

gribozavr commented Dec 27, 2015

Uh oh!

Uh oh!