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

[XCBuildConfiguration] Support environment variables in #resolve_build_setting #510

Merged

Conversation

UnsafePointer
Copy link
Contributor

This should address #508.

@UnsafePointer UnsafePointer force-pushed the handle_environment_variables branch 2 times, most recently from 612d6d9 to dc9936f Compare October 4, 2017 14:34
@jetersen
Copy link

jetersen commented Oct 4, 2017

Absolutely awesome!
With this PR xcodeproj covers my use case completely.

bundle exec rspec

xcodeproj variables
  should still resolve xcode build settings
  subtitues variables that are resolved by xcode build settings
  substitues bundle identifiers env variable
  substitues debug env variable
  substitues release env variable

Finished in 0.17304 seconds (files took 0.17947 seconds to load)
5 examples, 0 failures

Output from sample project

What I did not test if it works fastlane and the chosen xcode scheme.
xcode scheme

Schemes can have environment variables added on each scheme.

@UnsafePointer
Copy link
Contributor Author

This won't address the scheme use case, but I believe as far as Xcodeproj goes, this is good enough for build settings resolution. Xcodeproj doesn't know about the scheme you're trying to use, so it's up to fastlane to read that scheme and set up the build environment with those variables before attempting to resolve build settings.

@segiddins what do you think of this?

@jetersen
Copy link

jetersen commented Oct 5, 2017

@Ruenzuo thought as much 👍 I'll test it and create an issue with fastlane if there's a issue once this is released.

Still somewhat baffled how simple the change was. 💯

@jetersen
Copy link

jetersen commented Oct 12, 2017

@dantoml Could we have a release with this, pretty please 👍
Minor change, and validated it works.

@target.resolved_build_setting('TARGET_REFERENCE_ENVIRONMENT', true).should == { 'Release' => 'ENVIRONMENT_VARIABLE_VALUE', 'Debug' => 'ENVIRONMENT_VARIABLE_VALUE' }
end

it 'returns the resolved build setting considering environment variables' do
Copy link
Contributor

Choose a reason for hiding this comment

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

this test has the same name as the one above

@dnkoutso
Copy link
Contributor

@Ruenzuo can you also rebase?

@endocrimes
Copy link
Member

I'm not 100% sure that this is something we should support in Xcodeproj, as it introduces a discrepancy between what gets resolved in ruby, and the model of the project.

@jetersen
Copy link

Most Xcodeproj build settings have distinct names to not conflict with any unintended environment variables.

@endocrimes
Copy link
Member

@Casz I'm not so worried about accidental conflict, as I am in adding discrepancies between what is resolved in Xcodeproj as is in the model and project itself.

@UnsafePointer
Copy link
Contributor Author

@dantoml I guess it all depends on what do we want to get from the API. Currently it reads:

# XCBuildConfiguration#resolve_build_setting
# Gets the value for the given build setting considering any configuration
# file present and resolving inheritance between them

You can always use XCBuildConfiguration#build_settings to get the real model, but what I understand from the API is that this method should try to mimic Xcode value resolution.

@tmspzz
Copy link

tmspzz commented Dec 13, 2017

Any plans on merging this?

@dnkoutso
Copy link
Contributor

@Ruenzuo can you rebase this PR please?

@UnsafePointer
Copy link
Contributor Author

@dnkoutso rebased

@dnkoutso
Copy link
Contributor

@dantoml any objections of merging this? Seems you had some initial concerns to land this.

@dnkoutso
Copy link
Contributor

@Ruenzuo one more rebase please and we can land this sorry, can you please fix the CHANGELOG and move the entry up to the master section?

@dnkoutso
Copy link
Contributor

ping @Ruenzuo one more time

@UnsafePointer
Copy link
Contributor Author

@dnkoutso rebased

@dnkoutso dnkoutso merged commit 80ee774 into CocoaPods:master Jan 23, 2018
@dnkoutso
Copy link
Contributor

@Ruenzuo thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants