Creates a stage sandbox and sandbox dir cleaner step. #8339
Conversation
Here's an example of your CHANGELOG entry: * Creates a stage sandbox and sandbox dir cleaner step.
[sebastianv1](https://github.com/sebastianv1)
[#issue_number](https://github.com/CocoaPods/CocoaPods/issues/issue_number)note: There are two invisible spaces after the entry's text. Generated by 🚫 Danger |
lib/cocoapods/installer.rb
Outdated
| # | ||
| def stage_sandbox(sandbox, pod_targets, aggregate_targets) | ||
| SandboxHeaderPathsInstaller.new(sandbox, pod_targets).install! | ||
| clean_sandbox(pod_targets, aggregate_targets) |
There was a problem hiding this comment.
I'm thinking this should go somewhere else since it will change with the incremental installation since it will go after cache analysis.
There was a problem hiding this comment.
And SandboxHeaderPathsInstaller needs to go before analysis such that all the header search paths are set up correctly for the build settings
There was a problem hiding this comment.
Ended up redoing the generate_pods_project function. Let me know what y'all think
1e33279 to
7b77c7d
Compare
a69d7cb to
2f3e199
Compare
2f3e199 to
f2fa5ec
Compare
| mappings[sub_dir] << header | ||
| end | ||
| mappings | ||
| end |
There was a problem hiding this comment.
not that this is an issue, but note that this will now be part of the public API of PodTarget - does it need to be? it feels like something the sandbox should handle
There was a problem hiding this comment.
That's a good point. I'm open to hearing what others think about moving it into the sandbox. I don't really have a strong preference about where it lives, I just moved it here since everywhere we needed the header mappings we were already iterating over the pod targets, so it made sense in my head at the time to just put it in here.
There was a problem hiding this comment.
This might explode a bit but passing file_accessor does not make sense as the pod_target is already a holder and owner of file_accessors.
There was a problem hiding this comment.
So header_mappings won't actually be a part of the public API of PodTarget since it's under private. It takes in a file_accessor since it's more of a helper used by the public functions public_header_mappings_by_file_accessor and header_mappings_by_file_accessor
f2fa5ec to
0a5ccd5
Compare
0a5ccd5 to
f939393
Compare
| attr_reader :sandbox | ||
|
|
||
| # @return [Array<PodTarget>] | ||
| # The list of all pod targets that will be installed into the Sandbox. |
There was a problem hiding this comment.
is this correct or copy pasta?
There was a problem hiding this comment.
Correct in that I wrote it :) The list of pod targets that will be installed are used to compute which folders need to be deleted.
lib/cocoapods/installer.rb
Outdated
| end | ||
|
|
||
| aggregate_targets.each do |aggregate_target| | ||
| target_support_dirs.delete(aggregate_target.support_files_dir) | ||
| FileUtils.rm_rf(aggregate_target.support_files_dir) |
There was a problem hiding this comment.
this is actually a pretty big change in functionality here -- before, we'd only rm_rf the support_files_dirs for pods that aren't being installed, and now you always remove them. I don't think we want that
There was a problem hiding this comment.
Can remove. Any reason why we don't want that if the files are regenerated anyways? This has the possibility of leaving behind stale files. Say we change a Pod to use modular_headers and then revert back. We'd then leave behind a stale modulemap file.
There was a problem hiding this comment.
Because it means we have to rewrite the files. Right now, we only actually write to disk if there's a change
| @@ -318,22 +339,17 @@ def validate_build_configurations | |||
| # regenerated from scratch and any file which might not be | |||
| # overwritten. | |||
| # | |||
| # @todo [#247] Clean the headers of only the pods to install. | |||
| # | |||
| def clean_sandbox | |||
There was a problem hiding this comment.
all the old functionality now seems to be replicated in SandboxDirCleaner#clean!
There was a problem hiding this comment.
Sets up incremental since clean_sandbox will only take in a subset of all pod targets to clean.
f939393 to
6830327
Compare
The `stage_sandbox` step during installation adds all the respective header search paths to sandbox `HeaderStore` and each pod target's `HeaderStore`. These were previously only added during the `link_headers` step during project generation, and moving them out into an earlier step before generation helps set up the work for incremental pod installation because we would like to analyze each pod target's build settings (and header search paths) before project generation. The `SandboxDirCleaner` object also sets up incremental pod installation because we don't want to delete the entire `Pods/Headers` and `Pods/Target Support Files` directories for every installation.
6830327 to
3eaff05
Compare
Inside stage sandbox we now add all the respective header search paths to the sandbox
HeaderStoreand each pod target'sHeaderStore. This sets up the work for incremental pod installation because we would like to analyze each pod target's build settings to determine if it has changed; however, we previously only added the header search paths during installation which is too late in the pipline.Instead of removing the entire Headers and Target Support Files directory in the Sandbox for every installation, we now only remove the headers and target support files that are stale.
integration specs: CocoaPods/cocoapods-integration-specs#197