-
-
Notifications
You must be signed in to change notification settings - Fork 47
simplify init #288
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
simplify init #288
Conversation
Did you try to generate the phar file without those lines and verify that it works fine?
|
I tried to generate the phar (with But if I understand correctly the github action make a smoke test on the phar: |
The smoke test was done for that: test the phar generation. |
Which error do you get @fain182 ? |
bin-stub/phparkitect
Outdated
} | ||
|
||
require PHPARKITECT_COMPOSER_INSTALL; | ||
require __DIR__ . '/../vendor/autoload.php'; |
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.
AFAIK the only thing the above code is supposed to do is setting PHPARKITECT_COMPOSER_INSTALL as a way to say the package was installed via composer 🤔
To me it can be safely removed
On the other hand, I am not totally sure about adding this line: what is it effect?
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.
To me seems more like:
- try to find and autoloader (why? it's always in the same place)
- setup a warning if
composer install
has not been executed yet - include the autoload
The situation is different if we are searching the autoload of the project ouside of the phar, but I don't think it's this case.
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.
you are totally right... let's remove it then
the first error is:
If I correct the permission, it says something strange about PHP versions... |
If I remember correctly, the Box version we have works fine php 7.1, which is the version we use to generate the phar. Other 7.x version should work, 8.x version will not. More recent Box version will work with php 8, but they dropped the support for php 7.1 Long story short, if you want to generate the phar you need to have php 7 |
4cbc7a8
to
d5196f4
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #288 +/- ##
=========================================
Coverage 95.82% 95.82%
Complexity 657 657
=========================================
Files 76 76
Lines 1798 1798
=========================================
Hits 1723 1723
Misses 75 75 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
I tinkered a bit with it and for what I got we cannot just do a Context: we need to include the autoload file for phparkitect (not the one of the project we are analysing). As of now we do have two ways of using/installing phparkitect
When we use phparkitect as a phar the tool we use to package it takes care of embedding the autoload, so When we use phparkitect as a composer package the location of the phparkitect executable is configurable via composer json. It could be on the project root, project bin dir, vendor bin dir, and so on. In that case we need a way to guess where it is. From composer 2.2 (which I would say it covers the majority of cases) a variable is set by composer itself with the autoload path ( For previous composer versions a fallback mechanism is provided which tries to guess the bin location (not sure if it makes sense to support that) |
I think that's a good idea... |
|
I am not sure about this one, the different includes seems redundant, without this code the CI is green...
Any additional test we can make to check if this is useful?