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

fix(apple): include check if Podfile and Podfile.lock changed when deciding to install Cocoapods #2443

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

szymonrybczak
Copy link
Collaborator

Summary:

Comparing only dependencies field from config's command output takes into account only autolinked dependencies, however users can add manually Pods inside Podfile, in this Pull Request I added validating hash for Podfile and Podfile.lock.

Test Plan:

  1. Clone the repository and do all the required steps from the Contributing guide
  2. Create react-native.config.js and enable automaticPodsInstallation
  3. Whenever Podfile or Podfile.lock content changes installation of Cocoapods should be triggered
node /path/to/react-native-cli/packages/cli/build/bin.js init

Checklist

  • Documentation is up to date to reflect these changes.
  • Follows commit message convention described in CONTRIBUTING.md

@github-actions github-actions bot added the bugfix label Jul 3, 2024
@szymonrybczak szymonrybczak changed the title fix(apple): compare also Podfile and Podfile.lock when deciding to install Cocoapods fix(apple): include check if Podfile and Podfile.lock changed when deciding to install Cocoapods Jul 3, 2024
@szymonrybczak
Copy link
Collaborator Author

cc @tido64, I know that you had some concerns related to this topic. I'd really appreciate your input here what else should we improve to solve your case.

@thymikee
Copy link
Member

thymikee commented Jul 3, 2024

Can you include some tests please?

@szymonrybczak szymonrybczak force-pushed the fix/compare-podfile-podfile-lock-when-deciding-to-install-pods branch from f76df9f to 5d83cde Compare July 3, 2024 13:58
@TMisiukiewicz
Copy link
Collaborator

Could you check if it'd be faster to use checksum that is auto-generated inside Podfile.lock? I am wondering how it compares to your current implementation 🤔

@szymonrybczak
Copy link
Collaborator Author

Could you check if it'd be faster to use checksum that is auto-generated inside Podfile.lock? I am wondering how it compares to your current implementation 🤔

It turns out that hashing file is faster 👀

I've created two simple scripts that checks that:

Hashing file script

const fs = require('fs');
const { createHash } = require('crypto');

function generateMd5Hash(text) {
  return createHash('md5').update(text).digest('hex');
}

async function main() {
  const hash = generateMd5Hash(
    fs.readFileSync('_/ios/Podfile.lock', 'utf8'),
  );

  console.log(hash);
}

main();
Reading file and checking checksum

const fs = require('fs');
const readline = require('readline')

async function main() {
  const fileStream = fs.createReadStream('_/ios/Podfile.lock');

  const rl = readline.createInterface({
    input: fileStream,
    crlfDelay: Infinity,
  });

  let lines = [];
  for await (const line of rl) {
    lines.push(line);
  }

  lines = lines.reverse();

  for (const line of lines) {
    if (line.includes('PODFILE CHECKSUM')) {
      console.log(line.split(': ')[1]);
    }
  }
}

main();

On fresh project when Podfile.lock contains ~1k lines, the results are pretty much the same

❯ wc -l ios/Podfile.lock
    1402 ios/Podfile.lock

❯ hyperfine "node hash.js"
Benchmark 1: node hash.js
  Time (mean ± σ):      42.1 ms ±   4.1 ms    [User: 34.2 ms, System: 5.9 ms]
  Range (min … max):    39.9 ms …  72.0 ms    63 runs

~/development/tmp/elo                                                          
❯ hyperfine "node checksum.js"
Benchmark 1: node checksum.js
  Time (mean ± σ):      46.7 ms ±  17.3 ms    [User: 37.6 ms, System: 6.5 ms]
  Range (min … max):    42.8 ms … 178.3 ms    60 runs

but I've created an example scenario when Podfile.lock contains ~60k lines, and creating hash is much faster:

❯ wc -l ios/Podfile.lock
   60286 ios/Podfile.lock
                                                        
❯ hyperfine "node hash.js"
Benchmark 1: node hash.js
  Time (mean ± σ):      45.9 ms ±   1.0 ms    [User: 37.7 ms, System: 6.9 ms]
  Range (min … max):    43.5 ms …  49.8 ms    57 runs
                                                        
❯ hyperfine "node checksum.js"
Benchmark 1: node checksum.js
  Time (mean ± σ):      67.1 ms ±   1.8 ms    [User: 72.1 ms, System: 13.4 ms]
  Range (min … max):    64.5 ms …  70.8 ms    39 runs

@tido64
Copy link
Contributor

tido64 commented Jul 4, 2024

Could you check if it'd be faster to use checksum that is auto-generated inside Podfile.lock? I am wondering how it compares to your current implementation 🤔

It turns out that hashing file is faster 👀

Rather than speed, I think in this case I'd would much prefer that we are consistent with CocoaPods. If it thinks that the manifest needs to be updated, so should we, and vice versa. And I think the only way to ensure that is to reuse the checksum or CocoaPods directly.

@szymonrybczak
Copy link
Collaborator Author

szymonrybczak commented Jul 4, 2024

Yeah, so I've added reading checksum after installing Cocoapods, so that new checksum is saved. Together with @TMisiukiewicz we did some investigations and it turns out that there are 3 scenarios with current implementation where we'll run pod install where it's not necessary:

  1. someone install dependency and it's presented in config#dependencies output and then install Cocoapods with pod install manually, then CLI runs pod install because outdated checksum value is in cache and it's different than the one saved in Podfile.lock,
  2. the same scenario as 1) but with adding pod directly inside Podfile,
  3. someone has 3rd party scripts that is referenced in Podfile and it's adding dynamically Pods.

For 1), 2) I think we could add phase of updating cache values e.g. in post-install hook in pod install command (not sure if this is possible without changing Podfile in template right now).

For 3) I think it's really edge case-y and supporting this would solve very small amount of cases, or maybe even 0.

Copy link
Contributor

@cipolleschi cipolleschi left a comment

Choose a reason for hiding this comment

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

Small nits, the logic sounds good to me!

packages/cli-platform-apple/src/tools/pods.ts Outdated Show resolved Hide resolved
packages/cli-platform-apple/src/tools/pods.ts Outdated Show resolved Hide resolved
@szymonrybczak szymonrybczak force-pushed the fix/compare-podfile-podfile-lock-when-deciding-to-install-pods branch from c829229 to 5abea6e Compare August 31, 2024 10:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants