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

odoo fixes #327729

Merged
merged 8 commits into from
Jul 19, 2024
Merged

odoo fixes #327729

merged 8 commits into from
Jul 19, 2024

Conversation

zimbatm
Copy link
Member

@zimbatm zimbatm commented Jul 16, 2024

Description of changes

Banging Odoo 17 into shape.

  • odoo: add missing lxml-html-clean dependency
  • odoo: add missing geoip2 dependency
  • nixos/odoo: use env vars to configure Odoo
  • nixos/odoo: add autoInit option
  • nixos/odoo: set data_dir explicitly

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • 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/)
  • 24.11 Release Notes (or backporting 23.11 and 24.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
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

Fixes this runtime error:

    lxml.html.clean module is now a separate project lxml_html_clean.
    Install lxml[html_clean] or lxml_html_clean directly.
This allows running multiple commands with the same settings.
When enabled, Odoo will automatically initialize the database on
startup.
@zimbatm
Copy link
Member Author

zimbatm commented Jul 16, 2024

A follow-up to #327641

@@ -113,7 +115,9 @@ in
"HOME=%S/odoo"
"ODOO_RC=${cfgFile}"
];
};
} // (lib.optionalAttrs cfg.autoInit {
ExecStartPre = "${cfg.package}/bin/odoo --init=INIT --database=odoo --db_user=odoo --stop-after-init";
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 pessimistic that all init steps are idempotent, such that running this before each start won't mess up a database with runtime data.

Might I suggest something like:

INITIALIZED="${cfg.data_dir}/.odoo.initialized"
if [ ! -e "$INITIALIZED" ]; then
  "${cfg.package}/bin/odoo --init=INIT --database=odoo --db_user=odoo --stop-after-init"
  touch "$INITIALIZED"
fi

Copy link
Member Author

Choose a reason for hiding this comment

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

👍 It seems to work, but better safe than sorry.

Copy link
Member

Choose a reason for hiding this comment

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

I assume, if plugins are changed init does not need to be re-run? Then 👍

@siriobalmelli
Copy link
Contributor

I also recommend removing the following from nixos/tests/odoo.nix:

      # odoo does not automatically initialize its database,
      # even if passing what _should_ be the equivalent of these options:
      #  settings = {
      #    options = {
      #      database = "odoo";
      #      init = "base";
      #    };
      #  };
      systemd.services.odoo.preStart = ''
        HOME=$STATE_DIRECTORY ${package}/bin/odoo -d odoo -i base --stop-after-init --without-demo all
      '';

@siriobalmelli
Copy link
Contributor

I also recommending adding a cfg.autoInitOptions option which can contain eg --without-demo all

@zimbatm if you like I'm happy to add commits on top of this series with my proposals.

@siriobalmelli
Copy link
Contributor

Result of nixpkgs-review pr 327729 run on x86_64-linux 1

2 packages blacklisted:
  • nixos-install-tools
  • tests.nixos-functions.nixos-test
2 packages built:
  • odoo
  • odoo.dist

Don't hide the data is a private sub-folder.

Before:
* /var/lib/private/odoo/.local/share/Odoo/
After:
* /var/lib/private/odoo/data
@zimbatm
Copy link
Member Author

zimbatm commented Jul 17, 2024

Thanks, that's good feedback. I will stop force-pushing if you want to add your changes on top. Otherwise I'll get back to it tomorrow.

@siriobalmelli
Copy link
Contributor

siriobalmelli commented Jul 18, 2024

@zimbatm done, but I don't have commit access to append to this PR. I'm providing the proposed commits either of 2 ways:

  1. branch off of your PR branch in my fork: https://github.com/siriobalmelli/nixpkgs/tree/pr-327729

  2. patchset:

Copy link
Contributor

@siriobalmelli siriobalmelli left a comment

Choose a reason for hiding this comment

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

LGTM, proposed changes as per my previous message

Signed-off-by: Sirio Balmelli <sirio@b-ad.ch>
Allow module user to specify additional flags to be passed on autoInit

Signed-off-by: Sirio Balmelli <sirio@b-ad.ch>
Signed-off-by: Sirio Balmelli <sirio@b-ad.ch>
@zimbatm
Copy link
Member Author

zimbatm commented Jul 19, 2024

thanks, pushed your patches and waiting on ofBorg

@ofborg ofborg bot requested a review from siriobalmelli July 19, 2024 10:25
@zimbatm zimbatm merged commit 676e42a into NixOS:master Jul 19, 2024
20 of 22 checks passed
@zimbatm zimbatm deleted the odoo-fixes branch July 19, 2024 10:35
@zimbatm zimbatm restored the odoo-fixes branch July 19, 2024 11:40
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.

None yet

3 participants