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

Introduce composer plugin #35

Merged
merged 2 commits into from
Jan 24, 2025
Merged

Introduce composer plugin #35

merged 2 commits into from
Jan 24, 2025

Conversation

veewee
Copy link
Contributor

@veewee veewee commented Dec 27, 2024

This PR will make it possible to install the mago binaries from within composer.
It contains:

  • A composer command: composer mago:install-binary that can be used to install the binary for currently required mago version for current platform.
    • The downloaded files will be placed in the vendor directory of this package inside composer/bin/{version}
    • It will also create a .platform file that contains a relative link to the mago executable for current platform.
  • Event listeners to download the assets when the package is updated / installed through composer.

This package requires activation of the plugin:

    "config": {
        "allow-plugins": {
            "carthage-software/mago": true
        }
    }

Once the assets are in place, that is where the composer plugin stops.

Next up is a mago PHP binary that will be symlinked in vendor/bin/mago.
This script will:

  • Load the current platform executable from the .platform file
  • Executes the mago command in a separate processs
  • Currently open stdin, stdout and stderr are being linked to the executable, making it possible to use it just as you would in a regular command.

This setup allows to run mago on multiple platforms (docker, virtualbox, host, ...) just as one would expect from regular PHP tools.

Screenshot 2025-01-03 at 14 35 52

Screenshot 2025-01-03 at 14 36 25

@veewee veewee marked this pull request as draft December 27, 2024 19:34
@azjezz azjezz marked this pull request as ready for review December 27, 2024 21:05
@veewee veewee force-pushed the composer branch 7 times, most recently from 613176e to 881d28d Compare December 28, 2024 10:43
composer.json Outdated Show resolved Hide resolved
composer/bin/mago Show resolved Hide resolved
#!/usr/bin/env php
<?php

declare(strict_types=1);
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've kept this file for a few reasons:

  • Composer adds a binary proxy wrapper. It somehow expects this to be a PHP file.
  • In windows, the executable will be suffixed with .exe. I'm not sure if it runs without that suffix.
  • This gives us some control about the availability of the executable. For example when composer runs with no-scripts

@veewee veewee force-pushed the composer branch 2 times, most recently from 02adf92 to 4f510be Compare January 3, 2025 13:52
@veewee veewee requested a review from azjezz January 3, 2025 14:03
composer/InstallMagoBinaryCommand.php Outdated Show resolved Hide resolved
$executable_extension = '';

switch ($os) {
case 'windows':
Copy link

Choose a reason for hiding this comment

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

php_uname('s') will return Windows NT on windows so you should check for this string instead, though you should still make sure that the osname used in the filename is still windows

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can you verify if I understood your comment correctly.

Thanks for reviewing!

Copy link

Choose a reason for hiding this comment

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

Yes this looks fine to me 👍 I'll test again on windows tonight

Copy link
Member

Choose a reason for hiding this comment

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

@Zuruuh is this good to go?

Copy link

@Zuruuh Zuruuh Jan 23, 2025

Choose a reason for hiding this comment

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

Yeah I tried it and can confirm it works now 😄

@azjezz azjezz added Priority: High After critical issues are fixed, handle these before any further issues. Status: In Progress This issue is being worked on and has someone assigned. Status: Revision Needed Multiple reviewers found issues in the PR that need addressing. Type: Enhancement Request for additions or changes that improve existing functionality. labels Jan 23, 2025
@azjezz azjezz added Status: Review Needed A PR is attached that needs review. and removed Status: Revision Needed Multiple reviewers found issues in the PR that need addressing. Status: Review Needed A PR is attached that needs review. labels Jan 23, 2025
@azjezz azjezz merged commit 21895f6 into carthage-software:main Jan 24, 2025
4 checks passed
@azjezz azjezz added the Status: Closed This issue is closed and no more work is planned. label Jan 24, 2025
@azjezz azjezz removed the Status: In Progress This issue is being worked on and has someone assigned. label Jan 24, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Priority: High After critical issues are fixed, handle these before any further issues. Status: Closed This issue is closed and no more work is planned. Type: Enhancement Request for additions or changes that improve existing functionality.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants