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

Jump to bottom

fix(macCatalyst): construct correct path for executable #2510

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix(macCatalyst): construct correct path for executable #2510

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
97 changes: 97 additions & 0 deletions packages/cli-platform-apple/src/commands/runCommand/__tests__/getBuildPath.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,97 @@
import path from 'path';
import fs from 'fs';
import {getTempDirectory} from '../../../../../../jest/helpers';
import {BuildSettings} from '../getBuildSettings';
import {getBuildPath} from '../getBuildPath';

const targetBuildDirName = 'foo';
const targetBuildDirNameWithMaccatalyst = `${targetBuildDirName}-maccatalyst`;
const executableFolderPath = path.join('foo.app', 'Contents', 'MacOS', 'foo');

test('correctly determines macCatalyst build artifact path new style', async () => {
// setup:
const tmpBuildPath = getTempDirectory('maccatalyst-test-dir');
fs.mkdirSync(path.join(tmpBuildPath, targetBuildDirNameWithMaccatalyst), {
recursive: true,
});

// - create buildSettings object that represents this to CLI
const buildSettings: BuildSettings = {
TARGET_BUILD_DIR: path.join(
tmpBuildPath,
targetBuildDirNameWithMaccatalyst,
),
EXECUTABLE_FOLDER_PATH: executableFolderPath,
FULL_PRODUCT_NAME: 'unused-in-this-test',
INFOPLIST_PATH: 'unused-in-this-test',
};

// test:
// - send our buildSettings in and see what build path comes out
const buildPath = await getBuildPath(buildSettings, 'ios', true);

// assert:
expect(buildPath).toBe(
path.join(
tmpBuildPath,
targetBuildDirNameWithMaccatalyst,
executableFolderPath,
),
);
});

test('correctly determines macCatalyst build artifact path old style', async () => {
// setup:
const tmpBuildPath = getTempDirectory('maccatalyst-test-dir');
fs.mkdirSync(path.join(tmpBuildPath, targetBuildDirNameWithMaccatalyst), {
recursive: true,
});

// - create buildSettings object that represents this to CLI
// FIXME get the build settings as side effect from project definition,
// because it's the translation of project settings to path that fails
const buildSettings: BuildSettings = {
TARGET_BUILD_DIR: path.join(tmpBuildPath, targetBuildDirName),
EXECUTABLE_FOLDER_PATH: executableFolderPath,
FULL_PRODUCT_NAME: 'unused-in-this-test',
INFOPLIST_PATH: 'unused-in-this-test',
};

// test:
// - send our buildSettings in and see what build path comes out
const buildPath = await getBuildPath(buildSettings, 'ios', true);

// assert:
expect(buildPath).toBe(
path.join(
tmpBuildPath,
targetBuildDirNameWithMaccatalyst,
executableFolderPath,
),
);
});

test('correctly determines iOS build artifact path', async () => {
// setup:
const tmpBuildPath = getTempDirectory('ios-test-dir');
fs.mkdirSync(path.join(tmpBuildPath, targetBuildDirName), {
recursive: true,
});

// - create buildSettings object that represents this to CLI
const buildSettings: BuildSettings = {
TARGET_BUILD_DIR: path.join(tmpBuildPath, targetBuildDirName),
EXECUTABLE_FOLDER_PATH: executableFolderPath,
FULL_PRODUCT_NAME: 'unused-in-this-test',
INFOPLIST_PATH: 'unused-in-this-test',
};

// test:
// - send our buildSettings in and see what build path comes out
const buildPath = await getBuildPath(buildSettings);

// assert:
expect(buildPath).toBe(
path.join(tmpBuildPath, targetBuildDirName, executableFolderPath),
);
});
35 changes: 28 additions & 7 deletions packages/cli-platform-apple/src/commands/runCommand/getBuildPath.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import {CLIError} from '@react-native-community/cli-tools';
import path from 'path';
import fs from 'fs';
import {BuildSettings} from './getBuildSettings';
import {ApplePlatform} from '../../types';

Expand All @@ -8,7 +9,7 @@ export async function getBuildPath(
platform: ApplePlatform = 'ios',
isCatalyst: boolean = false,
) {
const targetBuildDir = buildSettings.TARGET_BUILD_DIR;
let targetBuildDir = buildSettings.TARGET_BUILD_DIR;
const executableFolderPath = buildSettings.EXECUTABLE_FOLDER_PATH;
const fullProductName = buildSettings.FULL_PRODUCT_NAME;

Expand All @@ -24,11 +25,31 @@ export async function getBuildPath(
throw new CLIError('Failed to get product name.');
}

if (isCatalyst) {
return path.join(`${targetBuildDir}-maccatalyst`, executableFolderPath);
} else if (platform === 'macos') {
return path.join(targetBuildDir, fullProductName);
} else {
return path.join(targetBuildDir, executableFolderPath);
// Default is platform == ios && isCatalyst == false
let buildPath = path.join(targetBuildDir, executableFolderPath);

// platform == ios && isCatalyst == true needs build path suffix,
// but this regresses from time to time with suffix present or not
// so check - there may be one already, or we may need to add suffix
if (platform === 'ios' && isCatalyst) {
// make sure path has one and only one '-maccatalyst' suffix on end
if (!targetBuildDir.match(/-maccatalyst$/)) {
targetBuildDir = `${targetBuildDir}-maccatalyst`;
}
buildPath = path.join(targetBuildDir, executableFolderPath);
}

// macOS gets the product name, not the executable folder path
if (platform === 'macos') {
Comment on lines +40 to +43
Copy link
Contributor

Choose a reason for hiding this comment

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

shouldn't this be if-else if?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it certainly could be, but...it seemed to me the three cases were all mutually exclusive so simple if overrides/clobbers were semantically identical ?

case 1 - we're the default platform == ios && isCatalyst == false: default case join targetBuildDir + executableFolderPath

case 2 - we're the rare platform == ios && isCatalyst == true: clobber with result of maybe adding -maccatalyst to targetBuildDir -- note that catalyst is an ios platform subset, it will not go in to the platform == macos conditional as true

case 3 - we're the platform == macos: clobber join of targetBuildDir + fullProductName

Each of the three cases is unambiguously identifiable and unique so I thought a simple set of ifs + clobbers worked?

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 just pushed a tiny change in response to this, for future developers so less explanation + thought is required I hope

1- I altered the isCatalyst conditional to explicitly contain a platform === 'ios' clause as that is the assumption, so now it is more clear that the catalyst block will not ever collide with the macos block

2- I updated the comments to reflect the same, and the idea that the default case and the two override blocks are exclusive of one another

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, they were equivalent. But with

if <cond1> {
}
if <cond2> {
}
if <cond3> {
}

All three conditions are always evaluated. With

if <cond1> {
} else if <cond2> {
} else if <cond3> {
}

cond2 is evaluated if and only if cond1 is false. cond3 is evaluated if and only if both cond1 and cond2 are false.

So, the if-elseif approach is slightly more efficient, and makes it clear that the conditions are mutually exclusive.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cipolleschi lots of discussion and we are in agreement with each other :-) - the question is that with the mixed signal I am hearing of "if/elseif/elseif is slightly better" vs "but I have approved the if/if/if version" - do you want me to convert it and re-push or not?

I don't want to swirl endlessly on this - it's a relatively small fix after all, but I'll do what you like (because, it's an easy conversion from if/if/if/ to if/elseif/elsif too).

So just let me know but I won't change+repush unless you ask, so we don't have another approval cycle unless desired

Cheers man

buildPath = path.join(targetBuildDir, fullProductName);
}

// Make sure the directory exists and fail fast vs silently failing
if (!fs.existsSync(targetBuildDir)) {
throw new CLIError(
`target build directory ${targetBuildDir} does not exist`,
);
}

return buildPath;
}
Loading