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

Fix warnings generated by Xcode 10 #1740

Merged
merged 13 commits into from Oct 18, 2018

Conversation

Projects
None yet
5 participants
@lexorus
Member

lexorus commented Oct 9, 2018

Background

I wanted to debug some of the Moya functionalities within the library. So I opened it using latest Xcode, which is 10.0, and I found that there are plenty of warnings. So I decided to fix them first.
I noticed that there is one opened PR (#1730) with update to Xcode 10/Swift 4.2, but it seems to be inactive.
@Moya/contributors This is a pretty easy PR to review, so it will be nice if somebody will find some time
Any related changes that I made has a separate commit, so if you would like to separate this PR into multiple, just let me know.

What has been done

  1. Update Swift version to 4.2
  2. Update project to recommended settings. This includes enabling CLANG_WARN_DEPRECATED_OBJC_IMPLEMENTATIONS and CLANG_WARN_OBJC_IMPLICIT_RETAIN_SELF build settings
  3. Disable APPLICATION_EXTENSION_API_ONLY of MoyaTests target. This fixes the linker errors linking against a dylib which is not safe for use in application extensions
  4. Update circleci config to use Xcode 10.0.0
  5. Updated RxSwift dependency to version 4.3.1

How to test

Just open the project using the Xcode 10, build the Moya/ReactiveMoya/RXMoya targets, run their tests and run the example applications.

@lexorus

This comment has been minimized.

Member

lexorus commented Oct 9, 2018

Should I also update .circleci/config.yml's xcode version?

macos:
  xcode: '10.0.0'
@MoyaBot

This comment has been minimized.

MoyaBot commented Oct 9, 2018

2 Warnings
⚠️ Consider adding supporting documentation to this change. Documentation can be found in the docs directory.
⚠️ The Cartfile.resolved was updated, but there were no changes in Moya.podspec, Cartfile, Package.swift and Package.resolved.
Did you forget to update Moya.podspec, Cartfile, Package.swift and Package.resolved?
3 Messages
📖 iOS: Executed 254 tests, with 0 failures (0 unexpected) in 11.535 (11.654) seconds
📖 tvOS: Executed 254 tests, with 0 failures (0 unexpected) in 11.294 (11.425) seconds
📖 macOS: Executed 254 tests, with 0 failures (0 unexpected) in 11.792 (11.914) seconds

Generated by 🚫 Danger

@codecov-io

This comment has been minimized.

codecov-io commented Oct 9, 2018

Codecov Report

Merging #1740 into development will increase coverage by 0.83%.
The diff coverage is n/a.

Impacted file tree graph

@@               Coverage Diff               @@
##           development    #1740      +/-   ##
===============================================
+ Coverage           90%   90.83%   +0.83%     
===============================================
  Files                5        5              
  Lines              140      131       -9     
===============================================
- Hits               126      119       -7     
+ Misses              14       12       -2
Impacted Files Coverage Δ
Sources/ReactiveMoya/SignalProducer+Response.swift 100% <0%> (+6.06%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9363f0b...d8e56b5. Read the comment docs.

@SD10

This comment has been minimized.

Member

SD10 commented Oct 14, 2018

@lexorus Thanks for taking care of this 👍 Sorry for the delay!

Should I also update .circleci/config.yml's xcode version?

I think that would be great 👍

We should be able to proceed with a Moya 12.0.0-beta.1 release after this.

@SD10 SD10 added this to the 12.0 milestone Oct 14, 2018

@SD10

This comment has been minimized.

Member

SD10 commented Oct 14, 2018

@lexorus If you also want to add a CHANGELOG entry regarding support for Swift 4.2 that would be 🔥

@lexorus

This comment has been minimized.

Member

lexorus commented Oct 15, 2018

It seems like the circleci is using cached dependencies that are compiled with older version of swift. Hopefully disabling the cache builds and forcing carthage to rebuild dependencies with new swift version will solve the problem.
I'll bring the option back once the issue will be solved.

lexorus added some commits Oct 15, 2018

Disable cache for carthage bootsrap in circleci job configs
It seems like the circleci is using cached dependencies that are compiled
with older version of swift. Hopefully disabling the cache builds and
forcing cartahge to rebuild dependencies with new swift version will
solve the problem.
@SD10

This comment has been minimized.

Member

SD10 commented Oct 15, 2018

@lexorus Looks like CI is still failing. That aside, I definitely think we should keep the cached builds. There used to be a rebuild without cache option on Circle CI but I'm not sure where it went now that we switched to Circle CI 2.0 🤔

Btw, are you using Moya at Crunchyroll? I only ask because... I ❤️ Crunchyroll 😆

@lexorus lexorus force-pushed the lexorus:fix-warnings branch from 61c23ae to 7ca3064 Oct 15, 2018

@freak4pc

This comment has been minimized.

Member

freak4pc commented Oct 15, 2018

Didn’t review this yet - but why are your removing SwiftLint from the test target ?

@lexorus

This comment has been minimized.

Member

lexorus commented Oct 16, 2018

@SD10

@lexorus Looks like CI is still failing. That aside, I definitely think we should keep the cached builds. There used to be a rebuild without cache option on Circle CI but I'm not sure where it went now that we switched to Circle CI 2.0 🤔

I disabled the cache only to force the build machine to rebuild the dependencies. After that I reverted the changes, but the cache is now updated and rebuilt with the new specified Xcode.

About the Crunchyroll, we are not using Moya in Crunchyroll, but we are using some abstraction around it in our other product, VRV. And may consider migration of the CR to that abstraction some day.

@lexorus

This comment has been minimized.

Member

lexorus commented Oct 16, 2018

@freak4pc There was some warnings related to the file length in test files, if I remember correctly. This is not really a good check for the tests. Adding swiftlint disable comments in testing files also isn't a really good solution. And, from my practice, we are usually disabling the Swiftlint for the test target.

But, I can propose another solution for this if you really want to persist the checks. I guess we can try to add different .swiftlint.yml config file for test target, and specify it in the build phases.

I'm also open for any other solutions.

@freak4pc

This comment has been minimized.

Member

freak4pc commented Oct 16, 2018

from my practice, we are usually disabling the Swiftlint for the test target.

I think it's not correct to bring in personal intents / habits into an open-source projects. When so many people touch this code base, the only way to make a unified coding-style is to have a proper lint tool.

The questions that needs asking are:

  1. Is that line of code actually too long?
  2. If not, is it happening a ton of times?
  3. If it does, maybe a .swiftlint.yml specific to the test target would work.

Removing it altogether seems wrong to me.

@lexorus

This comment has been minimized.

Member

lexorus commented Oct 16, 2018

@freak4pc This wasn't a personal intent. More like a fast possible fix. I'll recheck that and come with an update. Thanks for the feedback. Can you please also review the other changes?

@SD10

This comment has been minimized.

Member

SD10 commented Oct 16, 2018

Good thoughts here everyone 👍 I think @freak4pc is correct that we should not disable the SwiftLint for the Tests directory. @lexorus Would you mind reverting that change?

Disabling the warnings via comments seems to be the strategy we’ve used in the past and I’m fine continuing to do that moving forward. If there’s a better solution we can take care of it with another PR.

The SwiftLint errors in the test target are also not caused by this PR so I don’t think they need to be resolved in this specific PR

Sent with GitHawk

@lexorus

This comment has been minimized.

Member

lexorus commented Oct 16, 2018

@SD10 @freak4pc Reverted the change in .swiftlint.yml as you requested. I'm ok if we'll take care of this in a separate PR.

@lexorus

This comment has been minimized.

Member

lexorus commented Oct 16, 2018

The CI failed:

** BUILD INTERRUPTED **
Too long with no output (exceeded 10m0s)

Would be nice if somebody with permissions will Rerun workflow

SD10 added some commits Oct 18, 2018

@SD10

SD10 approved these changes Oct 18, 2018

LGTM 👍 I'm just going to wait for CI to go green again

@freak4pc

Looks good! Good thought on switching all those props to public. I imagine no one would be overriding these anyways.

@freak4pc freak4pc merged commit 4c44b35 into Moya:development Oct 18, 2018

2 checks passed

ci/circleci: build Your tests passed on CircleCI!
Details
ci/circleci: carthage_without_swiftlint_integration Your tests passed on CircleCI!
Details
@peril-moya

This comment has been minimized.

peril-moya bot commented Oct 18, 2018

@lexorus Thanks a lot for contributing to Moya! We've invited you to join
the Moya GitHub organization – no pressure to accept! If you'd like more
information on what that means, check out our contributor guidelines and
feel free to reach out to @Moya/core-team with any questions.

Generated by 🚫 dangerJS

@SD10 SD10 referenced this pull request Oct 22, 2018

Merged

Fix unit test build failed #1746

@SD10 SD10 referenced this pull request Nov 2, 2018

Merged

[Release] Moya 12.0 #1753

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment