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

Fix errors when using mutually recursive build settings #727

Merged
merged 5 commits into from
Jan 11, 2020
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -13,10 +13,15 @@
* Properly serialize array settings when running `config-dump`.
[Samuel Giddins](https://github.com/segiddins)

* Add iMessage extensions.
* Add iMessage extensions.
[wade0317](https://github.com/wade0317)
[#723](https://github.com/CocoaPods/Xcodeproj/pull/723)

* Fix errors when using mutually recursive build settings.
[revolter](https://github.com/revolter)
[#727](https://github.com/CocoaPods/Xcodeproj/pull/727)


## 1.13.0 (2019-10-16)

##### Enhancements
Expand Down
35 changes: 29 additions & 6 deletions lib/xcodeproj/project/object/build_configuration.rb
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,10 @@ module Object
# {PBXProject} or a {PBXNativeTarget}.
#
class XCBuildConfiguration < AbstractObject
MUTUAL_RECURSION_SENTINEL = 'xcodeproj.mutual_recursion_sentinel'.freeze
Copy link
Member

Choose a reason for hiding this comment

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

Might actually be more performant to make this = Object.new.freeze. Also, I think it can be a private_constant?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  1. It doesn't work with Object.new.freeze (actually ::Object.new), because String specific methods are called on it, and so it crashes.
  2. TIL about private_constant.


private_constant :MUTUAL_RECURSION_SENTINEL

# @!group Attributes

# @return [String] the name of the Target.
Expand Down Expand Up @@ -79,16 +83,19 @@ def type
# the key of the build setting.
#
# @param [PBXNativeTarget] root_target
# use this to resolve complete recursion between project and targets
# use this to resolve complete recursion between project and targets.
#
# @param [String] previous_key
# use this to resolve complete recursion between different build settings.
#
# @return [String] The value of the build setting
#
def resolve_build_setting(key, root_target = nil)
def resolve_build_setting(key, root_target = nil, previous_key = nil)
Copy link
Member

Choose a reason for hiding this comment

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

is it always sufficient to have a single previous key, or do we need a list of all of them?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you mean for cases like this:

'mutually_recursive' => 'mn: ${mutually_recursive_nested}',
'mutually_recursive_nested' => 'm1: ${mutually_recursive_1} m2: ${mutually_recursive_1}',

?

Case in which both mutually_recursive and mutually_recursive_nested are resolved to nil:

@configuration.resolve_build_setting('mutually_recursive_nested').should.nil?
@configuration.resolve_build_setting('mutually_recursive').should.nil?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Or if you have another specific case in mind, I could make a test for it and see if it works.

setting = build_settings[key]
setting = resolve_variable_substitution(key, setting, root_target)
setting = resolve_variable_substitution(key, setting, root_target, previous_key)

config_setting = config[key]
config_setting = resolve_variable_substitution(key, config_setting, root_target)
config_setting = resolve_variable_substitution(key, config_setting, root_target, previous_key)

project_setting = project.build_configuration_list[name]
project_setting = nil if equal?(project_setting)
Expand All @@ -99,6 +106,12 @@ def resolve_build_setting(key, root_target = nil)
'SRCROOT' => project.project_dir.to_s,
}

# if previous_key is nil, it means that we're back at the first call, so we can replace our sentinel string
# used to prevent recursion with nil
if previous_key.nil? && setting == MUTUAL_RECURSION_SENTINEL
setting = nil
end

[defaults[key], project_setting, config_setting, setting, ENV[key]].compact.reduce(nil) do |inherited, value|
expand_build_setting(value, inherited)
end
Expand Down Expand Up @@ -142,7 +155,7 @@ def expand_build_setting(build_setting_value, config_value)
build_setting_value.flat_map { |value| Constants::INHERITED_KEYWORDS.include?(value) ? inherited : value }
end

def resolve_variable_substitution(key, value, root_target)
def resolve_variable_substitution(key, value, root_target, previous_key = nil)
case value
when Array
return value.map { |v| resolve_variable_substitution(key, v, root_target) }
Expand All @@ -168,9 +181,19 @@ def resolve_variable_substitution(key, value, root_target)
when key
# to prevent infinite recursion
nil
when previous_key
# to prevent infinite recursion; we don't return nil as for the self recursion because it
Copy link
Member

Choose a reason for hiding this comment

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

missing some words at the end?

Copy link
Contributor Author

@revolter revolter Jan 9, 2020

Choose a reason for hiding this comment

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

Dang, and I forgot what exactly I wanted to say. I pushed an update.

MUTUAL_RECURSION_SENTINEL
else
configuration_to_resolve_against = root_target ? root_target.build_configuration_list[name] : self
resolved_value_for_variable = configuration_to_resolve_against.resolve_build_setting(variable, root_target) || ''
resolved_value_for_variable = configuration_to_resolve_against.resolve_build_setting(variable, root_target, key) || ''

# we use this sentinel string instead of nil, because, otherwise, it would be swallowed by the default empty
# string from the preceding line, and we want to distinguish between mutual recursion and other cases
if resolved_value_for_variable == MUTUAL_RECURSION_SENTINEL
return MUTUAL_RECURSION_SENTINEL
end

value = value.gsub(variable_reference, resolved_value_for_variable)
resolve_variable_substitution(key, value, root_target)
end
Expand Down
6 changes: 4 additions & 2 deletions spec/project/object/build_configuration_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,8 @@ module ProjectSpecs
'inherited_suffix' => 'suffix',
'FOOS' => %w(a b c d ${i} ${inherited} ${I${e}} ${${i}E}),
'self_recursive' => 'hi $(self_recursive)',
'mutually_recursive' => 'm1: ${mutually_recursive_1} m2: ${mutually_recursive_1}',
'mutually_recursive' => 'mn: ${mutually_recursive_nested}',
'mutually_recursive_nested' => 'm1: ${mutually_recursive_1} m2: ${mutually_recursive_1}',
'mutually_recursive_1' => 'mr2=${mutually_recursive_2}',
'mutually_recursive_2' => 'mr1=${mutually_recursive_1}',
'mixes_braces_and_parens' => '${ab) $(cd}){})',
Expand Down Expand Up @@ -115,9 +116,10 @@ module ProjectSpecs
@configuration.resolve_build_setting('mixes_braces_and_parens').should == '${ab) $(cd}){})'
end

xit 'resolves mutually-recursive references to nil' do
it 'resolves mutually-recursive references to nil' do
@configuration.resolve_build_setting('mutually_recursive_1').should.nil?
@configuration.resolve_build_setting('mutually_recursive_2').should.nil?
@configuration.resolve_build_setting('mutually_recursive_nested').should.nil?
@configuration.resolve_build_setting('mutually_recursive').should.nil?
end
end
Expand Down