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 support for Swift 5.5 and Xcode 13 #480

Merged
merged 8 commits into from
Oct 27, 2021
Merged

Conversation

AvdLee
Copy link
Contributor

@AvdLee AvdLee commented Oct 25, 2021

The dynamic type is no longer needed and actually breaks running tests for Danger plugins with Xcode 13.

The error that's fixed with this PR:

Swift package target 'Danger' is linked as a static library by 'WeTransferPRLinterTests' and 'Danger', but cannot be built dynamically because there is a package product with the same name.

Fixes #475

@AvdLee AvdLee self-assigned this Oct 25, 2021
Package.swift Outdated Show resolved Hide resolved
@orta
Copy link
Member

orta commented Oct 25, 2021

error: no tests found; create a target in the 'Tests' directory

Seems super strange

@f-meloni
Copy link
Member

I believe is because of 8ae3636
I've reverted it, so if you pull master it should work now

@AvdLee
Copy link
Contributor Author

AvdLee commented Oct 26, 2021

@f-meloni didn't seem to fix it 🤔

@f-meloni
Copy link
Member

f-meloni commented Oct 26, 2021

@AvdLee that commit is still in your branch, you will probably have to make an interactive rebase :(

Schermata 2021-10-26 alle 18 06 36

Schermata 2021-10-26 alle 18 06 41

Copy link
Member

@orta orta left a comment

Choose a reason for hiding this comment

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

500 commits for a 4 word change ;) - I've been there

Copy link
Member

@f-meloni f-meloni left a comment

Choose a reason for hiding this comment

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

Thank you for this!
I will make few other tests just to be sure everything works as expected, even if the CI passing is already a really good sign, and merge it.
Can you please update the changelog if you don't mind?

@AvdLee
Copy link
Contributor Author

AvdLee commented Oct 27, 2021

Can you please update the changelog if you don't mind?

There's a Danger rule for that, did you know? 😜
On a serious note; maybe you like this?
https://docs.github.com/en/repositories/releasing-projects-on-github/automatically-generated-release-notes

I'll add it now at least!

@f-meloni
Copy link
Member

To be fair I can test it in master as well, no reason to hold this PR

@f-meloni f-meloni merged commit 82e1d44 into danger:master Oct 27, 2021
@f-meloni
Copy link
Member

f-meloni commented Oct 27, 2021

Can you please update the changelog if you don't mind?

There's a Danger rule for that, did you know? 😜 On a serious note; maybe you like this? https://docs.github.com/en/repositories/releasing-projects-on-github/automatically-generated-release-notes

I'll add it now at least!

I think we are linking the changelog on Twitter when we make a release (which automatically adds the new version to the changelog blow the ## Master), if there is a way to automatically generate the release with an API, we might maybe point directly to the release, but would be generated after the tag is pushed, and so when I think the twitter hook is already been executed, and the tweet already created 🤔
cc. @orta

@f-meloni
Copy link
Member

f-meloni commented Oct 29, 2021

I've tried to do a make install (after I deleted the previous files in /usr/lib/danger, like brew would do, and if I run Danger at the moment I get

ERROR: Could not find a libDanger to link against at any of: [".build/debug", ".build/x86_64-unknown-linux/debug", ".build/release", "/usr/local/lib/danger", "/opt/homebrew/lib/danger"]
Or via Homebrew, or Marathon

because there is no library to copy in the .build.
It works only if you are using SPM in the project (and in that case works also if you install it via brew, because builds the framework with SPM)

@f-meloni
Copy link
Member

I've reproduced the issue here https://github.com/danger/swift/runs/4055493568?check_suite_focus=true and added a CI suite that can reproduce the issue

@AvdLee
Copy link
Contributor Author

AvdLee commented Nov 1, 2021

@f-meloni is there a middle ground you think that fixes the Xcode 13 bug ánd doesn't break old systems?
As a last resort we could keep using our own fork, but that's obviously not ideal!

@f-meloni
Copy link
Member

f-meloni commented Nov 4, 2021

@AvdLee I would love to find something like a parameter that I can pass to swift build to force it to build Danger as dynamic without having to specify it on SPM.
If that doesn't work then the only other solution I can think of is changing the Package.swift from the install script before installing

@AvdLee
Copy link
Contributor Author

AvdLee commented Nov 4, 2021

If that doesn't work then the only other solution I can think of is changing the Package.swift from the install script before installing

If it's only failing for that script, it would make total sense. It would be weird to me to not support SPM which is heavily used in Danger environments, so we need to get to a working solution. This sounds like the way to go to me!

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.

Latest release unhides DangerDeps product causing failures
3 participants