search

bazel-ios / cocoapods-bazel

A Cocoapods plugin for automatically generating Bazel BUILD files
Apache License 2.0
110 stars 21 forks source link

Do not remove a xcconfig entry if value is empty #54

Closed gyfelton closed 2 years ago

gyfelton commented 2 years ago

Why this change

Because each key will be used in build setting side as substitution, if we just remove the key if value is empty, we will end up not having the substitution to work with in the first place. Here we simply preserve the key value pairs with empty values.

Tests done

  1. rspec to red-green test
  2. integartion tests that red-green
  3. tested with downstream project that is affected by this and indeed it works.

Also did the following changes:

  1. Remove xcode-select step as it was not working and I don't think any integration tests here depends on a specific xcode version?
  2. Update gitignore, and test.yml for experiement_feature integartion spec
gyfelton commented 2 years ago

I'm a little confused by the PR description. It would be helpful to include a concrete example of what was broken by this and why. The question I was left wondering is if we couldn't add the missing key when needed in the upstream consumer. Having a setting set to an empty string isn't the same as having it not set. The former will stomp any Xcode default values.

Sorry to hear that, in the rspec there is a new EMPTY_CONFIG that is not preserved before the fix but now is. Correct that having a setting set will stomp any xcode default values, but in this fix the pre-condition is that the key is already present/set by podspec side so the author of podspc has the intention to set it to empty? I could not think of a case where we will accidentally write off other settings and I will run a CI job on the downstream project to ensure about it.

justinseanmartin commented 2 years ago

... in this fix the pre-condition is that the key is already present/set by podspec side so the author of podspc has the intention to set it to empty?

Sounds good, thanks for the clarification!

gyfelton commented 2 years ago

should have done more check but right now downstream gets lot of changes that might backfire when we don't omit the empty values. CI also fails on lots of tests. I start to think this omitting empty values is done intentionally and it's not really a good idea to fix it via this way. I think I should close this for now

gyfelton commented 2 years ago

Closing for now as this might do more harm than good without some proper testing. Will bring out the good changes out of this PR as another PR though