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

php: add new Composer builder #248184

Merged
merged 9 commits into from
Sep 14, 2023
Merged

php: add new Composer builder #248184

merged 9 commits into from
Sep 14, 2023

Conversation

drupol
Copy link
Contributor

@drupol drupol commented Aug 9, 2023

Introducing a new builder for PHP Composer-Based Applications

Ensuring reproducibility and stability of outputs is required even when using an external dependency manager (Composer). As such, we've explored several possible solutions.

  1. The first attempt of this PR was using a Fixed Output Derivation strategy, using the Composer cache. Although this solution rectified numerous issues, it was not entirely secure, as it was heavily dependent on Composer. If Composer implemented even a minor structural alteration in the cache structure, it could break all derivations based on this builder. As a result, we concluded that this solution wasn't ideal.

  2. Our second attempt involved the use of fossar/composition-c4. This project reads the composer.lock file and constructs a local Composer repository, which is then used to install the dependencies. However, as Import From Derivation (IFD) is not permitted in nixpkgs, this approach didn't serve as a feasible solution. Moreover, path type repositories are currently unsupported, adding another roadblock.

  3. A proof-of-concept led to the creation of our last and final attempt; a custom Composer plugin, nix-community/composer-local-repo-plugin providing a new command. It mirrors the functionalities of fossar/composition-c4 and is written in PHP. It relies entirely on Composer to create a local Composer repository, thus emerging as a solid solution.

Although this plugin is currently hosted under my own name, there is a consideration to host it elsewhere, a decision yet to be made.

Here's a diagram overview on how the new builder is working

Nix PHP builder

This PR includes the following changes:

  • Introduces two new helpers (mkDerivation wrappers)
    • php.buildComposerProject
    • php.mkComposerRepository
  • Introduces two new hooks
    • php.composerHooks.composerRepositoryHook
    • php.composerHooks.composerInstallHook
  • Documentation

This new helpers significantly simplifies the process of integrating applications such as Symfony App, Wordpress or even Drupal into Nix, streamlining development and deployment.

Below is a practical example demonstrating how to efficiently package Drupal using this approach:

image

Or magento2:

image

Todo

  • Add a parameter that enable or disable the flag --no-dev

Upcoming PRs

  • Add a new helper that parse composer.json for extensions and instantiate the adequate php attribute with these extensions enabled. This has already been done in https://github.com/loophp/nix-shell/, we just need to formalize it in here.

@ofborg ofborg bot added the 8.has: package (new) This PR adds a new package label Aug 9, 2023
@ofborg ofborg bot requested review from aanderse, globin, etu, Ma27 and talyz August 9, 2023 20:13
@ofborg ofborg bot added 11.by: package-maintainer This PR was created by the maintainer of the package it changes 10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10 labels Aug 9, 2023
@ofborg ofborg bot requested a review from etu August 10, 2023 07:35
Copy link
Member

@n0emis n0emis left a comment

Choose a reason for hiding this comment

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

I've been testing this builder with some packages and find it very intuitive to use and am hoping to get it included soon. 👍🏼

@RaitoBezarius
Copy link
Member

@drupol except if you express strong rejection, I suggest that we move forward whatever is left to do to another PR and I can merge this now.

@drupol
Copy link
Contributor Author

drupol commented Sep 9, 2023

Give me an extra day so I can make last minor changes please :)

@ofborg ofborg bot added the 2.status: merge conflict This PR has merge conflicts with the target branch label Sep 10, 2023
@drupol drupol removed the 2.status: merge conflict This PR has merge conflicts with the target branch label Sep 10, 2023
@drupol drupol force-pushed the php/add-new-builder-only branch 3 times, most recently from d94242b to cb6c49c Compare September 11, 2023 08:06
@drupol drupol marked this pull request as ready for review September 12, 2023 14:39
@etu
Copy link
Contributor

etu commented Sep 13, 2023

I'm also thinking about commit messages.

Rather than the prefixes builder: and feat: maybe you should just put php:

I think it goes more in line with the contribution guidelines that way

EDIT: We can also just squash merge it using GitHub.

@RaitoBezarius
Copy link
Member

Actually, yes, we never have feat: in our commit messages in nixpkgs, all prefixes should ideally follow a semantic path to the contents: build-support/php is a good example.

@drupol
Copy link
Contributor Author

drupol commented Sep 13, 2023

Let's squash all of these commits during the merge, WDYT?

@RaitoBezarius
Copy link
Member

Let's squash all of these commits during the merge, WDYT?

I disagree, this is valuable history which is not that bad in terms of atomicity, losing it does not really make sense or bring any value IMHO.

@sinavir
Copy link

sinavir commented Sep 13, 2023

Hello, does this PR handle git (vcs ?) dependencies as specified here ?

If not may be it could be stated in the docs !

I'm trying to package the above software and the tooling introduced by this PR gives me this :

Failed to clone the git@github.com:LycheeOrg/phpstan-lychee.git repository, try running in interactive mode so that you can enter your GitHub credentials

In Filesystem.php line 255:

  /homeless-shelter/.cache/composer/vcs does not exist and could not be creat
  ed.

(the full nix expression is here : https://github.com/sinavir/sinavir/blob/lychee/lychee-nix/lychee.nix)

Nevertheless, this PR seems amazing ! Thanks for your work !

(PS: I'm not very familiar with composer so it may be just me not understanding how things are working)

@drupol
Copy link
Contributor Author

drupol commented Sep 13, 2023

Hello, does this PR handle git (vcs ?) dependencies as specified here ?

If not may be it could be stated in the docs !

I'm trying to package the above software and the tooling introduced by this PR gives me this :

Failed to clone the git@github.com:LycheeOrg/phpstan-lychee.git repository, try running in interactive mode so that you can enter your GitHub credentials

In Filesystem.php line 255:

  /homeless-shelter/.cache/composer/vcs does not exist and could not be creat
  ed.

(the full nix expression is here : https://github.com/sinavir/sinavir/blob/lychee/lychee-nix/lychee.nix)

Nevertheless, this PR seems amazing ! Thanks for your work !

(PS: I'm not very familiar with composer so it may be just me not understanding how things are working)

I tried locally and I get:

❯ composer i
No composer.lock file present. Updating dependencies to latest instead of installing from lock file. See https://getcomposer.org/install for more information.
Loading composer repositories with package information
GitHub API limit (60 calls/hr) is exhausted, could not fetch https://api.github.com/repos/LycheeOrg/php-exif/contents/composer.json?ref=c78ec611bea970d61cfbf721dc4b5abb33437901. Create a GitHub OAuth token to go over the API rate limit. You can also wait until 2023-09-13 16:02:22 for the rate limit to reset.

When working with _public_ GitHub repositories only, head to https://github.com/settings/tokens/new?scopes=&description=Composer+on+x13+2023-09-13+1502 to retrieve a token.
This token will have read-only permission for public information only.
When you need to access _private_ GitHub repositories as well, go to https://github.com/settings/tokens/new?scopes=repo&description=Composer+on+x13+2023-09-13+1502
Note that such tokens have broad read/write permissions on your behalf, even if not needed by Composer.
Tokens will be stored in plain text in "/home/pol/.config/composer/auth.json" for future use by Composer.
For additional information, check https://getcomposer.org/doc/articles/authentication-for-private-packages.md#github-oauth
Token (hidden):

The new builder should support this kind of repo, but I have the feeling that you reached the limit for accessing that repo, that's why you get the error. Maybe this is something we could improve at some point (I have no clue how yet).

@drupol
Copy link
Contributor Author

drupol commented Sep 13, 2023

Let's squash all of these commits during the merge, WDYT?

I disagree, this is valuable history which is not that bad in terms of atomicity, losing it does not really make sense or bring any value IMHO.

Ok going to rewrite all of them properly.

@drupol
Copy link
Contributor Author

drupol commented Sep 13, 2023

@GrahamcOfBorg build phpPackages.composer

@sinavir
Copy link

sinavir commented Sep 13, 2023

The new builder should support this kind of repo, but I have the feeling that you reached the limit for accessing that repo, that's why you get the error. Maybe this is something we could improve at some point (I have no clue how yet).

Ok, Thank you for the information. I don't have the throttling message and composer i is working for me inside pure nix-shell created using the (failing) derivation I linked above. I will try to debug this on my side.

@drupol
Copy link
Contributor Author

drupol commented Sep 13, 2023

Well well well.. @RaitoBezarius or @etu ?

@etu etu merged commit 350cac1 into master Sep 14, 2023
14 checks passed
@etu etu deleted the php/add-new-builder-only branch September 14, 2023 05:50
jleightcap pushed a commit to ngi-nix/ngipkgs that referenced this pull request Sep 15, 2023
Add a module definition for flarum, copied/inspired by this closed PR from a few years ago https://github.com/NixOS/nixpkgs/pull/96869/files#diff-d694d99e5b93515b9aeb968b1335e30f9349a205780f0f558d131988c7913dec

We commented out most of the module and added pieces back one by one, following the instructions at https://docs.flarum.org/install/
A key realization (which was already in the original PR) was that we could pass in a configuration file to php flarum install https://discuss.flarum.org/d/22187-command-line-install-via-php-flarum-install-no-interaction/5

Add https://github.com/loophp/nix-php-composer-builder/ as an input to the module because it's used in packaging

We are pinning it to 0caf5a60753397cfa959a74fc9ea0562556573c1 because that's what we developed against. Currently the latest version puts build artifacts in directories that are not compatible with our module definition. We will look into pointing to the latest of that repo or use the upstreamed version introduced in php: add new Composer builder NixOS/nixpkgs#248184
add a default configuration for flarum

Future work:

- as mentioned previous, update the php-composer-builder to a newer SHA or use the upstream version
- add testing using nixos tests pattern that pretalx introduced

Co-authored-by: Jason Odoom jasonodoom@gmail.com
Co-authored-by: Anish Lakhwara anish+git@lakhwara.com
Co-authored-by: Dominic Mills dominic.millz27@gmail.com
Co-authored-by: Albert Chae albertchae@users.noreply.github.com
Co-authored-by: Jack Leightcap jack@leightcap.com
lorenzleutgeb added a commit to ngi-nix/ngipkgs that referenced this pull request Sep 15, 2023
The implementation was upstreamed to nixpkgs, see

  NixOS/nixpkgs#248184
drupol added a commit to loophp/nix-php-composer-builder that referenced this pull request Sep 16, 2023
drupol added a commit to loophp/nix-php-composer-builder that referenced this pull request Sep 19, 2023
@yu-re-ka
Copy link
Contributor

yu-re-ka commented Oct 2, 2023

I'm a bit late on this, but does the buildComposerProject also have a guard against forgetting to update the FOD-hash? In yarn and Cargo builders, we include the lockfile in the fixed-output derivation, and then check that the lockfile in the deps FOD is the same as in src when building.

@drupol
Copy link
Contributor Author

drupol commented Oct 2, 2023

I don't think we have that ... yet?

@lorenzleutgeb
Copy link
Member

@drupol Regarding #237307 we wanted to execute unit and possibly integration tests as part of the derivation created via buildComposerProject. It seems that checkPhase is exposed, but PHPUnit tests have to run after composer install, which is in installPhase. Probably buildComposerProject should also expose installCheckPhase. We also checked some packages that use buildComposerProject and could not find one that actually runs tests. Happy to have your comment/explanation on how you thought that testing should be achieved with this mkDerivation wrapper. Thanks!

@drupol
Copy link
Contributor Author

drupol commented Oct 16, 2023

What do you think of this? Could you test please? #261429

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/contribute-to-the-nix-2023-recap/37484/8

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants