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

Disable SPM caching in CI #1989

Merged
merged 12 commits into from
Aug 8, 2024
Merged

Conversation

mokagio
Copy link
Contributor

@mokagio mokagio commented Aug 6, 2024

This is a temporary workaround to address the SPM-resolution-related build failure in the Prototype Build step, example https://buildkite.com/automattic/pocket-casts-ios/builds/7634#019113ca-d4bd-4838-b93a-aae0e4a0528f.

The long term solution will be one of the two options proposed in #1990

While touching the CI scripts to implement the workaround, I also:

  • Centralized shared setup steps in a dedicated script
  • Removed an unnecessary install_cocoapods call
  • Upgraded the CI toolkit
  • Explicitly set the agents: queue: mac node in a few steps, to enable us to remove the implicit agents: queue: mac setting from the pipeline configuration at the Buildkite level, in turn letting us use a standard upload command which supports shared pipeline vars.

@pocketcasts
Copy link
Contributor

pocketcasts commented Aug 6, 2024

Pocket Casts Prototype Build📲 You can test the changes from this Pull Request in Pocket Casts Prototype Build by scanning the QR code below to install the corresponding build.
App NamePocket Casts Prototype Build Pocket Casts Prototype Build
Build Numberpr1989-2fd53e3
Version7.70
Bundle IDau.com.shiftyjelly.podcasts.prototype
Commit2fd53e3
App Center BuildPocket Casts - Prototype Builds #23
Automatticians: You can use our internal self-serve MC tool to give yourself access to App Center if needed.

@mokagio mokagio force-pushed the mokagio/workaround-double-package.resolved branch from 044c13e to 2fd53e3 Compare August 6, 2024 05:24
@mokagio mokagio changed the title Track xcodebuild -resolvePackageDependencies result Disable SPM caching in CI Aug 6, 2024
This will allow us to remove the implicit `agents: queue: mac` setting
from the pipeline configuration at the Buildkite level, in turn letting
us use a standard upload command which supports shared pipeline vars.
This is just so we are on up-to-date tools before making changes that
involve said tools.
@mokagio mokagio marked this pull request as ready for review August 6, 2024 06:31
@mokagio mokagio requested a review from a team as a code owner August 6, 2024 06:31
@mokagio mokagio requested review from leandroalonso and removed request for a team August 6, 2024 06:31
Comment on lines -8 to -9
export LANG=en_US.UTF-8
export LC_ALL=en_US.UTF-8
Copy link
Contributor

Choose a reason for hiding this comment

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

Do these need to be added to the shared_setup.sh script?

Copy link
Contributor

Choose a reason for hiding this comment

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

Good question. I wonder why we needed to export those… and if it's still needed though 🤔

I know that in general, those special env vars sometimes have impacts on some third-party toolds relying on the locale and that it's thus a good practice to set those to en_US.UTF-8 to fix various issues with those… But I have no idea:

  1. which of the tools we use might have required for us to add those in the past (would have been nice to add a comment back then to leave breadcrumbs 😓
  2. and thus if it's still necessary (maybe we don't use the tool that had an issue with this anymore, or maybe the CI agents we run our build.sh command on now has those env vars already set to en_US.UTF-8 at the system level?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They were legacy settings coming from the previous automation setup. Unfortunately I don't have additional context on the previous automation.

Those exports triggered some old memories from other projects though, so if I had to guess I'd say they are related to old CocoaPods requirements, e.g. CocoaPods/CocoaPods#6333

Copy link
Contributor

@AliSoftware AliSoftware left a comment

Choose a reason for hiding this comment

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

LGTM, with only nitpicks.

Approving to unblock, providing that we don't forget to follow-up with #1990 or any alternative long-term solution for it.

.buildkite/commands/shared_setup.sh Outdated Show resolved Hide resolved
Comment on lines -8 to -9
export LANG=en_US.UTF-8
export LC_ALL=en_US.UTF-8
Copy link
Contributor

Choose a reason for hiding this comment

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

Good question. I wonder why we needed to export those… and if it's still needed though 🤔

I know that in general, those special env vars sometimes have impacts on some third-party toolds relying on the locale and that it's thus a good practice to set those to en_US.UTF-8 to fix various issues with those… But I have no idea:

  1. which of the tools we use might have required for us to add those in the past (would have been nice to add a comment back then to leave breadcrumbs 😓
  2. and thus if it's still necessary (maybe we don't use the tool that had an issue with this anymore, or maybe the CI agents we run our build.sh command on now has those env vars already set to en_US.UTF-8 at the system level?)

@mokagio mokagio added this to the 7.71 milestone Aug 8, 2024
@mokagio mokagio added the [Type] Tooling Issues related to tooling: build tools, ruby, scripts, etc. label Aug 8, 2024
@mokagio mokagio enabled auto-merge August 8, 2024 10:48
@mokagio mokagio merged commit 5aaa4ab into trunk Aug 8, 2024
5 of 8 checks passed
@mokagio mokagio deleted the mokagio/workaround-double-package.resolved branch August 8, 2024 10:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Tooling Issues related to tooling: build tools, ruby, scripts, etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants