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 support for building PHAR files and associated tests #40

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

hussainweb
Copy link
Contributor

Description

This change adds a box configuration file so that we can build PHAR's as discussed in #25. There is also associated CI changes to test the PHAR file.

Motivation and context

This was discussed in #25.

How has this been tested?

I modified the expect files to run the PHAR file instead of entrypoint and ran the tests. All the tests passed. I also tested the PHAR file manually. This change is also implemented in the CI workflow.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

PR checklist

  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING.md document.
  • I have added tests to cover my changes.

@hussainweb hussainweb requested a review from ramsey as a code owner January 21, 2022 17:02
@codecov
Copy link

codecov bot commented Jan 21, 2022

Codecov Report

Merging #40 (522d6f3) into main (32edb54) will not change coverage.
The diff coverage is n/a.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff            @@
##               main      #40   +/-   ##
=========================================
  Coverage     98.76%   98.76%           
  Complexity      296      296           
=========================================
  Files            39       39           
  Lines           813      813           
=========================================
  Hits            803      803           
  Misses           10       10           

@hussainweb
Copy link
Contributor Author

@ramsey, I'm going to write a Github workflow to upload the PHAR file to the Github release. Can you tell me more about your workflow when you create a release? Do you manually create a tag, push it, and then create a release? Or do you directly do all of this using Github's releases page? How do you create the changelog?

I'm considering using https://github.com/svenstaro/upload-release-action/ and I see it can handle existing releases. It will work with an existing release and just add PHAR file. Optionally, it can also create the release when you push a tag and you can go in and fill the changelog. (There are changelog generators as well but that's a different issue.)

If you can explain your process, I can adapt the workflow to fit it or suggest an approach that can work reasonably well with it.

@ramsey
Copy link
Owner

ramsey commented Jan 23, 2022

Right now, the process is all manual, but I have been considering other tools to help automate releases.

@hussainweb
Copy link
Contributor Author

Got it. So, what do you think of a workflow that when you create a release manually, a GitHub action runs, creates the PHAR file, and uploads it to the release?

@ramsey
Copy link
Owner

ramsey commented Jan 24, 2022

That sounds good to me.

@ramsey
Copy link
Owner

ramsey commented Apr 20, 2022

Now that I've finally gotten around to fixing the nasty symfony/console bug, I can focus on this again. What's the status of your PR? It sounds like you still had some things left to finish, according to the comments above. Is that correct?

@hussainweb
Copy link
Contributor Author

@ramsey, it's been a while for me as well but I was finally able to spend time today catching up with my work.

The short answer to your question is that the Box build works well and I can write a workflow to package it with a release. The generated phar file is about 5 MB in size right now and I will look into optimizing it.

Can you give me an idea of how you create releases right now? I don't see a Github action for this, so I assume you create releases manually. I plan to write a workflow that will fire on the release event, create the phar file, and attach it to the release (probably using the action-gh-release but I'm still looking into it).

I am sharing all this so that if you have any reservations about any of these, let me know.

Github workflows contains invalid command options. This commit also adds phars.xml that locks the version of box we install.
box 4.1 depends on PHP 8.1, and we work with PHP 7.4+
@hussainweb
Copy link
Contributor Author

@ramsey, it's a good sign that the updated integration tests are working along with phar files. I can work on packaging next but I wanted to check on a few things.

  • How do you create releases? Do you create a tag and let GitHub make the releases or do you do that as a separate step manually?
  • Would you prefer the phar file to be committed to the repository? I see you already have a bin/conventional-commits executable. In this PR, I am adding the phar entry in .gitignore, but I could write a workflow that generates a phar file for every change and commits it. This phar file will eventually be used for releases. Alternatively, we could create a phar just before creating a release.
  • Apart from a phar file, there is also a signature that needs to be created and uploaded to OpenPGP. I am guessing you'd rather do these steps yourself. You will also have to set the relevant keys in environment variables so that workflows can use it to generate a signature file (conventional-commits.phar.asc). I'm afraid a lot of these steps will have to be done by you as the repo owner.
  • I found a good workflow for creating these files in phpstan. We don't have to use that process exactly but pick up the steps that generate the signature file.

Please share your thoughts on the above.

@ramsey ramsey force-pushed the main branch 2 times, most recently from a39279f to d9b11fd Compare April 17, 2024 04:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants