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 PHAR file to GitHub releases (PHIVE support) #531

Open
wants to merge 7 commits into
base: develop
Choose a base branch
from

Conversation

filips123
Copy link

@filips123 filips123 commented Oct 27, 2018

Add support for signed PHIVE installation. Fix #489.

It will not be available until new release, because PHAR will be added to it.

Also, @bobthecow must set up GPG keys and add them to keyservers so PHAR will be signed. Details how to do that are on PHIVE site (section prerequisites) and in this article (all about creating and encrypting keys and adding them to Travis CI environment vars).

Basically, encrypted public and private keys should be added to .github/keys.asc in this repository, key ID should be added to Travis CI environment variable KEY_ID, decryption key should be added to environment variable DECRYPT_KEY and signing key should be added to environment variable SIGN_KEY.

This will enable installation with:

phive install bobthecow/psysh

After that, pull request to PHIVE should be added to make alias available.

@codecov
Copy link

codecov bot commented Oct 27, 2018

Codecov Report

Merging #531 into develop will increase coverage by 8.45%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff              @@
##             develop    #531      +/-   ##
============================================
+ Coverage      58.14%   66.6%   +8.45%     
- Complexity      2033    2049      +16     
============================================
  Files            128     125       -3     
  Lines           5005    5024      +19     
============================================
+ Hits            2910    3346     +436     
+ Misses          2095    1678     -417
Impacted Files Coverage Δ Complexity Δ
src/Command/DumpCommand.php 56.52% <0%> (-2.57%) 6% <0%> (ø)
src/Command/WtfCommand.php 36.17% <0%> (-2.47%) 10% <0%> (+2%)
src/Command/BufferCommand.php 43.47% <0%> (-1.98%) 4% <0%> (ø)
src/Command/WhereamiCommand.php 23.63% <0%> (-1.37%) 19% <0%> (+2%)
src/Command/SudoCommand.php 47.36% <0%> (-1.29%) 10% <0%> (ø)
src/Command/EditCommand.php 40.29% <0%> (-0.62%) 19% <0%> (ø)
src/Command/TimeitCommand.php 30.9% <0%> (-0.58%) 14% <0%> (ø)
src/Command/TraceCommand.php 26.98% <0%> (-0.44%) 21% <0%> (ø)
src/Command/ListCommand.php 30.69% <0%> (-0.31%) 32% <0%> (+2%)
src/Command/DocCommand.php 17.24% <0%> (-0.31%) 17% <0%> (ø)
... and 32 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 827e79d...181aef1. Read the comment docs.

@filips123
Copy link
Author

@bobthecow Can you merge this PR?

Makefile Show resolved Hide resolved
@filips123
Copy link
Author

@bobthecow Can you check this?

Makefile Show resolved Hide resolved
.travis.yml Outdated
@@ -32,14 +32,17 @@ script:
after_success:
- bash <(curl -s https://codecov.io/bash)

before_deploy: make dist -j 4
before_deploy: make dist -j 4 && cp build/psysh/psysh dist/psysh.phar && cp build/psysh/psysh.asc dist/psysh.asc
Copy link
Owner

Choose a reason for hiding this comment

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

this should be done as part of the dist make target rather than inside .travis.yml

Copy link
Author

@filips123 filips123 Dec 1, 2019

Choose a reason for hiding this comment

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

As files already exist in build/psysh/psysh and build/psysh/psysh.asc, this (copying) is actually not needed, as I can directly release them to GitHub Releases.

Update: It is needed because psysh needs to be named psysh.phar and physh.asc needs to be named physh.phar.asc. I will move that to Makefile.

Copy link
Author

Choose a reason for hiding this comment

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

This is now fixed.

Copy link
Owner

Choose a reason for hiding this comment

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

Why does it need to be named psysh.phar? I’d really rather not.

Copy link
Author

Choose a reason for hiding this comment

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

Because PHIVE requires that.

Why you don't want this? In case of installation with PHIVE, .phar will be dropped so user when saving wouldn't notice it.

Copy link
Owner

Choose a reason for hiding this comment

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

For everyone who looks at the github release and doesn’t use phive. It means the most downloadable looking thing in the release is a .phar, which is bound to cause confusion. I’ve never loved this about phive. I feel like it forces suboptimal experiences for non-phive users, unless I want to maintain my own repo server.

Copy link
Author

Choose a reason for hiding this comment

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

But is this actually a problem? Because your website already shows installation with wget and composer. And if users need compat versions, they will probably already know that.

However, if you don't want that, you can set up custom repository server. I think that you can just use GitHub Pages for this with some CI script that automatically updates them at the new release. If you think this would be better, I can try to create new PR for it.

Makefile Show resolved Hide resolved
@theofidry
Copy link
Contributor

theofidry commented Dec 1, 2019 via email

@bobthecow bobthecow closed this Mar 15, 2020
@bobthecow bobthecow reopened this Mar 15, 2020
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.

None yet

3 participants