flarum: init at 1.8.1, module #311608
flarum: init at 1.8.1, module #311608
Conversation
|
Hi! Good idea to backport flarum from ngi-nix, did you used their repo as source for the code? |
Yes, only changes I made were updating the version and hash, some cleanup, and modifying this line to avoid this error: I'm open to suggestions especially from someone at ngi-nix, and very willing to add them as maintainer. Just give me a ping. |
|
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/prs-ready-for-review/3032/3967 |
Thanks for doing this @fsagbuya! I was able to build this successfully. Feel free to add me as second maintainer if you want. |
|
@jasonodoom, added you as maintainer. Thanks! |
|
@fsagbuya thanks for upstreaming this! last year one of the reasons we didn't upstream it was that no one wanted to be a maintainer in nixpkgs since we didn't use the software, but looks like @jasonodoom has changed his mind since 馃帀 @drupol thanks for linking the PRs to the ngipkgs repo. IIRC your recent comments in #foundation seemed to indicate you felt there were code quality issues with the flarum packaging work, so I was hoping you'd provide some more commentary on this upstreaming PR for our education Now that this PR has been merged, it's a good candidate for whenever ngipkgs implements ngi-nix/ngipkgs#76 |
Hello, My main concern has been addressed already, but I'd like to reiterate it here. I don't see the value in merging stuff into a repository outside I refrained from interfering with this PR, even though I could have requested some changes. I didn't want to block the merge over the use of a top-level I hope my message is clear. My goal is to consolidate nix recipes in their appropriate place and avoid fragmentation. Teaching contributors to work directly with |
| mkdir -p ${cfg.stateDir}/{extensions,public/assets/avatars} | ||
| mkdir -p ${cfg.stateDir}/storage/{cache,formatter,sessions,views} |
There was a problem hiding this comment.
This should use tmpfiles.d. A few months back was a treewide PR which converted many of those occurrences
| script = '' | ||
| mkdir -p ${cfg.stateDir}/{extensions,public/assets/avatars} | ||
| mkdir -p ${cfg.stateDir}/storage/{cache,formatter,sessions,views} | ||
| cd ${cfg.stateDir} |
There was a problem hiding this comment.
Working directory should be used instead
| cp -f ${cfg.package}/share/php/flarum/{extend.php,site.php,flarum} . | ||
| ln -sf ${cfg.package}/share/php/flarum/vendor . | ||
| ln -sf ${cfg.package}/share/php/flarum/public/index.php public/ | ||
| chmod a+x . public | ||
| chmod +x site.php extend.php flarum |
There was a problem hiding this comment.
This could all be covered by tmpfiles.d, too
| ensureUsers = [ | ||
| { | ||
| name = cfg.database.username; | ||
| ensurePermissions = { |
There was a problem hiding this comment.
Isn't this deprecated since months?
| # the database driver; i.e. MySQL; MariaDB... | ||
| driver = "mysql"; | ||
| # the host of the connection; localhost in most cases unless using an external service | ||
| host = "localhost"; | ||
| # the name of the database in the instance | ||
| database = "flarum"; | ||
| # database username | ||
| username = "flarum"; | ||
| # database password | ||
| password = ""; | ||
| # the prefix for the tables; useful if you are sharing the same database with another service | ||
| prefix = ""; | ||
| # the port of the connection; defaults to 3306 with MySQL |
There was a problem hiding this comment.
Those comments aren't visible for people visiting search.nixos.org
| description = "MySQL database parameters"; | ||
| default = { | ||
| # the database driver; i.e. MySQL; MariaDB... | ||
| driver = "mysql"; |
There was a problem hiding this comment.
Why not default to the more free mariadb?
| @@ -0,0 +1,210 @@ | |||
| { pkgs, lib, config, ... }: | |||
|
|
|||
| with lib; | |||
There was a problem hiding this comment.
Adding new file wide withs are highly discouraged
Description of changes
Based from #96869, ngi-nix/ngipkgs#47 and ngi-nix/ngipkgs#56.
Things done
nix.conf? (See Nix manual)sandbox = relaxedsandbox = truenix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/)Add a 馃憤 reaction to pull requests you find important.