Skip to content

Conversation

@marcrasi
Copy link

@marcrasi marcrasi commented Apr 11, 2019

I don't know if this will actually work. I ran SWIFT_PACKAGE=tensorflow_linux,no_test swift/utils/build-toolchain local.swift on my workstation to check, and I'll report back tomorrow morning when I wake up if that succeeded.

I think that install-tensorflow accidentally got removed a long time ago. install-tensorflow is responsible for copying the tensorflow libs into the toolchain:

https://github.com/apple/swift/blob/02351479c5423a78f46c5f65bfb3abac57123831/utils/build-script-impl#L3831.

I have an unverified guess about why this didn't cause problems earlier: Some other step is copying all ".so" files into the toolchain, and the tensorflow libs used to be "libtensorflow.so" and "libtensorflow_framework.so". But recent tensorflow changes added some ".so.VERSION" files that don't get copied by the other step. So we need to bring "install-tensorflow" back.

@marcrasi marcrasi requested a review from bgogul April 11, 2019 06:31
@marcrasi
Copy link
Author

worked on my workstation!

@marcrasi marcrasi merged commit 0e27a7d into swiftlang:tensorflow Apr 11, 2019
@marcrasi marcrasi deleted the add-install-tensorflow branch April 11, 2019 15:35
@eaplatanios
Copy link

eaplatanios commented Apr 12, 2019

Didn't this get fixed by #23895 ? Or at least, before I made those changes so that the versioned files are copied and it seemed to work.

@bgogul
Copy link
Contributor

bgogul commented Apr 12, 2019

Didn't this get fixed by #23895 ? Or at least, before I made those changes so that the versioned files are copied and it seemed to work.

Both are needed. This PR takes care of copying the files when creating an installable package.

@eaplatanios
Copy link

Oh right. There is also a couple test failures I currently get when trying to build the toolchain, for which I don't know the cause. I get two failures that both say something like this:

--
Exit Code: 1

Command Output (stderr):
--
/Users/eaplatanios/Development/GitHub/swift-source/swift/test/stdlib/TestUserInfo.swift:134:44: error: 'archivedData(withRootObject:)' is only available on OS X 10.11 or newer
        let archivedNote = NSKeyedArchiver.archivedData(withRootObject: note)
                                           ^
/Users/eaplatanios/Development/GitHub/swift-source/swift/test/stdlib/TestUserInfo.swift:134:44: note: add 'if #available' version check
        let archivedNote = NSKeyedArchiver.archivedData(withRootObject: note)
                                           ^
/Users/eaplatanios/Development/GitHub/swift-source/swift/test/stdlib/TestUserInfo.swift:130:10: note: add @available attribute to enclosing instance method
    func test_classForCoder() {
         ^
/Users/eaplatanios/Development/GitHub/swift-source/swift/test/stdlib/TestUserInfo.swift:62:7: note: add @available attribute to enclosing class
class TestUserInfo : TestUserInfoSuper {
      ^

--

Do you know what may be causing this?

@marcrasi
Copy link
Author

@eaplatanios, I've never seen that before, but I don't build on macos very often. I'll kick off a build on my mac machine to see if it happens for me. It'll be a few hours before I get results...

You're not on an OS X older than 10.11, are you?

@eaplatanios
Copy link

No I’m running the latest version of Mojave. Thanks Marc!

@marcrasi
Copy link
Author

I had to fix something (#24007), but after that, a full toolchain build with tests worked for me on my mac (SWIFT_PACKAGE=tensorflow_osx ./swift/utils/build-toolchain local.swift 2>&1 | tee buildlog.txt).

@eaplatanios
Copy link

@marcrasi Thanks for checking and fixing the issue with the dynamic libraries! It turns out I only get the errors if I use the following command: ./utils/build-toolchain-tensorflow tensorflow --pkg. If I remove --pkg it runs fine though so the issue is with building xcode packages for the toolchain. I am using MacOS 10.14.4 by the way.

rxwei pushed a commit to rxwei/swift that referenced this pull request May 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants