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.

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

fix: add more guard conditions for retrieving buildConfig from XcScheme #2196

Merged
merged 3 commits into from
Dec 18, 2023
Merged

fix: add more guard conditions for retrieving buildConfig from XcScheme #2196

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7c8bb12
fix: add more guard conditions for retrieving buildConfig from XcScheme
AnnatarHe Dec 7, 2023
14560bc
fix: mark unused variable as _
AnnatarHe Dec 13, 2023
bdb77e5
fix: revert code and add more info
AnnatarHe Dec 13, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
60 changes: 37 additions & 23 deletions packages/cli-platform-ios/src/tools/getBuildConfigurationFromXcScheme.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,29 +13,45 @@ export function getBuildConfigurationFromXcScheme(
sourceDir: string,
projectInfo: IosInfo | undefined,
): string {
let xcProjects: string[] = [];
try {
const xcProject = fs
xcProjects = fs
.readdirSync(sourceDir)
.find((dir) => dir.includes('.xcodeproj'));

if (xcProject) {
const xmlScheme = fs.readFileSync(
path.join(
sourceDir,
xcProject,
'xcshareddata',
'xcschemes',
`${scheme}.xcscheme`,
),
{
encoding: 'utf-8',
},
);

const {Scheme} = xmlParser.parse(xmlScheme);

return Scheme.LaunchAction['@_buildConfiguration'];
}
.filter((x) => x.endsWith('.xcodeproj'));
} catch (e: any) {
throw new CLIError(`Unknown error: ${e.toString()}`);
}

if (xcProjects.length === 0) {
throw new CLIError(
'Xcode project not found. please make sure it is a iOS project',
);
}

if (xcProjects.length > 1) {
throw new CLIError(
'Multiple Xcode projects found. we are only support one. please remove another one',
);
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I believe in that case, instead of forcing to remove the project, we should prompt user which project to use -> maybe store it in cache?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I’m not sure whether we should prompt the user to choose one option for processing. Additionally, it seems that the current process does not support different projects. the previous code used a simple [].find to select the first available option, but I guess this is not the only occurrence.

this PR just corrects the error message here. If the CLI is going to support multiple projects in future, we can easily identify where we need to make changes.

additionally, I’m not familiar with this project, so I don’t know how to show a prompt to the user in this project 😂

If necessary, maybe we can raise another pull request after this one is merged? This one is for correcting error messages, and the next one will be for enhancing the DX.


const xcProject = xcProjects[0];

try {
const xmlScheme = fs.readFileSync(
path.join(
sourceDir,
xcProject,
'xcshareddata',
'xcschemes',
`${scheme}.xcscheme`,
),
{
encoding: 'utf-8',
},
);

const {Scheme} = xmlParser.parse(xmlScheme);
return Scheme.LaunchAction['@_buildConfiguration'];
} catch {
const availableSchemas =
projectInfo && projectInfo.schemes && projectInfo.schemes.length > 0
Expand All @@ -48,6 +64,4 @@ export function getBuildConfigurationFromXcScheme(
`Could not find scheme ${scheme}. Please make sure the schema you want to run exists. ${availableSchemas}`,
);
}

return configuration;
}
Loading