Skip to content

[build] Don't infer Clang path when crosscompiling #34664

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Conversation

edymtt
Copy link
Contributor

@edymtt edymtt commented Nov 10, 2020

Addresses rdar://71240426

@edymtt
Copy link
Contributor Author

edymtt commented Nov 10, 2020

@swift-ci build toolchain macOS

@edymtt
Copy link
Contributor Author

edymtt commented Nov 10, 2020

@swift-ci please smoke test

@edymtt
Copy link
Contributor Author

edymtt commented Nov 10, 2020

Follow up to #34437

@edymtt
Copy link
Contributor Author

edymtt commented Nov 10, 2020

@swift-ci please test stdlib with toolchain

@edymtt
Copy link
Contributor Author

edymtt commented Nov 10, 2020

preset=stdlib_S_standalone_minimal_macho_x86_64,build,test
@swift-ci please test with toolchain and preset

@swift-ci
Copy link
Contributor

Build failed
Swift Test OS X Platform
Git Sha - f5afc7e

@swift-ci
Copy link
Contributor

Build failed
Swift Test OS X Platform
Git Sha - f5afc7e

@edymtt
Copy link
Contributor Author

edymtt commented Nov 10, 2020

@swift-ci Please Test Source Compatibility

@edymtt
Copy link
Contributor Author

edymtt commented Nov 10, 2020

@swift-ci please smoke test Linux

@shahmishal
Copy link
Member

16:12:18 Failed Tests (2):
16:12:18   Swift(macosx-x86_64) :: stdlib/KeyPathAppending.swift
16:12:18   Swift(macosx-x86_64) :: stdlib/ArrayDiagnostics.swift

Not sure why it's failing on this change

@edymtt
Copy link
Contributor Author

edymtt commented Nov 10, 2020

Those failures seem to be happening on the "main" job as well and makes me think they are unrelated to this change

https://ci.swift.org/job/oss-swift-test-stdlib-with-toolchain/62/

I would need to double check locally just in case

@swift-ci
Copy link
Contributor

macOS Toolchain
Download Toolchain
Git Sha - f5afc7e

Install command
tar -zxf swift-PR-34664-762-osx.tar.gz --directory ~/

@compnerd
Copy link
Member

Hey @edymtt could you share a bit more context on this? I don't understand what this is supposed to do/what it does.

From ~CMakeLists.txt:465

if("${SWIFT_NATIVE_LLVM_TOOLS_PATH}" STREQUAL "")
  set(SWIFT_CROSS_COMPILING FALSE)
else()
  set(SWIFT_CROSS_COMPILING TRUE)
endif()

The NOT SWIFT_CROSS_COMPILING implies that SWIFT_NATIVE_TOOLS_LLVM_PATH would be "", which means that this was just setting the variable to empty (so it should be a no-op)?

@edymtt
Copy link
Contributor Author

edymtt commented Nov 11, 2020

To the best of my understanding, when we are building Swift for the host architecture, we expect SWIFT_NATIVE_TOOLS_LLVM_PATH/SWIFT_NATIVE_CLANG_TOOLS_PATH to not be set when CMake is invoked (as per toolchain job log) -- the code in cmake/modules/SwiftSharedCMakeConfig.cmake will then leverage the values obtained by finding the LLVM package to set the path for the compiler.

When we are cross compiling, those values are set when invoking CMake and we want to consider them -- as such my patch is basically mimicking what we are already doing at https://github.com/apple/swift/pull/34664/files#diff-af70c8d589bfadd011360af736827b9f3e3bd1655f7ceb9f08db8fc3045acaeeR62 for SWIFT_NATIVE_TOOLS_LLVM_PATH, when we sense SWIFT_CROSS_COMPILING to understand if that what's the case and avoid overwriting the value

Let me know if anything is missing in my explanation.

Copy link
Member

@compnerd compnerd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, if the checks are for the value being set rather than empty, that would explain that. Thanks!

@edymtt
Copy link
Contributor Author

edymtt commented Nov 11, 2020

Closing this in favor of #34628

@edymtt edymtt closed this Nov 11, 2020
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.

4 participants