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

Add the --dry-run option #118

Closed
wants to merge 1 commit into from

Conversation

alexandre-daubois
Copy link
Contributor

Fix #18

Allows using the --dry-run option with both build and install command. This wasn't added the download command, as I'm not really sure it makes sense to have it.

@alexandre-daubois alexandre-daubois marked this pull request as ready for review November 20, 2024 13:23
@alexandre-daubois alexandre-daubois force-pushed the dry-run branch 4 times, most recently from 3cb2cbe to 6dce19e Compare November 26, 2024 15:37
src/Platform/TargetPlatform.php Outdated Show resolved Hide resolved
src/Building/UnixBuild.php Outdated Show resolved Hide resolved
src/Building/UnixBuild.php Outdated Show resolved Hide resolved
@@ -89,6 +90,16 @@ public static function configureDownloadBuildInstallOptions(Command $command): v
$command->ignoreValidationErrors();
}

public static function configureDryRunOption(Command $command): void
Copy link
Collaborator

Choose a reason for hiding this comment

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

IMO this should be merged into configureDownloadBuildInstallOptions

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, because it would mean the download command could dry run. I'm not sure this really makes sense to dry run a download?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think that is the part that is confusing; whilst we do actually need to download to dry-run the build+install parts, we should be able to restore the PIE_WORKING_DIRECTORY to its previous state.

Running \Php\Pie\ComposerIntegration\ComposerIntegrationHandler::__invoke updates your ${PIE_WORKING_DIRECTORY}/pie.json (+${PIE_WORKING_DIRECTORY}/pie.lock) and subsequently updates {$PIE_WORKING_DIRECTORY}/vendor/... with the new install.

The correct approach for dry-run would be:

  • Resolve dependency
  • Back up pie.json+pie.lock+vendor
    • Run the Composer integration handler
    • If requested, run the build with dry run flag
    • If requested, run the install with dry run flag
  • Restore the backups

BUT - perhaps a simpler way of doing this however, would be to sandbox the dry-run download by adding dry_run prefix or similar to $targetPlatformPath in \Php\Pie\Platform::getPieWorkingDirectory, so it is downloaded in isolation from someone's "real" PIE_WORKING_DIRECTORY; then after the dry-run download/build/install is complete, remove the pie working directory (checking it is only for a dry-run).

FWIW, this is one of the reasons I'd been avoiding implementing --dry-run; it is messy and non-trivial to implement correctly 😆

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for this complete insight! I think I'm going to close this PR. If we were to implement it, I think it should be started again from scratch, because the current implementation here is not what we want.

I'll link your comment in the issue for when I'll have a bit more time for this feature, or if someone wants to take over.

Thanks again!

@asgrim asgrim added the enhancement New feature or request label Nov 29, 2024
@alexandre-daubois alexandre-daubois force-pushed the dry-run branch 4 times, most recently from 44fd831 to d84dfd2 Compare December 3, 2024 15:31
@alexandre-daubois
Copy link
Contributor Author

Thanks James, I addressed all your comments! I really like the dry-run:: and the fact it could be used later to check whether a dry run was launched or not.

@alexandre-daubois
Copy link
Contributor Author

Closed as explained in #118 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add --dry-run option
2 participants