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

chore: remove swiftsettings from bdk-swift #449

Merged
merged 1 commit into from
Jan 23, 2024

Conversation

reez
Copy link
Collaborator

@reez reez commented Jan 22, 2024

Description

This removes the flag to suppress warnings on bdk-swift.

The motivation for this PR is to let bdk-swift work as a locally imported package, and work stably as a remote package.

Notes to the reviewers

We originally added this flag in PR 365 as the solution to the problem were bdk-swift users were getting many warnings in their project from bdk-swift. This was around the time of Xcode 13/14 and the correct solution.

However over the past few months I've seen a bunch of issues and weird behavior like in ldk-swift where the package would just error out, and being a blocker for anyone to even pull in the package I opened and resolved it; I didn't see that extreme of behavior in bdk-swift but kept monitoring it.

One bad behavior we did get in bdk-swift was if you were to use it as a local package Xcode wouldn't let you unless you commented it out this line of code manually, which is pretty frustrating and is why ldk-node just removed that code in a PR recently.

So my suggestion/solution for now its to remove it as well, and then monitor the situation to see if anything in Xcode changes that makes me feel comfortable if we put a fix in that it will remain stable.

An alternative solution is to put the line of code removed from bdk-ff Package.swift in the bdk-ffi Package.swift.txt, but at this point I’d rather wait on that since whatever change caused this in Xcode is not documented so they might keep changing things and potentially break whatever we fix.

Changelog notice

Checklists

All Submissions:

  • I've signed all my commits
  • I followed the contribution guidelines
  • I ran cargo fmt and cargo clippy before committing

New Features:

  • I've added tests for the new feature
  • I've added docs for the new feature

Bugfixes:

  • This pull request breaks the existing API
  • I've added tests to reproduce the issue which are now passing
  • I'm linking the issue being fixed by this PR

@reez reez marked this pull request as ready for review January 22, 2024 22:01
Copy link
Member

@thunderbiscuit thunderbiscuit left a comment

Choose a reason for hiding this comment

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

ACK 6f4b4c9.

@thunderbiscuit thunderbiscuit merged commit 6f4b4c9 into bitcoindevkit:master Jan 23, 2024
1 check passed
@reez reez deleted the spm-warn branch January 23, 2024 18:12
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.

2 participants