Skip to content
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

Add workaround for SR-15123 #685

Closed
wants to merge 1 commit into from
Closed

Conversation

keith
Copy link
Member

@keith keith commented Sep 13, 2021

This works around an issue in Swift 5.5 / Xcode 13.0 beta 5 where
absolute paths to the sources being compiled are always used and
embedded in the case a vfs overlay is used. This tricks the compiler by
using a vfs overlay for the source files as well.

https://bugs.swift.org/browse/SR-15123

@google-cla google-cla bot added the cla: yes label Sep 13, 2021
@keith
Copy link
Member Author

keith commented Sep 13, 2021

We might not need this pending swiftlang/llvm-project#3246 but it does appear to work (only with the change being reverted though, otherwise it fails)

@keith
Copy link
Member Author

keith commented Sep 14, 2021

optimized a bit for revertability here

@keith keith force-pushed the ks/add-workaround-for-sr-15123 branch from 4f268f1 to bf3e30e Compare October 21, 2021 19:39
@bazelbuild bazelbuild deleted a comment from lyft-lint-bot Nov 23, 2021
@bazelbuild bazelbuild deleted a comment from lyft-lint-bot Nov 23, 2021
@bazelbuild bazelbuild deleted a comment from lyft-lint-bot Nov 23, 2021
@bazelbuild bazelbuild deleted a comment from lyft-lint-bot Nov 23, 2021
@bazelbuild bazelbuild deleted a comment from lyft-lint-bot Nov 29, 2021
@acecilia
Copy link

acecilia commented Jan 5, 2022

I just hit this issue trying to update xcode from 12.5.1 to 13.2.1. In my case, what I observed is that #fileID returns an absolute path that ends up inside the binaries. Before finding this PR I did write a simple reproducible example here

Thanks for the multiple attempts to fix it in different repositories @keith 🙏

How are you currently dealing with this problem? I am not sure how to work around it considering that:

  • The bug is still present in the latest Xcode 13.2.1
  • This PR is a draft, did not make it to master

Some options on the top of my mind:

  • Wait for swift 5.6, which hopefully comes with a fix (this is probably not a good idea, as it may pass some time until next release)
  • Stop using vfs overlay
  • Maintain my own clone of rules_swift and add this PR to it

@brentleyjones
Copy link
Collaborator

Instead of maintaining a fork, you can apply a patch with bazel: https://brentley.dev/patching-bazel-external-dependencies/

@keith
Copy link
Member Author

keith commented Jan 5, 2022

Yep that's what I would recommend in general, and what we're doing at Lyft. I felt a bit weird about merging this at the time because I wasn't sure if it would have any unexpected side-effects. Realistically we could probably merge it now, but I was hoping that my actual fixes upstream would land soon enough that we wouldn't need too. Unfortunately it looks like they won't be fixed until Swift 5.6, which I assume won't come to Xcode until the spring

@acecilia
Copy link

acecilia commented Jan 5, 2022

Got it, thank you 🙌

@keith keith force-pushed the ks/add-workaround-for-sr-15123 branch from 4951b9d to 8a4fde7 Compare March 18, 2022 00:46
@lyft-lint-bot
Copy link

Lyft integration job started: https://buildkite.com/lyft/rules-swift/builds/262 (must be Lyft employee to view)

@keith keith force-pushed the ks/add-workaround-for-sr-15123 branch from 8a4fde7 to 0f341bf Compare July 7, 2022 23:16
@keith keith force-pushed the ks/add-workaround-for-sr-15123 branch from 0f341bf to c09ef2f Compare August 5, 2022 18:31
@brentleyjones
Copy link
Collaborator

The linked SR is closed. Is it still a valid bug that should be reopened?

@keith
Copy link
Member Author

keith commented Aug 5, 2022

needs more investigation, i think there's a new one

This works around an issue in Swift 5.5 / Xcode 13.0 beta 5 where
absolute paths to the sources being compiled are _always_ used and
embedded in the case a vfs overlay is used. This tricks the compiler by
using a vfs overlay for the source files as well.

https://bugs.swift.org/browse/SR-15123
@keith keith force-pushed the ks/add-workaround-for-sr-15123 branch from c09ef2f to aecc3f4 Compare September 14, 2022 00:23
@keith
Copy link
Member Author

keith commented Sep 22, 2022

as of Xcode 14 we have confirmed this is fixed, and don't see any new reason to use this at the moment. I believe it was fixed earlier, with Swift 5.6, but I didn't test before Swift 5.7. Please file an issue if you find any new case that needs this!

@keith keith closed this Sep 22, 2022
@keith keith deleted the ks/add-workaround-for-sr-15123 branch September 22, 2022 22:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants