showBuildSettings does not require to call clean #8921

punty · 2017-04-19T21:09:13Z

Checklist

[x ] I've run bundle exec rspec from the root directory to see all new and existing tests pass
[x ] I've followed the fastlane code style and run bundle exec rubocop -a to ensure the code style is valid
[x ] I've read the Contribution Guidelines
[ x] I've updated the documentation if necessary.

Description

In show building settings, don't call xcodebuild clean, because it's not a necessary workaround anymore.

Motivation and Context

This change will allow the user to do not clean the build (unless specified in the following commands).
Previously for example event setting clean: false, will result in callling clean because of the generated
xcodebuild clean -showBuildSettings

As stated in the comment this was a workaround for an xcodebuild bug. But this bug has being fixed in 8.3 so it's not necessary anymore. https://developer.apple.com/library/content/releasenotes/DeveloperTools/RN-Xcode/Chapters/Introduction.html
As huge benefit, I can keep using fastlane with my CI, and for example, caching pod builds, and reduce my build time significantly.

This pull request also fixes #8099

….3 because Xcode bug has being fixed and this workaround is not required anymore

hjanuschka

🚀 looking promising, left a couple of comments
many thx for your contribution @punty

hjanuschka · 2017-04-20T06:16:06Z

fastlane_core/spec/project_spec.rb

@@ -299,7 +299,12 @@ def within_a_temp_dir
      it "SUPPORTED_PLATFORMS should be iphonesimulator iphoneos" do
        options = { project: "./fastlane_core/spec/fixtures/projects/Example.xcodeproj" }
        @project = FastlaneCore::Project.new(options, xcodebuild_list_silent: true, xcodebuild_suppress_stderr: true)
-        expect(FastlaneCore::Project).to receive(:run_command).with("xcodebuild clean -showBuildSettings -project ./fastlane_core/spec/fixtures/projects/Example.xcodeproj 2> /dev/null", { timeout: 10, retries: 3, print: false }).and_return(File.read("./fastlane_core/spec/fixtures/projects/build_settings_with_toolchains"))
+        if FastlaneCore::Helper.xcode_higher_than_8_3?


i think this should be splitted up into 2 tests. with mocked return of xcode_atleast on time with false and one time with true to be sure that it works on old and new xcodes.

the current variation is depending on the local xcode installation.

hjanuschka · 2017-04-20T06:16:49Z

fastlane_core/lib/fastlane_core/helper.rb

@@ -213,6 +213,13 @@ def self.keychain_path(name)
      keychain_path
    end

+    # @return true if XCode version is higher than 8.3
+    def self.xcode_higher_than_8_3?


can we rename this to: self.xcode_atleast?(version) - so we can use this method for variuous version checks of xcode?

hjanuschka · 2017-04-20T06:17:08Z

fastlane_core/lib/fastlane_core/helper.rb

+    def self.xcode_higher_than_8_3?
+      FastlaneCore::UI.user_error!("Unable to locate Xcode. Please make sure to have Xcode installed on your machine") if xcode_version.nil?
+      v = xcode_version
+      Gem::Version.new(v) >= Gem::Version.new('8.3.0')


and use it here: Gem::Version.new(v) >= Gem::Version.new(version)

hjanuschka · 2017-04-20T06:17:50Z

gym/lib/gym/xcode.rb

-        UI.user_error!("Unable to locate Xcode. Please make sure to have Xcode installed on your machine") if xcode_version.nil?
-        v = xcode_version
-        Gem::Version.new(v) >= Gem::Version.new('8.3.0')
+        FastlaneCore::Helper.xcode_higher_than_8_3?


then use FastlaneCore::Helper.xcode_atleast?("8.3")

- Update Unit Testing

punty · 2017-04-20T09:40:59Z

@hjanuschka I implemented the changes you requested! Much better!

hjanuschka · 2017-04-20T13:05:38Z

fastlane_core/spec/project_spec.rb

        expect(@project.build_settings(key: "SUPPORTED_PLATFORMS")).to eq("iphonesimulator iphoneos")
      end
+      unless FastlaneCore::Helper.xcode_atleast?('8.3')


can we get rid of the unless, and mock the result of xcode_atleast to 8.1?
removing the unless and turning the allow to an expect should do it, the allow should also be changed in the above test. so we always no matter which local xcode is installed verify it works on 8.1 and 8.3

or i am i wrong?

thx for your patience.

I am not sure about this. According to the official Apple documentation, the XCode bug has being fixed in 8.3, so if we are running this test, by mocking the call to return true, we will end up calling showBuildSettings without the clean command (even for XCode < 8.3). So in this case there is a risk this command will hangs?
According to Apple this happens only if the project contains Core Data. But I still think we should include test for scenarios that are not possible.

Allow is used for Mocking, https://www.relishapp.com/rspec/rspec-mocks/v/2-14/docs/method-stubs so when I call that method i return true or false

the command is not really run in the tests.
expect(FastlaneCore::Project).to receive(:run_command).with(command.to_s, { timeout: 10, retries: 3, print: false }).and_return(File.read("./fastlane_core/spec/fixtures/projects/build_settings_with_toolchains")) mocks it.

the goal should be to test the following two scenarios.

user has xcode < 8.3

force xcode_atleast?("8.3") to return false

expect the resulting command, to include clean

user has xcode >= 8.3

force xcode_atleast?("8.3") to return true

expect the resulting command, to NOT include clean

by forcing the result we bypass the local installation interfering the unit test.

thanks for the clarification. I will have a look at that!

hjanuschka · 2017-04-20T13:06:43Z

@punty awesome, looking solid 👍 thx for your work and patience, one comment left.

punty · 2017-04-20T13:48:27Z

@hjanuschka thanks again for your suggestions! I implemented the changes you requested

KrauseFx · 2017-04-20T19:27:09Z

fastlane_core/lib/fastlane_core/helper.rb

@@ -213,6 +213,13 @@ def self.keychain_path(name)
      keychain_path
    end

+    # @return true if XCode version is higher than 8.3
+    def self.xcode_atleast?(version)


xcode_at_least?

KrauseFx · 2017-04-20T19:27:22Z

fastlane_core/spec/project_spec.rb

@@ -296,10 +296,21 @@ def within_a_temp_dir
    end

    describe "build_settings() can handle empty lines" do
-      it "SUPPORTED_PLATFORMS should be iphonesimulator iphoneos" do
+      it "SUPPORTED_PLATFORMS should be iphonesimulator iphoneos on XCode >= 8.3" do


KrauseFx · 2017-04-20T19:27:44Z

fastlane_core/lib/fastlane_core/project.rb

-      # The 'clean' portion of this command is a workaround for an xcodebuild bug with Core Data projects.
-      # See: https://github.com/fastlane/fastlane/pull/5626
-      command = "xcodebuild clean -showBuildSettings #{xcodebuild_parameters.join(' ')}"
+      if FastlaneCore::Helper.xcode_atleast?('8.3')


Could you reference in a command on why this was added, when it was fixed, etc.

- add more comments and fix some typos

punty · 2017-04-20T20:49:08Z

@KrauseFx thanks for your feedback. I just implemented the changes you requested.

.

hjanuschka · 2017-04-21T17:49:09Z

@punty 🚀 merged, many thx for your contribution 🎉

fastlane-bot · 2017-04-22T13:00:32Z

Congratulations! 🎉 This was released as part of fastlane 2.28.3 🚀

don’t clean when show build setting if Xcode version is higher than 8…

aaf0958

….3 because Xcode bug has being fixed and this workaround is not required anymore

googlebot added the cla: yes label Apr 19, 2017

punty mentioned this pull request Apr 19, 2017

-showBuildSettings does not require to call clean #8914

Closed

hjanuschka requested changes Apr 20, 2017

View reviewed changes

punty added 2 commits April 20, 2017 10:18

- Update version check for XCode

6cc1e00

- Update Unit Testing

Fix unit test

b8a3689

hjanuschka reviewed Apr 20, 2017

View reviewed changes

- Fix unit tests

7ac99c1

KrauseFx reviewed Apr 20, 2017

View reviewed changes

KrauseFx previously requested changes Apr 20, 2017

View reviewed changes

punty added 2 commits April 20, 2017 21:06

- replace xcode_atleast with xcode_at_least

0268ef2

- add more comments and fix some typos

fix comment

5773d92

hjanuschka approved these changes Apr 20, 2017

View reviewed changes

hjanuschka merged commit 18322ef into fastlane:master Apr 21, 2017

mpirri mentioned this pull request Apr 21, 2017

Version bump to 2.28.3 #8961

Merged

fastlane-bot added the status: released label Apr 22, 2017

punty deleted the fix-not-necessary-clean branch April 24, 2017 08:15

fastlane locked and limited conversation to collaborators Jul 23, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

showBuildSettings does not require to call clean #8921

showBuildSettings does not require to call clean #8921

punty commented Apr 19, 2017

hjanuschka left a comment

hjanuschka Apr 20, 2017

hjanuschka Apr 20, 2017 •

edited

hjanuschka Apr 20, 2017

hjanuschka Apr 20, 2017

punty commented Apr 20, 2017

hjanuschka Apr 20, 2017

punty Apr 20, 2017

punty Apr 20, 2017

hjanuschka Apr 20, 2017

punty Apr 20, 2017

hjanuschka commented Apr 20, 2017

punty commented Apr 20, 2017

KrauseFx Apr 20, 2017

KrauseFx Apr 20, 2017

KrauseFx Apr 20, 2017

punty commented Apr 20, 2017

hjanuschka commented Apr 21, 2017

fastlane-bot commented Apr 22, 2017

showBuildSettings does not require to call clean #8921

showBuildSettings does not require to call clean #8921

Conversation

punty commented Apr 19, 2017

Checklist

Description

Motivation and Context

hjanuschka left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hjanuschka Apr 20, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

punty commented Apr 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

user has xcode < 8.3

user has xcode >= 8.3

Choose a reason for hiding this comment

hjanuschka commented Apr 20, 2017

punty commented Apr 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

punty commented Apr 20, 2017

hjanuschka commented Apr 21, 2017

fastlane-bot commented Apr 22, 2017

hjanuschka Apr 20, 2017 •

edited