Skip to content

[swiftSyntax] Fix issue where swiftc was not found when swiftSyntax is build via Xcode #18673

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

Merged
merged 1 commit into from
Aug 15, 2018

Conversation

ahoppen
Copy link
Member

@ahoppen ahoppen commented Aug 13, 2018

swiftSyntax finds swiftc based on the directory layout of the build folder. However, that layout is different if swiftSyntax is built via Xcode than if it's built with Ninja.

For Ninja:

- bin/
  - swiftc
- lib/
  - swift/
    - ${target}/
      - libswiftSwiftSyntax.[dylib|so]

For Xcode:

- bin/
  - swiftc
- lib/
  - swift/
    - macosx/
      - x86_64/
        - libswiftSwiftSyntax.[dylib|so]

Allow swiftSyntax to find swiftc by traversing up to the lib folder and then moving to bin/ instead of hardcoding the nesting level of libswiftSwiftSyntax.dylib inside lib.

@ahoppen ahoppen requested a review from harlanhaskins August 13, 2018 16:06
@ahoppen
Copy link
Member Author

ahoppen commented Aug 13, 2018

@swift-ci Please smoke test

.deletingLastPathComponent()
.appendingPathComponent("bin")
.appendingPathComponent("swiftc")
// Traverse the path up until we find the
Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpick: unfinished comment

@harlanhaskins
Copy link
Contributor

This is definitely an improvement, thanks!

@ahoppen ahoppen force-pushed the swift-syntax-xcode branch from 0112573 to a46d08e Compare August 13, 2018 17:13
@ahoppen
Copy link
Member Author

ahoppen commented Aug 13, 2018

@swift-ci Please smoke test and merge

@compnerd
Copy link
Member

Hmm, is there a reason to not try to make the layout uniform across the two builds? It seems that it only complicates things to have the different layouts. Making the code more robust anyways is probably a good idea.

@harlanhaskins
Copy link
Contributor

@compnerd I think this is the correct answer -- this library is going to be somewhere under lib/ on all platforms, so walking up to lib/ and then to bin/ seems like a good way to support all layouts.

I agree that the layouts should match across platforms, but that's a more invasive change.

@ahoppen
Copy link
Member Author

ahoppen commented Aug 13, 2018

@swift-ci Please smoke test and merge

@compnerd
Copy link
Member

@harlanhaskins - also would be interesting to consider windows :-)

@compnerd
Copy link
Member

Sorry @ahoppen about causing the conflicts. Should be relatively easy to get these changes moved on top of the changes that I had.

@harlanhaskins
Copy link
Contributor

@compnerd Ah, yes! Apologies -- I am admittedly entirely uninformed of Windows library structure. What does the layout currently look like on Windows?

@ahoppen ahoppen force-pushed the swift-syntax-xcode branch from a46d08e to a4c879c Compare August 13, 2018 20:02
@ahoppen
Copy link
Member Author

ahoppen commented Aug 13, 2018

@swift-ci Please smoke test

Copy link
Contributor

@jrose-apple jrose-apple left a comment

Choose a reason for hiding this comment

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

Yeah, this is incorrect, sorry. For Apple platforms, both Ninja and Xcode build into the architecture-specific directories first, but then lipo together all the architectures into the platform directory. If Xcode's not doing that for you we either have a CMake bug or you're building the wrong target.

@ahoppen ahoppen force-pushed the swift-syntax-xcode branch from a4c879c to e6228c0 Compare August 14, 2018 23:27
@ahoppen ahoppen force-pushed the swift-syntax-xcode branch from e6228c0 to 3534733 Compare August 15, 2018 00:16
@ahoppen
Copy link
Member Author

ahoppen commented Aug 15, 2018

@swift-ci Please smoke test

@ahoppen ahoppen requested a review from compnerd August 15, 2018 00:23
@ahoppen
Copy link
Member Author

ahoppen commented Aug 15, 2018

@compnerd This change still covers your use case, right?

@ahoppen ahoppen merged commit 8177868 into swiftlang:master Aug 15, 2018
@ahoppen ahoppen deleted the swift-syntax-xcode branch August 15, 2018 18:09
maldahleh pushed a commit to maldahleh/swift that referenced this pull request Oct 26, 2020
[swiftSyntax] Fix issue where swiftc was not found when swiftSyntax is build via Xcode
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