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

Build fails if BUILD_LIBRARY_FOR_DISTRIBUTION is set to YES #94

Closed
2 tasks done
danieleformichelli opened this issue Aug 23, 2021 · 6 comments
Closed
2 tasks done
Labels
bug Something isn't working

Comments

@danieleformichelli
Copy link

danieleformichelli commented Aug 23, 2021

Hello,
At Tuist we cache project's frameworks via xcframeworks and thus we add BUILD_LIBRARY_FOR_DISTRIBUTION=YES to xcodebuild command when building. This has worked fine but it's a problem when a project depends on swift-collections as build fails if we specify the BUILD_LIBRARY_FOR_DISTRIBUTION setting.

How hard would it be to make compilation work also with this flag?

Information

  • Package version: 0.0.7
  • Platform version: macOS 11.5.1
  • Swift version: Apple Swift version 5.4.2 (swiftlang-1205.0.28.2 clang-1205.0.19.57) Target: x86_64-apple-darwin20.6.0

Checklist

  • If possible, I've reproduced the issue using the main branch of this package.
  • I've searched for existing GitHub issues.

Steps to Reproduce

Run swift build -Xswiftc -enable-library-evolution

Expected behavior

Build should work fine

Actual behavior

Build fails with error:

swift build -Xswiftc -enable-library-evolution                                                                                                               15:09:48
/wrkdir/swift-collections/Sources/DequeModule/_DequeBuffer.swift:14:3: error: deinitializer can only be '@inlinable' if the class is '@_fixed_layout'
  @inlinable
  ^~~~~~~~~~

/wrkdir/swift-collections/Sources/DequeModule/_DequeBuffer.swift:14:3: error: deinitializer can only be '@inlinable' if the class is '@_fixed_layout'
  @inlinable
  ^~~~~~~~~~

/wrkdir/swift-collections/Sources/DequeModule/_UnsafeWrappedBuffer.swift:26:10: error: 'let' property 'first' may not be initialized directly; use "self.init(...)" or "self = ..." instead
    self.first = first
         ^
/wrkdir/swift-collections/Sources/DequeModule/_UnsafeWrappedBuffer.swift:15:16: note: 'first' declared here
  internal let first: UnsafeBufferPointer<Element>
               ^
/wrkdir/swift-collections/Sources/DequeModule/_UnsafeWrappedBuffer.swift:27:10: error: 'let' property 'second' may not be initialized directly; use "self.init(...)" or "self = ..." instead
    self.second = second
         ^
/wrkdir/swift-collections/Sources/DequeModule/_UnsafeWrappedBuffer.swift:18:16: note: 'second' declared here
  internal let second: UnsafeBufferPointer<Element>?
               ^
/wrkdir/swift-collections/Sources/DequeModule/_UnsafeWrappedBuffer.swift:68:10: error: 'let' property 'first' may not be initialized directly; use "self.init(...)" or "self = ..." instead
    self.first = first
         ^
/wrkdir/swift-collections/Sources/DequeModule/_UnsafeWrappedBuffer.swift:57:16: note: 'first' declared here
  internal let first: UnsafeMutableBufferPointer<Element>
               ^
/wrkdir/swift-collections/Sources/DequeModule/_UnsafeWrappedBuffer.swift:69:10: error: 'let' property 'second' may not be initialized directly; use "self.init(...)" or "self = ..." instead
    self.second = second?.count == 0 ? nil : second
         ^
/wrkdir/swift-collections/Sources/DequeModule/_UnsafeWrappedBuffer.swift:60:16: note: 'second' declared here
  internal let second: UnsafeMutableBufferPointer<Element>?
               ^
/wrkdir/swift-collections/Sources/DequeModule/_UnsafeWrappedBuffer.swift:26:10: error: 'let' property 'first' may not be initialized directly; use "self.init(...)" or "self = ..." instead
    self.first = first
         ^
/wrkdir/swift-collections/Sources/DequeModule/_UnsafeWrappedBuffer.swift:15:16: note: 'first' declared here
  internal let first: UnsafeBufferPointer<Element>
               ^
/wrkdir/swift-collections/Sources/DequeModule/_UnsafeWrappedBuffer.swift:27:10: error: 'let' property 'second' may not be initialized directly; use "self.init(...)" or "self = ..." instead
    self.second = second
         ^
/wrkdir/swift-collections/Sources/DequeModule/_UnsafeWrappedBuffer.swift:18:16: note: 'second' declared here
  internal let second: UnsafeBufferPointer<Element>?
               ^
/wrkdir/swift-collections/Sources/DequeModule/_UnsafeWrappedBuffer.swift:68:10: error: 'let' property 'first' may not be initialized directly; use "self.init(...)" or "self = ..." instead
    self.first = first
         ^
/wrkdir/swift-collections/Sources/DequeModule/_UnsafeWrappedBuffer.swift:57:16: note: 'first' declared here
  internal let first: UnsafeMutableBufferPointer<Element>
               ^
/wrkdir/swift-collections/Sources/DequeModule/_UnsafeWrappedBuffer.swift:69:10: error: 'let' property 'second' may not be initialized directly; use "self.init(...)" or "self = ..." instead
    self.second = second?.count == 0 ? nil : second
         ^
/wrkdir/swift-collections/Sources/DequeModule/_UnsafeWrappedBuffer.swift:60:16: note: 'second' declared here
  internal let second: UnsafeMutableBufferPointer<Element>?
               ^
/wrkdir/swift-collections/Sources/DequeModule/Deque._UnsafeHandle.swift:31:12: error: 'let' property '_header' may not be initialized directly; use "self.init(...)" or "self = ..." instead
      self._header = header
           ^
/wrkdir/swift-collections/Sources/DequeModule/Deque._UnsafeHandle.swift:16:9: note: '_header' declared here
    let _header: UnsafeMutablePointer<_DequeBufferHeader>
        ^
/wrkdir/swift-collections/Sources/DequeModule/Deque._UnsafeHandle.swift:32:12: error: 'let' property '_elements' may not be initialized directly; use "self.init(...)" or "self = ..." instead
      self._elements = elements
           ^
/wrkdir/swift-collections/Sources/DequeModule/Deque._UnsafeHandle.swift:18:9: note: '_elements' declared here
    let _elements: UnsafeMutablePointer<Element>
        ^
/wrkdir/swift-collections/Sources/DequeModule/Deque._UnsafeHandle.swift:34:12: error: 'let' property '_isMutable' may not be initialized directly; use "self.init(...)" or "self = ..." instead
      self._isMutable = isMutable
           ^
/wrkdir/swift-collections/Sources/DequeModule/Deque._UnsafeHandle.swift:21:9: note: '_isMutable' declared here
    let _isMutable: Bool
        ^
/wrkdir/swift-collections/Sources/DequeModule/Deque._UnsafeHandle.swift:31:12: error: 'let' property '_header' may not be initialized directly; use "self.init(...)" or "self = ..." instead
      self._header = header
           ^
/wrkdir/swift-collections/Sources/DequeModule/Deque._UnsafeHandle.swift:16:9: note: '_header' declared here
    let _header: UnsafeMutablePointer<_DequeBufferHeader>
        ^
/wrkdir/swift-collections/Sources/DequeModule/Deque._UnsafeHandle.swift:32:12: error: 'let' property '_elements' may not be initialized directly; use "self.init(...)" or "self = ..." instead
      self._elements = elements
           ^
/wrkdir/swift-collections/Sources/DequeModule/Deque._UnsafeHandle.swift:18:9: note: '_elements' declared here
    let _elements: UnsafeMutablePointer<Element>
        ^
/wrkdir/swift-collections/Sources/DequeModule/Deque._UnsafeHandle.swift:34:12: error: 'let' property '_isMutable' may not be initialized directly; use "self.init(...)" or "self = ..." instead
      self._isMutable = isMutable
           ^
/wrkdir/swift-collections/Sources/DequeModule/Deque._UnsafeHandle.swift:21:9: note: '_isMutable' declared here
    let _isMutable: Bool
        ^
/wrkdir/swift-collections/Sources/DequeModule/Deque+Collection.swift:31:21: error: 'self' used before 'self.init' call or assignment to 'self'
      self._storage = _storage
                    ^
/wrkdir/swift-collections/Sources/DequeModule/Deque+Collection.swift:32:22: error: 'self' used before 'self.init' call or assignment to 'self'
      self._nextSlot = start
                     ^
/wrkdir/swift-collections/Sources/DequeModule/Deque+Collection.swift:33:21: error: 'self' used before 'self.init' call or assignment to 'self'
      self._endSlot = end
                    ^
/wrkdir/swift-collections/Sources/DequeModule/Deque+Collection.swift:34:5: error: 'self.init' isn't called on all paths before returning from initializer
    }
    ^
/wrkdir/swift-collections/Sources/DequeModule/Deque+Collection.swift:31:21: error: 'self' used before 'self.init' call or assignment to 'self'
      self._storage = _storage
                    ^
/wrkdir/swift-collections/Sources/DequeModule/Deque+Collection.swift:32:22: error: 'self' used before 'self.init' call or assignment to 'self'
      self._nextSlot = start
                     ^
/wrkdir/swift-collections/Sources/DequeModule/Deque+Collection.swift:33:21: error: 'self' used before 'self.init' call or assignment to 'self'
      self._endSlot = end
                    ^
/wrkdir/swift-collections/Sources/DequeModule/Deque+Collection.swift:34:5: error: 'self.init' isn't called on all paths before returning from initializer
    }
    ^
