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

Make Octokit build with Linux #69

Merged
merged 25 commits into from
Nov 12, 2018

Conversation

f-meloni
Copy link
Member

@f-meloni f-meloni commented Nov 6, 2018

I was trying to use octokit.swift as Github client on Danger-swift (PR danger/swift#95 )

But i discovered that octokit.swift doesn't compile on the Linux environment

https://travis-ci.org/danger/swift/jobs/451464822

This PR solves the problem

https://travis-ci.org/danger/swift/jobs/451515014

To keep the objc compatibility i had to duplicate the models to be not @objc on Linux.
Not sure this is the best possible approach, i would prefer to not have duplications obviously, i'm open to any suggestion or better ideas :)

@f-meloni f-meloni changed the title Work with linux Make Octokit build with Linux Nov 6, 2018
Copy link
Contributor

@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.

Might also be worth adding linux to the CI too, to ensure forward compatibility - might only need to run swift build

@@ -9,6 +9,55 @@ public enum Openness: String, Codable {
case All = "all"
}

#if os(Linux)
open class Issue: Codable {
Copy link
Contributor

Choose a reason for hiding this comment

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

is it possible that the if statements can just wrap the first line of the definition?

Copy link
Member Author

@f-meloni f-meloni Nov 6, 2018

Choose a reason for hiding this comment

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

It needs to remove also the @objc variables,
I tried anyway to not put all the class inside the if but if i put there just the first line plus the objc variables, this is what happens

screenshot 2018-11-06 at 19 18 25

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks!

@pietbrauer
Copy link
Member

Hey, thanks for the PR.

I am also on the fence if removing the @objc is actually fine these days. It would be a breaking change, though. These issues weren't present on RequestKit when Tiago introduced the Linux build: https://github.com/nerdishbynature/RequestKit/pull/55/files.

Maybe you can have a look at the .travis.yml and make the same changes here too, so we have CI for Linux.

@f-meloni
Copy link
Member Author

f-meloni commented Nov 7, 2018

@pietbrauer It took some attempts but now it is running the CI on Linux as well.

Re: @objc i personally think that remove the objc compatibility would be the best option, even if it requires a new major release, but is your call :)

@pietbrauer
Copy link
Member

Sweet, thanks a lot!

It's 2018, I created this 2015 when Swift was only 1,5 years old. I guess it is ok to have a Swift Only APIClient these days. Any opinions @phatblat @nwest? Or anyone else of the 11 people who read this?

@f-meloni
Copy link
Member Author

f-meloni commented Nov 9, 2018

Hey @pietbrauer, did you have a think about this?
Should i remove the objc compatibility? :)

@pietbrauer
Copy link
Member

Yeah, let's go for it.

@f-meloni
Copy link
Member Author

@pietbrauer It should be fine now :)
If you are happy with it, can you please merge and release? Then i can point the release from danger-swift?
Thank you :)

Copy link
Member

@pietbrauer pietbrauer left a comment

Choose a reason for hiding this comment

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

Just one white space, everything else looks good.

open var numberOfPublicRepos: Int?
open var numberOfPublicGists: Int?
open var numberOfPrivateRepos: Int?

Copy link
Member

Choose a reason for hiding this comment

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

Could you remove the whitespace?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed in 4ac9946

@pietbrauer
Copy link
Member

Nice work, thanks!

@pietbrauer pietbrauer merged commit b63f2ec into nerdishbynature:master Nov 12, 2018
tngranados pushed a commit to tngranados/octokit.swift that referenced this pull request Jul 13, 2022
* Remove not used Color extension

* Duplicate Models to compile also with Linux

* Added allTests property to make the tests executable also on Linux

* Updated .travis.yml to use also Linux

* Renamed travis scripts

* Use the correct Script folder name

* Updated permissions for travis scripts

* Use swift version 4.2 on Linux

* Added LinuxMain.swift

* Try to make swift find LinuxMain file

* Update LinuxMain permissions

* Reverse 21892d6

* Updated macos install script

* install bundler on macos

* Use SPM default test location to make it find LinuxMain.swift

* Fixed defaultTestSuite compilation issue for macOS and tvOS

* Set correct path to the Tests info.plist

* Don't use Bundle(for:) on Linux

* Not use the Bundle at all on Linux

* Make tests compile

* Remove all the other Bundle(for:) calls on Linux

* Fix: Use resourceName

* Find files also on Fixtures folder

* Remove objc compatibility

* Remove spaces from User.swift file
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.

3 participants