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

Composer #3

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Composer #3

wants to merge 4 commits into from

Conversation

JaxkDev
Copy link
Member

@JaxkDev JaxkDev commented Nov 24, 2020

Writeup needed.

…d/installed dependencies users can now specify everything* they need in composer.json
- Sorry but it got messy.
- Default level changed to 5
- Added composer 'properly'
@JaxkDev JaxkDev requested a review from SOF3 February 15, 2021 20:51
@JaxkDev
Copy link
Member Author

JaxkDev commented Feb 15, 2021

PR Changes:

  • Most notably composer is now used to control what version pmmp & phpstan are used, however this also means any composer dependencies can also be used in analysis (even if plugins cant dynamically install composer deps via poggit CI yet)
  • Updated phpstan default config to reflect the changes below and above.
  • Paths have changed slightly, all data including plugin dependencies are now in /source
  • Composer v2 must be used to allow use of InstalledVersions.php (Absolutely amazing util saves the ugly hacks...)
  • PHPStan and PocketMine-MP default versions can be passed via -e when container started meaning they are no longer hard coded and do not require a rebuild of the image.
  • Bumped phpstan default level back up to 5 :)

Notes:

Basic tests passed

  • No composer file in plugin (use default)
  • Invalid composer file in plugin (bail)
  • valid composer file with no pmmp (use plugin specified phpstan and default pmmp)
  • valid composer file with no phpstan (use plugin specified pmmp and default phpstan)

TODO:

  • Somhow exclude the virion injected code from analysis but not scan. (excludePaths.analyse - But dont have an exact path...)
  • Test phpstan config files.
  • Test different ENV variables passed from poggit.
  • Test inside poggit (this was tested by standalone script replicating poggit behaviour)

@NhanAZ
Copy link

NhanAZ commented Jun 11, 2022

2 years. This pr is still alone.

@SOF3
Copy link
Member

SOF3 commented Jun 12, 2022

@NhanAZ how is this actually useful to most people anyway? It is just for phpstan analysis, and it's easier to do that on a github action.

@JaxkDev
Copy link
Member Author

JaxkDev commented May 31, 2023

This might be viable again since new virion spec 3.0 where virions are via composer.json too this solves the analysis problem of shaded virions.

Only remaining issue is PHP binary used.

@JaxkDev JaxkDev mentioned this pull request May 31, 2023
Closed
Copy link
Member

@dktapps dktapps left a comment

Choose a reason for hiding this comment

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

For the PHP version, I think the easiest thing would be to build and tag different images for PM4 and PM5, and then select the appropriate tag based on the API version declared in plugin.yml.

@@ -1,21 +1,41 @@
FROM pmmp/pocketmine-mp:latest
FROM ubuntu:18.04 as phpBuild
Copy link
Member

Choose a reason for hiding this comment

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

18.04 is obsolete

RUN bash compile.sh -t linux64 -f -u -g -l -j

# New slate to lose all unwanted libs (~300mb lost here)
FROM ubuntu:18.04
Copy link
Member

Choose a reason for hiding this comment

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

Same again here

RUN mkdir /php
RUN apt-get update && apt-get install --no-install-recommends -y ca-certificates wget make autoconf automake libtool-bin m4 gzip bzip2 bison g++ git cmake pkg-config re2c
WORKDIR /php
RUN wget -q https://github.com/pmmp/php-build-scripts/stable/compile.sh
Copy link
Member

Choose a reason for hiding this comment

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

This won't work for PM5.

@dktapps
Copy link
Member

dktapps commented May 31, 2023

Is this likely to be merged any time soon, or should I continue working with the existing image?

RUN apt-get update && apt-get install --no-install-recommends -y ca-certificates wget make autoconf automake libtool-bin m4 gzip bzip2 bison g++ git cmake pkg-config re2c
WORKDIR /php
RUN wget -q https://github.com/pmmp/php-build-scripts/stable/compile.sh
RUN bash compile.sh -t linux64 -f -u -g -l -j
Copy link
Member

Choose a reason for hiding this comment

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

These flags will need updating - -u is no longer used and -l now has a different purpose

also, -j expects a value for number of compile threads

@JaxkDev
Copy link
Member Author

JaxkDev commented May 31, 2023

Is this likely to be merged any time soon, or should I continue working with the existing image?

Ideally I want this to be the way forward, much cleaner just to use composer but it’s a very outdated PR
So anything big probable base off this anything small interim can just go straight

Hopefully I can update my local poggit once PM5 things are updated and continue continue some poggit duct tape

@JaxkDev
Copy link
Member Author

JaxkDev commented May 31, 2023

Also need to see if this is viable on the server, since it will probably result in higher resource usage per build

@SOF3
Copy link
Member

SOF3 commented Jun 1, 2023

Build rate is not very high, so server resources are sufficient. But I would still advise using GitHub actions for more flexibility.

@dktapps
Copy link
Member

dktapps commented Jun 1, 2023

But I would still advise using GitHub actions for more flexibility.

IMO this is missing the point. Poggit is still going to exist for a while to come and the system should work properly while people continue to use it. I use GitHub Actions for all my plugins, but they still have to be published by Poggit.

@SOF3
Copy link
Member

SOF3 commented Jun 1, 2023

I mean you could just disable the Poggit lint if you already use GitHub CI actively.

@dktapps
Copy link
Member

dktapps commented Jun 1, 2023

I would imagine it's helpful to reviewers to have a quick overview of the plugin's quality, no?

@dktapps
Copy link
Member

dktapps commented Jun 1, 2023

To be clear, the reason I started using Actions is because of Poggit CI's shortcomings, some of which this PR might resolve.

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.

4 participants