-
Notifications
You must be signed in to change notification settings - Fork 58
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
Linux testing on CI #90
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot for this PR, this is awesome to have StencilSwiftKit made compatible with Linux 👍
I don't have a Linux machine to test it and our CI isn't configured to test on Linux yet so I'll trust you tested it locally 😉
Can you just please add an entry in the CHANGELOG to mention the change (eg "added Linux compatibility" or something) and credit yourself? Thanks!
As a bonus it would be even better if you could also edit the circleci.yml to add a Linux container to our CI config so that we ensure that we won't introduce a regression on Linux in future versions of the lib 🙂
Totally agree with @AliSoftware, if we add Linux support we should test it on CI. |
53f2a81
to
ff94edd
Compare
Yeah so, |
51163a2
to
f6f1dbc
Compare
Right, so I've finally got linux tests working on CI. We should update the job names GitHub expects after the merge. Note that this introduces |
ae30d24
to
f40f5a8
Compare
Oh hey, forgot about this, we merged #94 that fixes a similar issue 😅. I'll rebase this PR to just add the linux support part. |
f40f5a8
to
9700bf5
Compare
@RahulKatariya With what Swift version did you run the tests on Linux? There's a bug (swiftlang/swift-corelibs-foundation#1536) that causes our Linux tests to crash when using |
Checked and the fix is neither in the swift 4.1 or 4.2 branches, only in master. @AliSoftware do you think they'd be open to cherry-picking that change into those branches, and would we go about it? Comment on that PR? Open an issue? |
For reference, we've pinged some Swift core team members, and @millenomi told us they'll look into merging swiftlang/swift-corelibs-foundation#1536 soon, but that it will not be included in the upcoming release of Swift anyway (too late already). So we'll have to rely on a workaround (at least in the test suite, if a local fix is possible without requiring a dedicated version of |
7593d2a
to
22ead52
Compare
@djbe The path methods were causing the compilation error on Linux, I didn't really use them. |
@AliSoftware I'm merging this now as it's "fixed", we can revisit this later if we want, but at least we won't block the PRs in SwiftGen anymore. |
I'm ok with merging to avoid waiting. A comment in the code referencing |
Note: This PR originally fixed some bugs with NSString on Linux. That has been fixed in #94. Now this PR is for testing SPM support on Linux.
StencilSwiftKit was not usable building on Linux due to two string to NSString coercions. The Linux swift compiler does not recognize the coercion.
Replacing them with NSString(string: ...) allows building projects using StencilSwiftKit on Linux.