/wrkdir/swift-collections/Sources/DequeModule/Deque+Collection.swift:31:21: error: 'self' used before 'self.init' call or assignment to 'self'
      self._storage = _storage
                    ^
/wrkdir/swift-collections/Sources/DequeModule/Deque+Collection.swift:32:22: error: 'self' used before 'self.init' call or assignment to 'self'
      self._nextSlot = start
                     ^
/wrkdir/swift-collections/Sources/DequeModule/Deque+Collection.swift:33:21: error: 'self' used before 'self.init' call or assignment to 'self'
      self._endSlot = end
                    ^
/wrkdir/swift-collections/Sources/DequeModule/Deque+Collection.swift:34:5: error: 'self.init' isn't called on all paths before returning from initializer
    }
    ^
@lorentey
Copy link
Member

This package isn't targeting the BUILD_LIBRARY_FOR_DISTRIBUTION dialect of Swift, so this isn't a supported configuration. In particular:

  1. We make absolutely no promises about preserving ABI compatibility across even minor releases of this project. It's quite the opposite in fact: we will change the layout of any type, add/remove internal/@usableFromInline methods etc. whenever there is any benefit in doing so, even across patch releases.
  2. We do not test that the project behaves correctly in the -enable-library-evolution dialect. (As a rule, we never build or test the package with this option enabled. It may work, or it may not. PRs with fixes are welcome, as long as they do not interfere with regular builds.)
  3. We do not measure/optimize the performance of such builds.

That said, the errors above reveal some (trivial) inconsistencies that are pretty easy to fix by sprinkling some @frozen attributes over the codebase. #97 will do that, and as a happy side effect, it also happens to fix the build errors above.

Note: because the package isn't ABI-stable, @frozen in this case does not imply that we promise to preserve the storage layout of structs with this attribute between any two releases, including minor/patch releases.

⚠️⚠️⚠️ Never upgrade to a new swift-collection release without recompiling all dependent projects. ⚠️⚠️⚠️

@xwu
Copy link
Contributor

xwu commented Aug 30, 2021

@lorentey Doesn't this suggest that it would be best to deliberately throw something in the project to prevent compiling with -enable-library-evolution? Seems otherwise it'd be overpromising and delivering a footgun.

@lorentey
Copy link
Member

lorentey commented Aug 31, 2021

@xwu Well, we could add a canary struct that intentionally emits an error in this specific configuration, but it feels a bit rude -- I don't actually see any major reason we would want to actively fight against such configurations. The ABI stable dialect isn't that different from regular Swift, as long as one doesn't care about ABI breaks or minor performance differences.

/* non-frozen */ public struct _ResiliencyTrap {
 @inlinable init() {}
}

Developers working on large apps sometimes like to set up binary artifact caches to speed up builds. By and large I think it's fair to assume they know what they're doing. Personally, I would prefer to make this (quite tricky) process easier -- throwing obstacles in their way seems like a very unproductive use of our collective time. (Although I can't promise that we won't break things accidentally, like we did in 0.0.6.)

Ideally such caching would be done with the full cooperation of the package manager, but IIUC sadly that isn't entirely possible yet. So I expect such folks vendor their package dependencies and replace SPM with xcodebuild (or whatever) to produce xcframeworks or whatnot -- which is fine as long as they do not violate the licensing terms, and as long as this doesn't generate an unreasonable maintenance burden upstream. In these setups, upgrading to a new upstream release usually requires some manual labor that hopefully includes triggering a rebuild of all dependents, and preventing old builds from linking with the new version.

@danyf90 are you able to tell us a little about your setup? Is this a reasonable description of it?

@danieleformichelli
Copy link
Author

Yep, tuist can replace SPM dependencies with xcframework binaries built using xcodebuild.

Luckily, looks like we have been able to do that without enabling library evolution, by using the -allow-internal-distribution xcodebuild flag, so looks like this is no longer a problem for tuist.

That said, it might be useful to someone else to be able to build with library evolution enabled, so if it's an easy change it might be worth to consider it

@lorentey
Copy link
Member

lorentey commented Sep 3, 2021

-allow-internal-distribution is a good solution. (Make sure the generated binaries are only used for speeding up internal development, though. They will only work if dependents are built with the exact same toolchain; a compiler update or package update will require a full rebuild.)

Supporting BUILD_LIBRARY_FOR_DISTRIBUTION would require the package to be ABI stable. This is wouldn't be reasonable at this point -- the whole point of developing these data structures in package form is to prevent us from having to freeze the ABI until we are confident we won't want to change implementation details. (Otherwise we could just propose adding them for the stdlib right now.)

@danieleformichelli
Copy link
Author

Thanks for your detailed answers! 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants