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

Make (in)compatible OS families non-empty-list or a nullable to express intent better #2

Merged
merged 2 commits into from
Dec 3, 2024
Merged

Make (in)compatible OS families non-empty-list or a nullable to express intent better #2

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
aa7fd4b
Make (in)compatible OS families non-empty-list or a nullable to expre…
asgrim Nov 28, 2024
6bb4730
Simplify os-families/os-families-exclude expressions
asgrim Nov 28, 2024
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
43 changes: 20 additions & 23 deletions src/DependencyResolver/Package.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
use Php\Pie\ExtensionType;
use Php\Pie\Platform\OperatingSystemFamily;

use Webmozart\Assert\Assert;
use function array_key_exists;
use function array_map;
use function array_slice;
Expand All @@ -31,9 +32,9 @@
final class Package
{
/**
* @param list<ConfigureOption> $configureOptions
* @param list<OperatingSystemFamily> $compatibleOsFamilies
* @param list<OperatingSystemFamily> $incompatibleOsFamilies
* @param list<ConfigureOption> $configureOptions
* @param non-empty-list<OperatingSystemFamily>|null $compatibleOsFamilies
Copy link
Author

Choose a reason for hiding this comment

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

The key change here is that the OS families can either be null (explicitly not specified in php-ext), or a non-empty-list; I think this helps express intent better, otherwise, maybe someone could try:

  "os-families": []

which does not make sense

* @param non-empty-list<OperatingSystemFamily>|null $incompatibleOsFamilies
*/
public function __construct(
public readonly CompletePackageInterface $composerPackage,
Expand All @@ -46,8 +47,8 @@ public function __construct(
public readonly bool $supportZts,
public readonly bool $supportNts,
public readonly string|null $buildPath,
public readonly array $compatibleOsFamilies,
public readonly array $incompatibleOsFamilies,
public readonly array|null $compatibleOsFamilies,
public readonly array|null $incompatibleOsFamilies,
) {
}

Expand All @@ -74,17 +75,10 @@ public static function fromComposerCompletePackage(CompletePackageInterface $com
? $phpExtOptions['build-path']
: null;

/** @var list<string> $compatibleOsFamilies */
Copy link
Author

Choose a reason for hiding this comment

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

These are not necessary; we need upstream Composer support first

$compatibleOsFamilies = $phpExtOptions !== null && array_key_exists('os-families', $phpExtOptions)
? $phpExtOptions['os-families']
: [];

/** @var list<string> $incompatibleOsFamilies */
Copy link
Author

Choose a reason for hiding this comment

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

These are not necessary; we need upstream Composer support first

$incompatibleOsFamilies = $phpExtOptions !== null && array_key_exists('os-families-exclude', $phpExtOptions)
? $phpExtOptions['os-families-exclude']
: [];
$compatibleOsFamilies = $phpExtOptions['os-families'] ?? null;
$incompatibleOsFamilies = $phpExtOptions['os-families-exclude'] ?? null;

if ($compatibleOsFamilies && $incompatibleOsFamilies) {
if ($compatibleOsFamilies !== null && $incompatibleOsFamilies !== null) {
throw new InvalidArgumentException('Cannot specify both "os-families" and "os-families-exclude" in composer.json');
}

Expand Down Expand Up @@ -129,25 +123,28 @@ public function githubOrgAndRepository(): string
}

/**
* @param list<string> $input
* @param list<string>|null $input
*
* @return list<OperatingSystemFamily>
* @return non-empty-list<OperatingSystemFamily>|null
*/
private static function convertInputStringsToOperatingSystemFamilies(array $input): array
private static function convertInputStringsToOperatingSystemFamilies(array|null $input): array|null
{
if ($input === null) {
return null;
}

$osFamilies = [];
foreach ($input as $value) {
// try to normalize a bit the input
$valueToTry = ucfirst(strtolower($value));
Copy link
Author

Choose a reason for hiding this comment

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

One improvement I didn't do, but I think it could make sense to always lowercase (including the enum values), to avoid case sensitivity nonsense, as this approach isn't flawless.


$family = OperatingSystemFamily::tryFrom($valueToTry);
if ($family === null) {
throw new InvalidArgumentException(sprintf('Expected operating system family to be one of "%s", got "%s".', implode('", "', OperatingSystemFamily::asValuesList()), $value));
}
Assert::inArray($valueToTry, OperatingSystemFamily::asValuesList(), 'Expected operating system family to be one of: %2$s. Got: %s');

$osFamilies[] = $family;
$osFamilies[] = OperatingSystemFamily::from($valueToTry);
}

Assert::isNonEmptyList($osFamilies, 'Expected operating systems families to be a non-empty list.');

return $osFamilies;
}
}
4 changes: 2 additions & 2 deletions src/DependencyResolver/ResolveDependencyWithComposer.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,14 +75,14 @@ private function assertCompatibleThreadSafetyMode(ThreadSafetyMode $threadSafety

private function assertCompatibleOsFamily(TargetPlatform $targetPlatform, Package $resolvedPackage): void
{
if ($resolvedPackage->compatibleOsFamilies && ! in_array($targetPlatform->operatingSystemFamily, $resolvedPackage->compatibleOsFamilies, true)) {
if ($resolvedPackage->compatibleOsFamilies !== null && ! in_array($targetPlatform->operatingSystemFamily, $resolvedPackage->compatibleOsFamilies, true)) {
throw IncompatibleOperatingSystemFamily::notInCompatibleOperatingSystemFamilies(
$resolvedPackage->compatibleOsFamilies,
$targetPlatform->operatingSystemFamily,
);
}

if ($resolvedPackage->incompatibleOsFamilies && in_array($targetPlatform->operatingSystemFamily, $resolvedPackage->incompatibleOsFamilies, true)) {
if ($resolvedPackage->incompatibleOsFamilies !== null && in_array($targetPlatform->operatingSystemFamily, $resolvedPackage->incompatibleOsFamilies, true)) {
throw IncompatibleOperatingSystemFamily::inIncompatibleOperatingSystemFamily(
$resolvedPackage->incompatibleOsFamilies,
$targetPlatform->operatingSystemFamily,
Expand Down
2 changes: 1 addition & 1 deletion src/Platform/OperatingSystemFamily.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ enum OperatingSystemFamily: string
case Linux = 'Linux';
case Unknown = 'Unknown';

/** @return array<string> */
/** @return non-empty-list<non-empty-string> */
public static function asValuesList(): array
{
return array_map(
Expand Down
20 changes: 10 additions & 10 deletions test/integration/Building/UnixBuildTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,8 +48,8 @@ public function testUnixBuildCanBuildExtension(): void
true,
true,
null,
[],
[],
null,
null,
),
self::TEST_EXTENSION_PATH,
);
Expand Down Expand Up @@ -103,8 +103,8 @@ public function testUnixBuildWillThrowExceptionWhenExpectedBinaryNameMismatches(
true,
true,
null,
[],
[],
null,
null,
),
self::TEST_EXTENSION_PATH,
);
Expand Down Expand Up @@ -146,8 +146,8 @@ public function testUnixBuildCanBuildExtensionWithBuildPath(): void
true,
true,
'pie_test_ext',
[],
[],
null,
null,
),
dirname(self::TEST_EXTENSION_PATH),
);
Expand Down Expand Up @@ -205,8 +205,8 @@ public function testCleanupDoesNotCleanWhenConfigureIsMissing(): void
true,
true,
null,
[],
[],
null,
null,
),
self::TEST_EXTENSION_PATH,
);
Expand Down Expand Up @@ -249,8 +249,8 @@ public function testVerboseOutputShowsCleanupMessages(): void
true,
true,
null,
[],
[],
null,
null,
),
self::TEST_EXTENSION_PATH,
);
Expand Down
4 changes: 2 additions & 2 deletions test/integration/Downloading/GithubPackageReleaseAssetsTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,8 +54,8 @@ public function testDeterminingReleaseAssetUrlForWindows(): void
true,
true,
null,
[],
[],
null,
null,
);

$io = $this->createMock(IOInterface::class);
Expand Down
4 changes: 2 additions & 2 deletions test/integration/Installing/UnixInstallTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,8 +91,8 @@ public function testUnixInstallCanInstallExtension(string $phpConfig): void
true,
true,
null,
[],
[],
null,
null,
),
self::TEST_EXTENSION_PATH,
);
Expand Down
4 changes: 2 additions & 2 deletions test/integration/Installing/WindowsInstallTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,8 +55,8 @@ public function testWindowsInstallCanInstallExtension(): void
true,
true,
null,
[],
[],
null,
null,
),
self::TEST_EXTENSION_PATH,
);
Expand Down
4 changes: 2 additions & 2 deletions test/unit/Command/CommandHelperTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,8 +110,8 @@ public function testProcessingConfigureOptionsFromInput(): void
true,
true,
null,
[],
[],
null,
null,
);
$inputDefinition = new InputDefinition();
$inputDefinition->addOption(new InputOption('with-stuff', null, InputOption::VALUE_REQUIRED));
Expand Down
4 changes: 2 additions & 2 deletions test/unit/DependencyResolver/PackageTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -168,8 +168,8 @@ public function testGithubOrgAndRepo(string $composerPackageName, string|null $d
true,
true,
null,
[],
[],
null,
null,
);

self::assertSame($expectedGithubOrgAndRepo, $package->githubOrgAndRepository());
Expand Down
8 changes: 4 additions & 4 deletions test/unit/Downloading/DownloadedPackageTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,8 +33,8 @@ public function testFromPackageAndExtractedPath(): void
true,
true,
null,
[],
[],
null,
null,
);

$extractedSourcePath = uniqid('/path/to/downloaded/package', true);
Expand All @@ -58,8 +58,8 @@ public function testFromPackageAndExtractedPathWithBuildPath(): void
true,
true,
'Downloading',
[],
[],
null,
null,
);

$extractedSourcePath = realpath(__DIR__ . '/../');
Expand Down
8 changes: 4 additions & 4 deletions test/unit/Downloading/Exception/CouldNotFindReleaseAssetTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,8 +34,8 @@ public function testForPackage(): void
true,
true,
null,
[],
[],
null,
null,
);

$exception = CouldNotFindReleaseAsset::forPackage($package, ['something.zip', 'something2.zip']);
Expand All @@ -56,8 +56,8 @@ public function testForPackageWithMissingTag(): void
true,
true,
null,
[],
[],
null,
null,
);

$exception = CouldNotFindReleaseAsset::forPackageWithMissingTag($package);
Expand Down
12 changes: 6 additions & 6 deletions test/unit/Downloading/GithubPackageReleaseAssetsTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,8 +82,8 @@ public function testUrlIsReturnedWhenFindingWindowsDownloadUrl(): void
true,
true,
null,
[],
[],
null,
null,
);

$releaseAssets = new GithubPackageReleaseAssets('https://test-github-api-base-url.thephp.foundation');
Expand Down Expand Up @@ -144,8 +144,8 @@ public function testUrlIsReturnedWhenFindingWindowsDownloadUrlWithCompilerAndThr
true,
true,
null,
[],
[],
null,
null,
);

$releaseAssets = new GithubPackageReleaseAssets('https://test-github-api-base-url.thephp.foundation');
Expand Down Expand Up @@ -187,8 +187,8 @@ public function testFindWindowsDownloadUrlForPackageThrowsExceptionWhenAssetNotF
true,
true,
null,
[],
[],
null,
null,
);

$releaseAssets = new GithubPackageReleaseAssets('https://test-github-api-base-url.thephp.foundation');
Expand Down
4 changes: 2 additions & 2 deletions test/unit/Platform/WindowsExtensionAssetNameTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,8 +53,8 @@ public function setUp(): void
true,
true,
null,
[],
[],
null,
null,
);
}

Expand Down