Skip to content

Conversation

@spencertransier
Copy link
Contributor

@spencertransier spencertransier commented Mar 27, 2023

What does it do?

This action adds an action called ios_get_build_number to get the current build number from an xcconfig file. Because Day One iOS and Mac use the build number to name builds, this new action is needed for things like GitHub release names and tags.

How to test

  1. In the bloom/DayOne-Apple repo, switch to the spencer/demo-get-build-number-action branch
  2. Run bundle exec fastlane ios test_get_build_number_action
  3. The output should be "The build number is 2392"
  4. Run bundle exec fastlane mac test_get_build_number_action
  5. The output should be "The build number is 1471"

Checklist before requesting a review

  • Run bundle exec rubocop to test for code style violations and recommendations
  • Add Unit Tests (aka specs/*_spec.rb) if applicable
  • Run bundle exec rspec to run the whole test suite and ensure all your tests pass
  • Make sure you added an entry in the CHANGELOG.md file to describe your changes under the approprioate existing ### subsection of the existing ## Trunk section.

@spencertransier spencertransier requested a review from a team March 27, 2023 18:46
@spencertransier spencertransier marked this pull request as ready for review March 27, 2023 18:46
# @return [String] The current build number according to the public xcconfig file.
#
def self.get_build_number
read_build_number_from_config_file(ENV['PUBLIC_CONFIG_FILE'])
Copy link
Contributor

Choose a reason for hiding this comment

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

Have you considered passing the file as an argument?

See discussion in #380 and the work done in #445.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mokagio Ready for re-review. It now uses an argument instead of the env variable

@spencertransier spencertransier requested a review from mokagio April 20, 2023 16:21
Comment on lines +16 to +24
it 'parses an xcconfig file with keys with spaces and returns a nil build number' do
xcconfig_mock_content = <<~CONTENT
VERSION_SHORT = 6
VERSION_LONG = 6.30.1
BUILD_NUMBER = 1940
CONTENT

expect_build_number(xcconfig_mock_content: xcconfig_mock_content, expected_build_number: nil)
end
Copy link
Contributor

Choose a reason for hiding this comment

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

I was surprised by this because key = value is a valid xcconfig syntax. Looking at the pre-existing, underlying implementation, I can see why this is required given that we manually parse the xcconfig files

# Read the value of a given key from an `.xcconfig` file.
#
# @param [String] key The xcconfig key to get the value for
# @param [String] filePath The path to the `.xcconfig` file to read the value from
#
# @return [String] The value for the given key, or `nil` if the key was not found.
#
def self.read_from_config_file(key, filePath)
File.open(filePath, 'r') do |f|
f.each_line do |line|
line = line.strip()
return line.split('=')[1] if line.start_with?("#{key}=")
end
end
return nil
end

We should be able to replace that bespoke logic with the API provided by the xcodeproj gem like we do in Jetpack and WordPress iOS:

version_config_path = File.join(PROJECT_ROOT_FOLDER, 'config', 'Version.internal.xcconfig')
versions = Xcodeproj::Config.new(File.new(version_config_path)).to_hash

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It looks like @iangmaia already started working on using xcodeproj instead of the manual parsing: #451

I decided to implement this as-is and update it as needed after Ian's PRs are fully merged into the project.

@spencertransier spencertransier merged commit 5cf95d2 into trunk Apr 27, 2023
@spencertransier spencertransier deleted the add/get-build-number-action branch April 27, 2023 03:15
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.

4 participants