Skip to content

Conversation

fain182
Copy link
Collaborator

@fain182 fain182 commented Aug 19, 2022

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?

@AlessandroMinoccheri
Copy link
Member

Did you try to generate the phar file without those lines and verify that it works fine?
Can we automate this check?

  • Generate phar
  • using phar with some rule with some files

@AlessandroMinoccheri AlessandroMinoccheri added the enhancement New feature or request label Aug 19, 2022
@fain182
Copy link
Collaborator Author

fain182 commented Aug 19, 2022

I tried to generate the phar (with make phar) but it failed, even on the main branch..

But if I understand correctly the github action make a smoke test on the phar:
https://github.com/phparkitect/arkitect/blob/main/.github/workflows/build.yml#L116

@AlessandroMinoccheri
Copy link
Member

The smoke test was done for that: test the phar generation.
@micheleorselli what do you think?

@AlessandroMinoccheri
Copy link
Member

Which error do you get @fain182 ?
It's about /tmp/.. permission, right?

}

require PHPARKITECT_COMPOSER_INSTALL;
require __DIR__ . '/../vendor/autoload.php';
Copy link
Member

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?

Copy link
Collaborator Author

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.

Copy link
Member

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

@fain182
Copy link
Collaborator Author

fain182 commented Aug 30, 2022

Which error do you get @fain182 ? It's about /tmp/.. permission, right?

the first error is:

bin/box.phar compile -c /tmp/arkitect/box.json
make: bin/box.phar: Permesso negato
make: *** [Makefile:26: phar] Errore 127

If I correct the permission, it says something strange about PHP versions...
Schermata del 2022-08-30 14-33-26

@micheleorselli
Copy link
Member

micheleorselli commented Aug 30, 2022

Which error do you get @fain182 ? It's about /tmp/.. permission, right?

the first error is:

bin/box.phar compile -c /tmp/arkitect/box.json
make: bin/box.phar: Permesso negato
make: *** [Makefile:26: phar] Errore 127

If I correct the permission, it says something strange about PHP versions... Schermata del 2022-08-30 14-33-26

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

@fain182 fain182 added this to the v1 milestone Dec 14, 2022
Copy link

codecov bot commented Apr 20, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.82%. Comparing base (5b5edfb) to head (014e007).
Report is 1 commits behind head on main.

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.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@micheleorselli
Copy link
Member

micheleorselli commented Apr 20, 2025

I tinkered a bit with it and for what I got we cannot just do a require __DIR__ . '/../vendor/autoload.php';

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

  • as a phar
  • as a composer package

When we use phparkitect as a phar the tool we use to package it takes care of embedding the autoload, so require __DIR__ . '/../vendor/autoload.php'; will work ✅

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 ($_composer_autoload_path). It that variable is set we can use it and that's it.

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)

wdyt @AlessandroMinoccheri @fain182?

@fain182
Copy link
Collaborator Author

fain182 commented Apr 21, 2025

I think that's a good idea...
And I agree that supporting older composer version is excessive.

Copy link

@micheleorselli micheleorselli merged commit 45bd46f into main Apr 24, 2025
35 checks passed
@micheleorselli micheleorselli deleted the phar-experiment branch April 24, 2025 07:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants