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.

Sign up for GitHub

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

Jump to bottom

[Feat] Add support for .xcode.env file in init and doctor commands #1585

Merged

thymikee merged 1 commit into master from feat/support-xcode-env

Apr 14, 2022

Contributor

cipolleschi commented Apr 6, 2022

Summary:

This PR adds the support for .xcode.env file in the init and doctor commands.
This PR depends on this other one: facebook/react-native#33546

Test Plan:

This PR is quite similar to 1530. I would rely on the same tests.
If anyone has suggestions about how to test this, I will be more than happy to implement them.

cipolleschi requested review from grabbou, thymikee and Esemesek as code owners

April 6, 2022 11:45

cipolleschi force-pushed the feat/support-xcode-env branch from 5d2ce3b to 839cbc8 Compare

April 6, 2022 11:48

cipolleschi mentioned this pull request

Introduce .xcode.env configuration file to source node facebook/react-native#33546

Closed

Contributor Author

cipolleschi commented Apr 6, 2022

The CI is failing on two tasks related to Android (they are related to gradlew). I don't think that my PR touched those files, though... and I can see that the master branch is failing as well.

tido64 reviewed

View reviewed changes

packages/cli-doctor/src/tools/healthchecks/xcodeEnv.ts Outdated

+                description: 'File to customize Xcode environment',
+                getDiagnostics: async () => {
+                  return {
+                    needsToBeFixed: fs.existsSync('./ios/.xcode.env'),

Contributor

tido64 Apr 6, 2022 •

edited

Loading

This is making assumptions on where the iOS project lives. Should we use the config to figure out where the closest Podfile is? And what about macOS?

Contributor Author

cipolleschi Apr 6, 2022

I updated it using findProjectRoot. I'm not sure that findPodfilePath is the right approach, because for macOS project we have an ios folder and a macos folder, both of which have their own Podfile.

$ ls -all ios
total 48
drwxr-xr-x  10 cipolleschi  staff    320  6 Apr 17:00 .
drwxr-xr-x  24 cipolleschi  staff    768  6 Apr 17:03 ..
-rw-r--r--   1 cipolleschi  staff   1106  6 Apr 16:59 Podfile
// ... other files here ...

$ ls -all macos
total 56
drwxr-xr-x  10 cipolleschi  staff    320  6 Apr 17:06 .
drwxr-xr-x  24 cipolleschi  staff    768  6 Apr 17:03 ..
-rw-r--r--   1 cipolleschi  staff   1086  6 Apr 17:03 Podfile

cipolleschi force-pushed the feat/support-xcode-env branch 4 times, most recently from c4cc9fa to 6da4683 Compare

April 6, 2022 16:38

tido64 reviewed

View reviewed changes

packages/cli-doctor/src/tools/healthchecks/xcodeEnv.ts

+                label: '.xcode.env',
+                description: 'File to customize Xcode environment',
+                getDiagnostics: async () => {
+                  const projectRoot = findProjectRoot();

Contributor

tido64 Apr 7, 2022

Somewhat unrelated (and definitely not to be fixed in this PR), I looked briefly at the options that are passed to all CLI commands and it looks like they include this information. However, it doesn't look like they are properly forwarded to the individual fixers. It's a bit silly that we have to duplicate the effort to find this info again. Do we have an issue tracking this? cc @mikehardy @thymikee

Member

thymikee Apr 7, 2022

I don't think we track this. Would make sense

Contributor

tido64 Apr 7, 2022

Opened #1588

packages/cli-doctor/src/tools/healthchecks/xcodeEnv.ts Outdated

Comment on lines 49 to 47

+                  const src =
+                    projectRoot +
+                    '/node_modules/react-native/template/ios/' +
+                    templateFilePath;

Contributor

tido64 Apr 7, 2022

This is assuming the installation path of react-native. We should probably use require.resolve instead?

Contributor Author

cipolleschi Apr 7, 2022

I hope I used it properly. Let me know if I have to update the call.

packages/cli-doctor/src/tools/healthchecks/xcodeEnv.ts Outdated

Comment on lines 55 to 61

+                  applePlatformFolders.forEach(async (folder) => {
+                    const destFolder = projectRoot + folder;
+                    const destFilePath = destFolder + '/' + xcodeEnvFile;
+                    if (fs.existsSync(destFolder) && !fs.existsSync(destFilePath)) {
+                      await copyFileAsync(src, destFilePath);
+                    }
+                  });

Contributor

tido64 Apr 7, 2022

This is assuming the location of the .xcworkspace. I've seen many differing setups including:

<project-root>/ios/MyApp.xcworkspace
<project-root>/MyApp.xcworkspace
<project-root>/example/ios/MyApp.xcworkspace
<project-root>/apps/ios/src/MyApp.xcworkspace

Can we refactor findPodfilePath to return all found Podfiles and add a .xcode.env next to them? I think that will be more accurate.

Member

thymikee Apr 7, 2022

Or introduce a findAllPodfilePaths helper

Contributor Author

cipolleschi Apr 7, 2022

Hi! Thank you all for the suggestions and @tido64 for sharing its experience.
I updated the PR with the findAllPodfilePaths function, as suggested by @thymikee and adding the file in all the returned paths.

tido64 mentioned this pull request

doctor: refactor the fixers so we can forward options and avoid duplicating efforts #1588

Closed

cipolleschi force-pushed the feat/support-xcode-env branch 2 times, most recently from 096e572 to e46b6be Compare

April 7, 2022 18:26

thymikee reviewed

View reviewed changes

packages/cli-doctor/src/tools/healthchecks/xcodeEnv.ts Outdated

+              import path from 'path';
+              import {promisify} from 'util';
+              import {findProjectRoot} from '@react-native-community/cli-tools';
+              import findAllPodfilePaths from '@react-native-community/cli-platform-ios/src/config/findAllPodfilePaths';

Member

thymikee Apr 13, 2022

We can't import from src since we transform the code before publishing and put it into build directory; src is not there in published files. We either need to export findAllPodfilePaths from @react-native-community/cli-platform-ios – and add a dependency on it for cli-doctor and update "references" in its tsconfig.json – or move this helper to the cli-tools package (and add a dependency on glob). I'd go with the former for now.

Member

thymikee Apr 13, 2022

FYI we have a yarn watch script that builds files on the go

Contributor Author

cipolleschi Apr 14, 2022

Hi @thymikee, thanks for the review.
I updated the PR with your suggestion. Let me know if it's ok!

thymikee reviewed

View reviewed changes

Member

thymikee left a comment

Almost there! One thing to address and we're good :)

cipolleschi force-pushed the feat/support-xcode-env branch from e46b6be to d5b49d1 Compare

April 14, 2022 08:50

thymikee approved these changes

View reviewed changes

Member

thymikee left a comment

LGTM 👍🏼

thymikee reviewed

View reviewed changes

packages/cli-doctor/tsconfig.json Show resolved Hide resolved

cipolleschi force-pushed the feat/support-xcode-env branch from d5b49d1 to 414d4e6 Compare

April 14, 2022 09:03

thymikee reviewed

View reviewed changes

packages/cli-doctor/package.json Outdated Show resolved Hide resolved


          [Feat] Add support for .xcode.env file in init and doctor commands

eb1b479

cipolleschi force-pushed the feat/support-xcode-env branch from f569596 to eb1b479 Compare

April 14, 2022 09:15

thymikee merged commit 99f953e into master

thymikee deleted the feat/support-xcode-env branch

April 14, 2022 11:08

adamTrz mentioned this pull request

fix: properly create .xcode.env file with doctors auto fix #1668

Merged

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet