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

flarum: init module at 1.8.0 #47

Merged
merged 1 commit into from
Sep 15, 2023
Merged

flarum: init module at 1.8.0 #47

merged 1 commit into from
Sep 15, 2023

Conversation

jleightcap
Copy link
Collaborator

@jleightcap jleightcap commented Sep 14, 2023

Closes #10

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

@jleightcap jleightcap force-pushed the moss/flarum-module branch 4 times, most recently from ffcbd5e to 9814670 Compare September 15, 2023 01:43
@albertchae albertchae changed the title flarum: init module at 1.7.0 flarum: init module at 1.8.0 Sep 15, 2023
@albertchae albertchae self-requested a review September 15, 2023 01:44
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
@jasonodoom jasonodoom merged commit 0d1d22b into main Sep 15, 2023
2 checks passed
@jasonodoom jasonodoom deleted the moss/flarum-module branch September 15, 2023 01:52
@drupol
Copy link

drupol commented Sep 15, 2023

Hello,

The PHP/Composer builder has been merged into Nix yesterday, you should be using that one instead of the one I maintain in an external flake!

Reason:

  1. The PHP/Composer is now official and you should use it by default
  2. A bug has already been fixed upstream and it is not fixed in https://github.com/loophp/nix-php-composer-builder/

Let me know if you need some assistance.

@@ -2,7 +2,7 @@
description = "NgiPkgs";

inputs.nixpkgs.url = "github:NixOS/nixpkgs/nixpkgs-unstable";
inputs.nix-php-composer-builder.url = "github:loophp/nix-php-composer-builder";
inputs.nix-php-composer-builder.url = "github:loophp/nix-php-composer-builder?ref=0caf5a60753397cfa959a74fc9ea0562556573c1";
Copy link
Member

Choose a reason for hiding this comment

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

Do you have a particular reason to pin this input to specific rev without a ref? If you just require a newer version, use nix flake lock --update-input nix-php-composer-builder.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, from the PR description

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 NixOS/nixpkgs#248184

Since we finally got to a state where things were working, we wanted to merge that as a baseline and then update it afterwards

@@ -112,7 +112,7 @@
// {
# The default module adds the default overlay on top of nixpkgs.
# This is so that `ngipkgs` can be used alongside `nixpkgs` in a configuration.
default.nixpkgs.overlays = [self.overlays.default];
default.nixpkgs.overlays = [self.overlays.default nix-php-composer-builder.overlays.default];
Copy link
Member

Choose a reason for hiding this comment

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

Why did you add this overlay here? This output is supposed to only contain modules that are defined in or deliberately re-exported by ngipkgs. But you seem to just consume it to use pkgs.api.buildComposerProject in pkgs/flarum/default.nix. Such a use-case should already be covered by line 51 in flake.nix.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not entirely sure but we didn't have buildComposerProject available to us at the time we were developing this and this allowed us to consume it, so it's possible after rebase on more recent changes that it became redundant. Regardless, we will be removing this to update to use the upstreamed buildComposerProject

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

When removed, we run into this error:

… while calling the 'head' builtin

         at /nix/store/2ayipj5kgy67231fvbl8agsl591kanw8-source/lib/attrsets.nix:820:11:

          819|         || pred here (elemAt values 1) (head values) then
          820|           head values
             |           ^
          821|         else

       … while evaluating the attribute 'value'

         at /nix/store/2ayipj5kgy67231fvbl8agsl591kanw8-source/lib/modules.nix:800:9:

          799|     in warnDeprecation opt //
          800|       { value = builtins.addErrorContext "while evaluating the option `${showOption loc}':" value;
             |         ^
          801|         inherit (res.defsFinal') highestPrio;

       (stack trace truncated; use '--show-trace' to show the full trace)

       error: attribute 'api' missing

       at /nix/store/why05jnspyk66zll3dk404r4rpmnpp6c-source/pkgs/flarum/default.nix:7:1:

            6| }:
            7| pkgs.api.buildComposerProject (finalAttrs: {
             | ^
            8|   pname = "flarum";
       Did you mean one of acpi, agi, ali, ape or apg?       

If you happen to know why this is we can apply a quick fix.
Otherwise this will be made redundant when we refactor to use the upstream PHP builder anyway.

Copy link

Choose a reason for hiding this comment

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

Here's an example on how to use it : NixOS/nixpkgs#255122

@lorenzleutgeb
Copy link
Member

In reply to @drupol:

The PHP/Composer builder has been merged into Nix yesterday, you should be using that one instead of the one I maintain in an external flake!

The PR made it to master, but not yet to unstable. I prepared main...flarum which should just need another nix flake lock --update-inputs nix-php-composer-builder as soon as unstable is ready.

@albertchae
Copy link
Contributor

In reply to @drupol:

The PHP/Composer builder has been merged into Nix yesterday, you should be using that one instead of the one I maintain in an external flake!

The PR made it to master, but not yet to unstable. I prepared main...flarum which should just need another nix flake lock --update-inputs nix-php-composer-builder as soon as unstable is ready.

We can use this branch as a starting point but we will also need to rework some of the module definition in flarum.nix to correspond to the changed directory structure as mentioned in #47 (comment)

@albertchae
Copy link
Contributor

We updated to use the upstream composer builder and fix the module definition in #56

@drupol
Copy link

drupol commented Sep 18, 2023

Excellent :)

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

Successfully merging this pull request may close these issues.

Flarum
5 participants