Resolve variable substitution for xcconfig declared build settings #501

UnsafePointer · 2017-07-14T13:49:45Z

Currently, given an xcconfig like:

PRODUCT_BUNDLE_IDENTIFIER_Release = com.cocoapods.app
PRODUCT_BUNDLE_IDENTIFIER_Debug = com.cocoapods.app.dev
PRODUCT_BUNDLE_IDENTIFIER = $(PRODUCT_BUNDLE_IDENTIFIER_$(CONFIGURATION))

Using XCBuildConfiguration#resolve_build_setting for PRODUCT_BUNDLE_IDENTIFIER would return $(PRODUCT_BUNDLE_IDENTIFIER_$(CONFIGURATION)). With this change, it returns the proper value com.cocoapods.app or com.cocoapods.app.dev depending of the configuration.

UnsafePointer · 2017-07-14T13:50:40Z

lib/xcodeproj/project/object/build_configuration.rb

+          expression = /\$\((.*)\)/
+          match_data = config_setting.match(expression)
+          if match_data.nil?
+            return name if config_setting.eql?('CONFIGURATION')


Not sure if this is an exhaustive way to resolve the value, would love feedback on this.

segiddins

Does this only change the behavior when $(CONFIGURATION) needs to be expanded?

segiddins · 2017-07-14T14:19:04Z

lib/xcodeproj/project/object/build_configuration.rb

@@ -82,6 +82,7 @@ def type
        def resolve_build_setting(key)
          setting = build_settings[key]
          config_setting = base_configuration_reference && config[key]
+          config_setting = resolve_variable_substitution(config_setting) if !config_setting.nil? && config_setting.is_a?(String)


why do we need to check is_a?(String)?

xcconfig file parsing goes through XCBuildConfiguration#normalize_array_settings and we get array values from there. I'm completely ignoring those, as I believe it's mostly used for OTHER_LDFLAGS, but I don't fully understand why those have to be arrays, I assume this is expected in other parts of system.

UnsafePointer · 2017-07-14T14:37:38Z

@segiddins not sure if I get the question correctly. Previously $(CONFIGURATION) was not expanded at all (at least not by this method), now it is correctly expanded; so yes, it does.

segiddins · 2017-07-14T16:21:55Z

But does it effect any other setting names?

…red build settings

UnsafePointer · 2017-07-17T09:23:45Z

@segiddins Yes. It basically should capture anything between {()} and try to resolve the value of that, respecting the level rules in XCBuildConfiguration#resolve_build_setting, if no value is found it will just fallback to the config name.

Sorry about the rebase, someone reported a problem with my implementation, which should be fixed now. I'll add more tests to try to cover all the different scenarios.

segiddins · 2017-07-17T15:38:54Z

This looks reasonable to me. 👍
I think I'd like @samdmarshall to chime in if there are known corner cases we should test against for variable expansion, and @dantoml to review the changes.
Thanks!

samdmarshall · 2017-07-17T16:34:10Z

The major thing to watch out for is that the variables you are attempting to expand are actually valid as xcconfig identifiers. Otherwise you are going to run into problems with invalid xcconfig files and people wondering why their unicode configuration names aren't working as expected.

…g variable substitution

UnsafePointer · 2017-07-18T16:56:23Z

Some changes I did after the last review:

Applied @samdmarshall suggestion, the regex now looks only for valid xcconfig identifiers.
Made the regex lazy so cases like $(USER_DEFINED)_$(CONFIGURATION) work now.
Added more tests.

Thanks to the feedback on fastlane/fastlane#9760 (comment) I realised that there's a problem with this approach:

If a build setting defined at a project level uses variable substitution referencing values at a target level the method won't work. I'm still looking into this. Ideally I'd like to jump to the target level from this point (if self == project.build_configuration_list[name]), but not sure if this is possible without passing a reference of the target from the root of the recursion which is something I want to avoid.

Update:

I think that problem can be avoided by using PBXNativeTarget#resolved_build_setting which will lookup on both levels separately and resolve the value. Will try this now.

…tween project and targets

UnsafePointer · 2017-08-07T13:18:20Z

@segiddins @dantoml anything else I can do here to move forward this?

segiddins

Looks 👍 other than the minor comments

segiddins · 2017-08-11T13:14:47Z

lib/xcodeproj/project/object/build_configuration.rb

          setting = build_settings[key]
+          setting = resolve_variable_substitution(setting, root_target) if !setting.nil? && setting.is_a?(String)


checking both nil? and is_a? is redundant

Makes totally sense!

segiddins · 2017-08-11T13:14:53Z

lib/xcodeproj/project/object/build_configuration.rb

          config_setting = base_configuration_reference && config[key]
+          config_setting = resolve_variable_substitution(config_setting, root_target) if !config_setting.nil? && config_setting.is_a?(String)


checking both nil? and is_a? is redundant

Makes totally sense!

segiddins · 2017-08-11T13:15:21Z

lib/xcodeproj/project/object/build_configuration.rb

@@ -103,6 +108,20 @@ def expand_build_setting(build_setting_value, config_value)
          build_setting_value.map { |value| Constants::INHERITED_KEYWORDS.include?(value) ? inherited : value }.flatten
        end

+        def resolve_variable_substitution(config_setting, root_target)
+          expression = /\$[{(]([^inherited][$(){}_a-zA-Z0-9]*?)[})]/


extract this into a constant and please comment the regexp

…pression to local class constant

segiddins · 2017-08-18T02:30:19Z

I'll let @dantoml merge this one

endocrimes · 2017-08-18T02:31:40Z

lib/xcodeproj/project/object/build_configuration.rb

@@ -96,13 +101,42 @@ def resolve_build_setting(key)

        private

+        CAPTURE_VARIABLE_IN_BUILD_CONFIG = /


thanks for documenting the regex 👍

litalhassine · 2017-09-06T09:12:58Z

Any plans for a new release with this commit?

dnkoutso · 2017-09-06T13:55:39Z

@litalhassine you can use http://bundler.io/ and point to a specific SHA or branch to use this.

UnsafePointer · 2017-09-06T15:47:57Z

@dnkoutso it would be really nice to have this as a patch release (probably nobody is using that API anyway) so we could submit a fix for the issue described here: fastlane/fastlane#9460 (comment).

jetersen · 2017-09-29T08:43:00Z

Thanks this resolved one of the issue I had in my sample for environment variables substitution
https://github.com/casz/xcodeproj-sample-env
the case The subtitues variables that are resolved by xcode build settings
written because I saw this PR and wanted to see that it kept working after environment variables variable substitutes was in xcodeproj 👍

So it did not work with xcodeproj 1.5.1

However it works in 1.5.2, great work 🚀

epatel · 2017-10-21T16:05:31Z

This don't seem to work for me. I am using fastlane and it always fails on export now, did work earlier though (Xcode8? Xcode9 now).

I have $(TEST_GROUP) that can be defined as a couple of test groups and then a number of other variables, in a BUNDLE_ID that is a concatenation like $($(TEST_GROUP)_BUNDLE_ID) which then gets i.e. from EXTERNAL_BUNDLE_ID = com.myapp.external if TEST_GROUP is set to EXTERNAL.

I see $($(TEST_GROUP)_BUNDLE_ID) becomes TEST_GROUP_BUNDLE_ID

This worked for a while and now it does not...
http://dyna.mo/xcconfig/

UnsafePointer · 2017-11-10T08:55:05Z

@epatel I'm happy to have a look if you could open an issue with a sample project to reproduce this.

UnsafePointer commented Jul 14, 2017

View reviewed changes

segiddins reviewed Jul 14, 2017

View reviewed changes

UnsafePointer mentioned this pull request Jul 14, 2017

Resolve PRODUCT_BUNDLE_IDENTIFIER and PROVISIONING_PROFILE_SPECIFIER for Xcode 9 export settings fastlane/fastlane#9760

Merged

4 tasks

Renzo Crisóstomo added 2 commits July 17, 2017 11:12

[BuildConfiguration] Resolve variable substitution for xcconfig decla…

ba03682

…red build settings

Add CHANGELOG entry for #501

3738989

Renzo Crisóstomo added 2 commits July 18, 2017 14:19

[BuildConfiguration] Add more tests to xcconfig variable substitution

e348184

[BuildConfiguration] Make regular expression lazy to support appendin…

b7aa0ee

…g variable substitution

[BuildConfiguration] Add root_target to resolve complete recursion be…

7f4ff33

…tween project and targets

segiddins reviewed Aug 11, 2017

View reviewed changes

[BuildConfiguration] Remove redundant nil? checks, extract regular ex…

32d4e8d

…pression to local class constant

endocrimes approved these changes Aug 18, 2017

View reviewed changes

endocrimes merged commit 84ebf06 into CocoaPods:master Aug 18, 2017

UnsafePointer mentioned this pull request Sep 14, 2017

Rollback https://github.com/fastlane/fastlane/pull/9760 fastlane/fastlane#9850

Merged

thasegaw mentioned this pull request Sep 22, 2017

[gym] (Xcode 9 b6) Generated exportOptionsPlist bundle identifiers contain variables instead of actual bundle identifiers fastlane/fastlane#10315

Closed

4 tasks

UnsafePointer mentioned this pull request Oct 26, 2017

SystemStackError after MacOS & Xcode upgrade #511

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Resolve variable substitution for xcconfig declared build settings #501

Resolve variable substitution for xcconfig declared build settings #501

UnsafePointer commented Jul 14, 2017

UnsafePointer Jul 14, 2017

segiddins left a comment

segiddins Jul 14, 2017

UnsafePointer Jul 14, 2017

UnsafePointer commented Jul 14, 2017

segiddins commented Jul 14, 2017

UnsafePointer commented Jul 17, 2017 •

edited

Loading

segiddins commented Jul 17, 2017

samdmarshall commented Jul 17, 2017

UnsafePointer commented Jul 18, 2017 •

edited

Loading

UnsafePointer commented Aug 7, 2017

segiddins left a comment

segiddins Aug 11, 2017

UnsafePointer Aug 14, 2017

segiddins Aug 11, 2017

UnsafePointer Aug 14, 2017

segiddins Aug 11, 2017

UnsafePointer Aug 14, 2017

segiddins commented Aug 18, 2017

endocrimes Aug 18, 2017

litalhassine commented Sep 6, 2017

dnkoutso commented Sep 6, 2017

UnsafePointer commented Sep 6, 2017

jetersen commented Sep 29, 2017 •

edited

Loading

epatel commented Oct 21, 2017 •

edited

Loading

UnsafePointer commented Nov 10, 2017

		setting = build_settings[key]
		setting = resolve_variable_substitution(setting, root_target) if !setting.nil? && setting.is_a?(String)

		config_setting = base_configuration_reference && config[key]
		config_setting = resolve_variable_substitution(config_setting, root_target) if !config_setting.nil? && config_setting.is_a?(String)

		@@ -96,13 +101,42 @@ def resolve_build_setting(key)

		private

		CAPTURE_VARIABLE_IN_BUILD_CONFIG = /

Resolve variable substitution for xcconfig declared build settings #501

Resolve variable substitution for xcconfig declared build settings #501

Conversation

UnsafePointer commented Jul 14, 2017

Choose a reason for hiding this comment

segiddins left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

UnsafePointer commented Jul 14, 2017

segiddins commented Jul 14, 2017

UnsafePointer commented Jul 17, 2017 • edited Loading

segiddins commented Jul 17, 2017

samdmarshall commented Jul 17, 2017

UnsafePointer commented Jul 18, 2017 • edited Loading

UnsafePointer commented Aug 7, 2017

segiddins left a comment

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

segiddins commented Aug 18, 2017

Choose a reason for hiding this comment

litalhassine commented Sep 6, 2017

dnkoutso commented Sep 6, 2017

UnsafePointer commented Sep 6, 2017

jetersen commented Sep 29, 2017 • edited Loading

epatel commented Oct 21, 2017 • edited Loading

UnsafePointer commented Nov 10, 2017

UnsafePointer commented Jul 17, 2017 •

edited

Loading

UnsafePointer commented Jul 18, 2017 •

edited

Loading

jetersen commented Sep 29, 2017 •

edited

Loading

epatel commented Oct 21, 2017 •

edited

Loading