[build support] Generic prefixed binary lookup function in swift_build_support #1586

harlanhaskins · 2016-03-08T22:51:45Z

What's in this pull request?

A small modification to swift_build_support to allow looking up arbitrary commands like llvm-cov and llvm-profdata alongside clang and clang++.

Resolved bug number: N/A

Will help with SR-237.

Before merging this pull request to apple/swift repository:

Test pull request on Swift continuous integration.

Triggering Swift CI

The swift-ci is triggered by writing a comment on this PR addressed to the GitHub user @swift-ci. Different tests will run depending on the specific comment that you use. The currently available comments are:

Smoke Testing

Platform	Comment
All supported platforms	@swift-ci Please smoke test
OS X platform	@swift-ci Please smoke test OS X platform
Linux platform	@swift-ci Please smoke test Linux platform

Validation Testing

Platform	Comment
All supported platforms	@swift-ci Please test
OS X platform	@swift-ci Please test OS X platform
Linux platform	@swift-ci Please test Linux platform

Note: Only members of the Apple organization can trigger swift-ci.

harlanhaskins · 2016-03-09T23:13:56Z

@modocache @gribozavr Thoughts?

harlanhaskins · 2016-03-11T22:14:55Z

Actually, this doesn't take the native llvm/clang tools option into account, regardless. That needs to be addressed.

harlanhaskins · 2016-03-16T21:10:57Z

Any ideas here? The current functions don't take into account NATIVE_[LLVM|CLANG]_TOOLS_PATH, which is pretty important.

Neglecting that, I think this PR is good (and necessary to get coverage working on Linux).

harlanhaskins · 2016-03-18T23:39:59Z

@gribozavr How about this?

modocache · 2016-03-19T20:27:46Z

utils/swift_build_support/swift_build_support/toolchain.py

+
+    It requires a 'cc' and 'cxx' tool in its keyword args.
+    """
+    def __init__(self, cc=None, cxx=None, **kwargs):


Sorry if I'm not picking up on something here, but why provide default arguments for arguments the documentation claims are necessary? If you're looking for the ability to use named arguments like Toolchain(cc="foo", cxx="bar"), you can do that without supplying default arguments in Python.

I figured it was good to express that these arguments are strictly required, though the same could be expressed with

assert(tools.get('cc') is not None)

modocache · 2016-03-19T20:34:57Z

The commit message claims these changes will allow searching for arbitrary executables besides clang, like llvm-cov. Can you add a test case to prove it?

Modulo comments this looks like a good direction to me! 👍

harlanhaskins · 2016-03-19T20:37:36Z

I can add a test case, but I don't want tests to fail if the tester actually doesn't have the tools installed...

modocache · 2016-03-19T21:27:09Z

Can we assume that certain tools will always exist on OS X systems? If so you can run them on OS X only, like these are: https://github.com/apple/swift/blob/master/utils/swift_build_support/tests/test_xcrun.py#L18-L20

harlanhaskins · 2016-03-20T02:15:29Z

That's totally doable. IIRC, Xcode includes, at the very least, llvm-profdata, right @gribozavr?

modocache · 2016-03-21T13:54:18Z

Sure! I also don't think the test has to be for something related to profdata, just something besides Clang that can be found with the new functionality (i.e.: a test that would have failed prior to these changes).

harlanhaskins · 2016-03-21T20:08:16Z

@modocache Thoughts on the extra test?

modocache · 2016-03-23T22:11:03Z

Looks good to me! @gribozavr could you ask @swift-ci to please test?

gribozavr · 2016-03-23T22:35:25Z

@swift-ci Please test

harlanhaskins · 2016-03-24T20:21:35Z

Tests pass. @gribozavr Good to merge?

shahmishal · 2016-03-27T21:45:06Z

@swift-ci Please python lint

shahmishal · 2016-03-27T21:53:58Z

--- Python lint ---
./utils/swift-api-dump.py:98:80: W291 trailing whitespace
./utils/swift-api-dump.py:99:80: E501 line too long (82 > 79 characters)
./utils/swift_build_support/swift_build_support/toolchain.py:2:80: E501 line too long (80 > 79 characters)
./utils/swift_build_support/swift_build_support/toolchain.py:21:1: F401 'collections' imported but unused
./utils/swift_build_support/tests/test_toolchain.py:14:1: I100 Import statements are in the wrong order. import platform

shahmishal · 2016-03-29T07:53:23Z

@swift-ci Please python lint

shahmishal · 2016-03-29T07:54:52Z

@harlanhaskins Thanks for fixing the python lint errors.

harlanhaskins · 2016-04-07T21:24:18Z

@gribozavr @modocache Think this is good to merge after I rebase?

gribozavr · 2016-04-07T21:28:52Z

LGTM.

@swift-ci Please test

harlanhaskins · 2016-04-07T21:47:18Z

That test failure is unrelated.

Geforce8472 · 2016-04-08T08:33:06Z

This change seems to leave me unable to build swift on Fedora Linux.

which: no clang-3.8 in (/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/root/bin)
which: no clang-3.7 in (/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/root/bin)
which: no clang-3.6 in (/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/root/bin)
which: no clang-3.5 in (/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/root/bin)

./build-script: Can't find clang. Please install clang-3.5 or a later version.
./build-script: command terminated with a non-zero exit status 1, aborting

clang exists in /usr/bin currently version 3.7

rintaro · 2016-04-08T08:59:54Z

utils/swift_build_support/swift_build_support/toolchain.py

+            return host_toolchain(xcrun_toolchain,
+                                  suffixes=['38', '37', '36', '35'])
+    return host_toolchain(xcrun_toolchain,
+                          suffixes=['-3.8', '-3.7', '-3.6', '-3.5'])


suffixes should contain '', as before

Yep, you're right. Thanks for catching that! Fixed in master.

RLovelett · 2016-04-09T00:33:09Z

Unfortunately this PR broke Python 3 support.

$ utils/build-script --release -- --reconfigure
Traceback (most recent call last):
  File "utils/build-script", line 1167, in <module>
    sys.exit(main())
  File "utils/build-script", line 1163, in main
    return main_normal()
  File "utils/build-script", line 1023, in main_normal
    xcrun_toolchain=args.darwin_xcrun_toolchain)
  File "utils/swift_build_support/swift_build_support/toolchain.py", line 124, in host_clang
    suffixes=['-3.8', '-3.7', '-3.6', '-3.5'])
  File "utils/swift_build_support/swift_build_support/toolchain.py", line 101, in host_toolchain
    return _first_common_toolchain(tools, suffixes=suffixes)
  File "utils/swift_build_support/swift_build_support/toolchain.py", line 70, in _first_common_toolchain
    for name, tool in tools.iteritems():
AttributeError: 'dict' object has no attribute 'iteritems'

harlanhaskins force-pushed the generic-command-lookup branch 2 times, most recently from 951ab5b to 6980ae9 Compare March 8, 2016 23:46

harlanhaskins changed the title ~~[build support] Added generic prefixed binary lookup function to swift_build_support~~ [build support] Generic prefixed binary lookup function to swift_build_support Mar 8, 2016

harlanhaskins changed the title ~~[build support] Generic prefixed binary lookup function to swift_build_support~~ [build support] Generic prefixed binary lookup function in swift_build_support Mar 8, 2016

harlanhaskins force-pushed the generic-command-lookup branch from 6980ae9 to fec3c65 Compare March 9, 2016 00:04

harlanhaskins force-pushed the generic-command-lookup branch from fec3c65 to 81b6009 Compare March 9, 2016 23:55

[build_support] Added generic 'toolchain' lookup

af6f844

harlanhaskins force-pushed the generic-command-lookup branch from 81b6009 to af6f844 Compare March 18, 2016 23:39

modocache reviewed Mar 19, 2016
View reviewed changes

[build_support] Added test to find llvm-cov

6804328

Harlan Haskins added 2 commits March 21, 2016 13:10

[build_support] Added test to ensure llvm_cov properly exists

83535dd

[build_support] Added some comments

0aad53c

harlanhaskins mentioned this pull request Mar 22, 2016

[build_support] Added -sparse option for llvm-profdata if supported #1775

Merged

1 task

[build support] Fixed python lint warnings

f4ba4d3

harlanhaskins merged commit 55f1bb1 into swiftlang:master Apr 8, 2016

harlanhaskins deleted the generic-command-lookup branch April 8, 2016 05:18

rintaro reviewed Apr 8, 2016
View reviewed changes

harlanhaskins mentioned this pull request Apr 9, 2016

[build_support] Fixed python 3 errors #2111

Merged

1 task

[build support] Generic prefixed binary lookup function in swift_build_support #1586

[build support] Generic prefixed binary lookup function in swift_build_support #1586

Uh oh!

Conversation

harlanhaskins commented Mar 8, 2016

What's in this pull request?

Resolved bug number: N/A

Uh oh!

harlanhaskins commented Mar 9, 2016

Uh oh!

harlanhaskins commented Mar 11, 2016

Uh oh!

harlanhaskins commented Mar 16, 2016

Uh oh!

harlanhaskins commented Mar 18, 2016

Uh oh!

modocache Mar 19, 2016

Choose a reason for hiding this comment

Uh oh!

harlanhaskins Mar 19, 2016

Choose a reason for hiding this comment

Uh oh!

modocache commented Mar 19, 2016

Uh oh!

harlanhaskins commented Mar 19, 2016

Uh oh!

modocache commented Mar 19, 2016

Uh oh!

harlanhaskins commented Mar 20, 2016

Uh oh!

modocache commented Mar 21, 2016

Uh oh!

harlanhaskins commented Mar 21, 2016

Uh oh!

modocache commented Mar 23, 2016

Uh oh!

gribozavr commented Mar 23, 2016

Uh oh!

harlanhaskins commented Mar 24, 2016

Uh oh!

shahmishal commented Mar 27, 2016

Uh oh!

shahmishal commented Mar 27, 2016

Uh oh!

shahmishal commented Mar 29, 2016

Uh oh!

shahmishal commented Mar 29, 2016

Uh oh!

harlanhaskins commented Apr 7, 2016

Uh oh!

gribozavr commented Apr 7, 2016

Uh oh!

harlanhaskins commented Apr 7, 2016

Uh oh!

Geforce8472 commented Apr 8, 2016

Uh oh!

rintaro Apr 8, 2016

Choose a reason for hiding this comment

Uh oh!

harlanhaskins Apr 8, 2016

Choose a reason for hiding this comment

Uh oh!

RLovelett commented Apr 9, 2016

Uh oh!

Uh oh!