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

Dolibarr #185258

Closed
wants to merge 3 commits into from
Closed

Dolibarr #185258

wants to merge 3 commits into from

Conversation

SCOTT-HAMILTON
Copy link
Contributor

@SCOTT-HAMILTON SCOTT-HAMILTON commented Aug 5, 2022

Description of changes

Fixes #184752
Fixes #181035

Things done

Documentation regarding the module and the test can be found on nixos.wiki

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 22.11 Release Notes (or backporting 22.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
    • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
  • Fits CONTRIBUTING.md.

@github-actions github-actions bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 6.topic: python 8.has: module (update) This PR changes an existing module in `nixos/` labels Aug 5, 2022
@ofborg ofborg bot requested a review from jraygauthier August 5, 2022 13:51
@nixos-discourse
Copy link

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

https://discourse.nixos.org/t/advice-for-packaging-a-web-app/20777/6

@aanderse
Copy link
Member

aanderse commented Aug 7, 2022

There are a number of issues with this. I'll try to find some time to highlight in the next few days.

@jraygauthier
Copy link
Member

The selenium change seems fine to me.

@aanderse
Copy link
Member

I'm actually working on this during my currently limited free time. Hopefully comments and results soon.

myphp = pkgs.php;
version = "15.0.3";
configFile = "/etc/${app}/conf.php";
dolibarr = with pkgs; stdenvNoCC.mkDerivation {
Copy link
Member

Choose a reason for hiding this comment

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

A separate package should be created for dollibar listing the proper dependencies.

'';
in stringAfter [ "etc" "groups" "users" ] ''
# Setup folders in /var/lib/${app}/documents
mkdir -p /var/lib/${app}/documents/{mycompany,medias,users,facture,propale,ficheinter,produit,doctemplates}
Copy link
Member

@jraygauthier jraygauthier Aug 12, 2022

Choose a reason for hiding this comment

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

I see some french folder names here. Is it expected or is this some company specific remnants?

Copy link
Member

Choose a reason for hiding this comment

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

I would assume this is because the author is french but it might be otherwise.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a fresh install and I'm sure it wasn't influenced by the LANG env var.

Copy link
Member

Choose a reason for hiding this comment

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

Ok, I would not be surprised, that typical of french author to leave some untranslated traces ;)

Copy link
Member

Choose a reason for hiding this comment

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

We should use tmpfiles.d for that.

];
};

system.activationScripts.dolibarr-init-script = let
Copy link
Member

Choose a reason for hiding this comment

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

Is there a good reason to run this as an activation script rather than as a service init script?

head -n 100 /dev/random| md5sum | cut -d' ' -f1 > $tmp_hash
[ ! -f /etc/${app}/conf.php ] && \
echo ${escapeShellArg initConfig} > /etc/${app}/conf.php && \
${pkgs.replace-secret}/bin/replace-secret '${passwordPlaceholder}' '${cfg.initialDbPasswordFile}' /etc/${app}/conf.php && \
Copy link
Member

Choose a reason for hiding this comment

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

No cfg.initialDbPasswordFile != null guard same as above? Any reason why?

@@ -31,6 +32,11 @@ buildPythonPackage rec {
cd py
'';

postInstall = ''
install -Dm 755 ../rb/lib/selenium/webdriver/atoms/getAttribute.js $out/${python.sitePackages}/selenium/webdriver/remote/getAttribute.js
Copy link
Member

Choose a reason for hiding this comment

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

Might be worth adding the ref to upstream issue (SeleniumHQ/selenium#9917) as part of the changeset's comment so that we can easily check (using blame) when this workaround is no longer required.

Copy link
Member

Choose a reason for hiding this comment

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

It might be a good idea to create a separate PR for the selenium change as it as more chances to get merged faster.

Copy link
Member

Choose a reason for hiding this comment

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

Might be worth adding the ref to upstream issue (SeleniumHQ/selenium#9917) as part of the changeset's comment so that we can easily check (using blame) when this workaround is no longer required.

The real problem is that we can't easily run their bazel build infrastructure and I fail to understand what needs to be run to only install those files.

as part of the changeset's comment

I am not sure what you mean by that. You want to add it to this commit?

@@ -0,0 +1,12228 @@
-- MariaDB dump 10.19 Distrib 10.6.8-MariaDB, for Linux (x86_64)
Copy link
Member

Choose a reason for hiding this comment

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

This is a quite huge file. I do not think it would be a good idea to add this to already bloated nixpkgs. Can this instead be fetched from an external location using a fetcher derivation?

Copy link
Member

Choose a reason for hiding this comment

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

Or alternatively generated?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, we must do something about this file.

@aanderse
Copy link
Member

I took some time to read through dolibarr documentation and source code. As always php applications are notoriously difficult to "properly" package for any distribution - NixOS adds a few extra complications to that.

I believe I came up with an ideal solution for packaging this application. I pushed my code here. I didn't finish the code, but I did test it a fair bit before making a few changes and pushing. A number of TODO items exist and have been marked. I like caddy for development but it is lightweight and easy to use, but I would recommend replacing caddy in this code with nginx.

If you're interested in adopting this code and putting on the finishing touches please feel free to run with it. I have no interest in this application but thought it would be a good example of how to package some php software like this for NixOS.

If you have any questions, comments, or need a hand to finish things up please don't hesitate to ask or ping me.

@RaitoBezarius
Copy link
Member

I'm quite interested into this package & module, @SCOTT-HAMILTON do you want to adapt the work of @aanderse or would you let me open a new PR to move forward this (of course, I can put you maintainer too) ?

@SCOTT-HAMILTON
Copy link
Contributor Author

I'm running short on time right now so feel free to open a new PR and to adapt @aanderse 's work. I will probably close this PR and open a new you one for the selenium fix.

@@ -31,6 +32,11 @@ buildPythonPackage rec {
cd py
'';

postInstall = ''
install -Dm 755 ../rb/lib/selenium/webdriver/atoms/getAttribute.js $out/${python.sitePackages}/selenium/webdriver/remote/getAttribute.js
Copy link
Member

Choose a reason for hiding this comment

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

Might be worth adding the ref to upstream issue (SeleniumHQ/selenium#9917) as part of the changeset's comment so that we can easily check (using blame) when this workaround is no longer required.

The real problem is that we can't easily run their bazel build infrastructure and I fail to understand what needs to be run to only install those files.

as part of the changeset's comment

I am not sure what you mean by that. You want to add it to this commit?

Comment on lines +95 to +97
machine.wait_for_unit("multi-user.target")
machine.wait_for_unit("nginx.service")
machine.wait_for_unit("mysql.service")
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
machine.wait_for_unit("multi-user.target")
machine.wait_for_unit("nginx.service")
machine.wait_for_unit("mysql.service")

This shouldn't be necessary if dolibarr sets its systemd options correct and we miss a testing potential here.

Comment on lines +99 to +108
machine.succeed("echo passwordfile is='${passwordFile}' >&2")
machine.succeed("cat ${passwordFile} >&2")
machine.succeed("cat ${mysqlCreds} >&2")
machine.succeed(
"echo 'SELECT User,Host,Password FROM mysql.user;' | mysql -u root >&2"
)
machine.succeed(
"echo 'SHOW DATABASES;' | mysql --defaults-extra-file=${mysqlCreds} -u dolibarr -N >&2"
)
machine.succeed("cat /etc/dolibarr/conf.php >&2")
Copy link
Member

Choose a reason for hiding this comment

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

This looks like debugging leftover to me.

Comment on lines +110 to +111
machine.succeed("${pkgs.mariadb}/bin/mysqldump -u root dolibarr > /tmp/dolibarr-db.sql")
machine.copy_from_vm("/tmp/dolibarr-db.sql")
Copy link
Member

Choose a reason for hiding this comment

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

Why do we dump this sql file? What do we do with it?

@@ -0,0 +1,12228 @@
-- MariaDB dump 10.19 Distrib 10.6.8-MariaDB, for Linux (x86_64)
Copy link
Member

Choose a reason for hiding this comment

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

Yes, we must do something about this file.

Comment on lines +42 to +54
description = ''
Whether to preinstall dolibarr or not.

<note><para>
The database is automatically configured with the following credentials:


<itemizedlist>
<listitem><para>login is <literal>dolibarrlogin</literal></para></listitem>
<listitem><para>password is <literal>123dolibarrlogin_pass</literal></para></listitem>
</itemizedlist>
</para></note>
'';
Copy link
Member

Choose a reason for hiding this comment

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

This should use mkDoc

with lib; {
options.services.dolibarr = {
enable = mkEnableOption "Dolibarr ERP &amp; CRM";
preInstalled = mkEnableOption "Preinstall dolibarr"// {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
preInstalled = mkEnableOption "Preinstall dolibarr"// {
preInstalled = mkEnableOption "Preinstall dolibarr" // {

Comment on lines +9 to +33
dolibarr = with pkgs; stdenvNoCC.mkDerivation {
pname = "dolibarr-src";
inherit version;
src = fetchFromGitHub {
owner = "Dolibarr";
repo = "dolibarr";
rev = version;
sha256 = "sha256-HMOYj93ZvqM0FQjt313yuGj/r9ELqQlnNkg/CxrBjRM=";
};
postPatch = ''
sed -i \
-e 's|\$conffile = .*|\$conffile = "${configFile}";|g' \
-e 's|\$conffiletoshow = .*|\$conffiletoshow = "${configFile}";|g' \
htdocs/filefunc.inc.php
sed -i \
-e 's|\$conffile = .*|\$conffile = "${configFile}";|g' \
-e 's|\$conffiletoshow = .*|\$conffiletoshow = "${configFile}";|g' \
htdocs/install/inc.php
'';
dontBuild = true;
installPhase = ''
mkdir -p "$out"
cp -r * $out
'';
};
Copy link
Member

Choose a reason for hiding this comment

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

THis should be in pkgs.

version = "15.0.3";
configFile = "/etc/${app}/conf.php";
dolibarr = with pkgs; stdenvNoCC.mkDerivation {
pname = "dolibarr-src";
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
pname = "dolibarr-src";
pname = "dolibarr";

{ config, lib, pkgs, options, ... }:
let
cfg = config.services.dolibarr;
app = "dolibarr";
Copy link
Member

Choose a reason for hiding this comment

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

What is the intention to use this variable in a lot of places? It makes the code less readable.

@RaitoBezarius
Copy link
Member

RaitoBezarius commented Sep 4, 2022 via email

@aanderse aanderse closed this Sep 4, 2022
@RaitoBezarius
Copy link
Member

In favor of #189635.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 6.topic: python 8.has: module (update) This PR changes an existing module in `nixos/` 10.rebuild-darwin: 11-100 10.rebuild-linux: 11-100
Projects
None yet
Development

Successfully merging this pull request may close these issues.

pythonPackages.selenium: FileNotFoundError: [Errno 2] No such file or directory, 'getAttribute.js' dolibarr
6 participants