-
Notifications
You must be signed in to change notification settings - Fork 20
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 a script to build a PHAR executable #124
Add a script to build a PHAR executable #124
Conversation
.travis.yml
Outdated
@@ -24,6 +24,7 @@ before_script: | |||
- phpenv rehash | |||
- phpenv config-rm xdebug.ini | |||
- export PATH=~/.composer/vendor/bin:$PATH | |||
- rm composer.lock |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we're removing the lock file for tests I see little point in having it at all.
It's either useful to allow us to build a known working version with deps or it's not of much use at all. If we aren't running tests against the lock we ship with the tool we can't really be certain the shipped version works
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. I'll add an entry to the matrix that keeps the lock file and test against that.
README.md
Outdated
To install the PHAR executable: | ||
|
||
1. [Download the upgrader as a PHAR executable](releases/download/1.2.0/upgrade-code.phar) or `wget https://github.com/silverstripe/silverstripe-upgrader/releases/download/1.2.0/upgrade-code.phar` | ||
1. Make the file executable `chmod +x upgrade-code.phar` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do quite enjoy this abuse of markdown, however markdown is meant to be a format that's easily readable both in it's rendered and raw format. I fear we're prioritising clean diffs over readability of the raw file here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I presume you're referring to the repetitive use 1.
instead of having an incremented number for each step?
Given it's a small list, I'm happy to use 1.
, 2.
, 3.
, etc.
build.php
Outdated
|
||
$compiledPath = PACKAGE_ROOT . "upgrade-code.phar"; | ||
if ($fs->exists($compiledPath)) { $fs->remove($compiledPath);} | ||
@$compiler->compile($compiledPath); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we silence errors here but then proceed as if nothing went wrong (if it did) is that intentional / the best way to do this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's some silly warning getting thrown here. Looks like the problem is with the third party library I'm using. I'll try doing a PR against that third party library.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem is not with the third party library per say. It calls token_get_all
on each file which throws warning on some minor syntax errors in a few third party libs. I've suppress those specific warning instead.
OK - this looks cool. So how do we work this into our release process to make sure we either always release the phar with each tag or indeed with each commit? We solved this problem on sspak by publishing the phar on travis when non-PR builds pass. (see |
I had a chat with Ingo when I submitted this issue. We felt it wasn't worth the extra work to automate the PHAR generation with each release. The release cycle for the upgrader is a bit more fluid ... basically, I do a release whenever I feel like it. So it's probably manageable to manually generate the phar before each release and attach it to the release. |
Mmm, but that easily becomes lost when someone else who doesn't know that comes and does a release... It wasn't much work to publish the sspak releases on merge... |
A quick Google and look how simple it is :) |
Add secondtruth/phar-compiler to dev dependencies and start ttracking composer.lock again. silverstripe#100
Add a test that uses the dependencies define in the lock file. silverstripe#100
9694327
to
4db0ad2
Compare
My preference would have been to to automate it. It's mostly a question of is it worth spending an hour or two now to save 5 minutes every month. There's also a bit of manual work involved with incrementing the release version. That's our last bit of upgrader work for a while. If we end up picking up extra upgrader tasks we can revisit this. |
This will save us from always updating the doc.
[ci skip]
Following discussion with @chillu, we'll merge this and look at auto building the phar. Next time we have a big chunk of work to do on the upgrader. |
Hmm, it's about more than that; that's it reduced to it's most basic level, but what about the single point of failure, the knowledge sharing required and the time it takes to rectify a problem when it goes wrong. |
2. Increment the version number in `src/upgrade-code`, commit your changes and push them back up to GitHub. | ||
3. Call `php build.php` on the command line. This will build the upgrader as a PHAR file. | ||
4. Move `upgrade-code.phar` to a temporary folder. | ||
5. Switch to the `gh-pages` branch. e.g.: `git checkout gh-pages` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are we pushing into the gh-pages branch? that's reserved for running a static website off the repo, if we upload it to the release page this step is not needed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's mimicking the set up that sspak is using. https://silverstripe.github.io/sspak/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah - I noticed that after reading the framework PR; I'm not sure that's the "right" approach, but we can live with it.
Parent issue