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

thelounge: fix build #233511

Merged
merged 2 commits into from
May 28, 2023
Merged

thelounge: fix build #233511

merged 2 commits into from
May 28, 2023

Conversation

winterqt
Copy link
Member

@winterqt winterqt commented May 22, 2023

Description of changes

Upstream switched to using TypeScript in v4.4.0, which broke the patch. This fixes that issue by migrating to building The Lounge from source, instead of having to patch the minified JavaScript.

Closes #233630.

Things done
  • 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/)
  • 23.05 Release Notes (or backporting 22.11 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.

Copy link
Member

@lilyinstarlight lilyinstarlight left a comment

Choose a reason for hiding this comment

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

Assuming ofborg shows all green, this looks good to me (really want those yarn hooks though and my PR to add a fixup-yarn-lock command independent of yarn2nix...)

@JulienMalka JulienMalka added the 0.kind: ZHF Fixes Fixes during the ZHF campaign label May 23, 2023
@RaitoBezarius
Copy link
Member

I'm going to be a guinea pig for that, pushing in production.

@RaitoBezarius
Copy link
Member

Production is running with it, without any hiccups. :)

Copy link
Member

@RaitoBezarius RaitoBezarius left a comment

Choose a reason for hiding this comment

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

LGTM!

@winterqt winterqt marked this pull request as ready for review May 23, 2023 21:00
@winterqt
Copy link
Member Author

Waiting on OfBorg.

@winterqt
Copy link
Member Author

Actually, let me try something else... I want to reduce this closure size.

@winterqt
Copy link
Member Author

Before this push, it was just about 400 MiB, now it's 81 MiB.

Copy link
Member

@RaitoBezarius RaitoBezarius left a comment

Choose a reason for hiding this comment

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

LGTM

@ofborg ofborg bot requested a review from RaitoBezarius May 23, 2023 21:48
Copy link
Member

@lilyinstarlight lilyinstarlight left a comment

Choose a reason for hiding this comment

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

This looks good to me too. I'll second Sandro's minor lexicographical ordering nit, but the dontNpmPrune install hook change looks fine enough for me

(I wish it were reasonable to separate prune/install into separate hooks entirely, but it really sorta has to happen in the middle there, so this is acceptable for now)

In some odd scenarios, `npm prune` either fails, or hangs. I have no idea
what could possibly be wrong at the moment, but let's provide an escape
hatch for packages that can still use the rest of the install hook's
functionality.
Upstream switched to using TypeScript in v4.4.0, which broke the patch.
This fixes that issue by migrating to building The Lounge from source,
instead of having to patch the minified JavaScript.
@winterqt
Copy link
Member Author

winterqt commented May 28, 2023

Should be good to go now. Waiting on OfBorg.

@winterqt winterqt merged commit 41bb263 into NixOS:master May 28, 2023
@winterqt winterqt deleted the fix-thelounge branch May 28, 2023 04:47
@github-actions
Copy link
Contributor

Successfully created backport PR for release-23.05:

@Tom-Hubrecht
Copy link
Contributor

Production is running with it, without any hiccups. :)

It seems that it is not really without hiccups, when upgrading, the service launches, but the message history cannot be read, as it throws :
thelounge[158468]: 2023-05-29 13:41:45 [ERROR] Unable to load sqlite3 module. See https://github.com/mapbox/node-sqlite3/wiki/Binaries

@winterqt
Copy link
Member Author

I need more details, please -- what system are you running it on? This seemingly can't be reproduced in CI, or on Ryan's end.

@shyim
Copy link
Member

shyim commented May 29, 2023

I have the same aarch64-linux my config: https://github.com/shyim/nixcfg/blob/main/systems/shea/services/thelounge.nix

@Tom-Hubrecht
Copy link
Contributor

I need more details, please -- what system are you running it on? This seemingly can't be reproduced in CI, or on Ryan's end.

I am running thelounge on an x86_64 server, the config is at https://git.hubrecht.ovh/hubrecht/nixos/src/branch/master/machines/sd-02/thelounge.nix

The issue seems to be that old messages are no longer shown, but new messages still are (I am next to Ryan's guinea pig who confirmed the issue)

@RaitoBezarius
Copy link
Member

I need more details, please -- what system are you running it on? This seemingly can't be reproduced in CI, or on Ryan's end.

I confirm that my users doesn't see their messages, but the logs are on disk and the SQLite DB is intact.

@winterqt
Copy link
Member Author

Let me look into this, please give me a bit.

@RaitoBezarius
Copy link
Member

I do have this:

May 23 16:15:00 amadeus thelounge[108838]: 2023-05-23 14:15:00 [ERROR] Unable to load sqlite3 module. See https://github.com/mapbox/node-sqlite3/wiki/Binaries
May 23 16:15:00 amadeus thelounge[108838]: 2023-05-23 14:15:00 [INFO] The Lounge v4.4.0 (Node.js 18.16.0 on linux x64)
May 23 16:15:00 amadeus thelounge[108838]: 2023-05-23 14:15:00 [INFO] Configuration file: /var/lib/thelounge/config.js
May 23 16:15:01 amadeus thelounge[108838]: 2023-05-23 14:15:01 [INFO] Available at http://127.0.0.1:9000/ in private mode

in my systemd logs, FWIW.

@winterqt Thank you for looking into it (and I have no worries, I have backups and all.) :)

It's annoying that upstream didn't make the error fatal… :/

@winterqt winterqt mentioned this pull request May 29, 2023
12 tasks
winterqt added a commit to winterqt/nixpkgs that referenced this pull request Jun 3, 2023
Previously, we never actually built the SQLite binding, causing The Lounge
to bail when attempting to load SQLite logs [0]. It wasn't caught before
because it wasn't thrown fatally, for whatever reason. Perhaps we should
fix this in the future with a patch and/or more robust tests, but for now,
let's just fix the issue.

[0]: NixOS#233511 (comment)
github-actions bot pushed a commit that referenced this pull request Jun 3, 2023
Previously, we never actually built the SQLite binding, causing The Lounge
to bail when attempting to load SQLite logs [0]. It wasn't caught before
because it wasn't thrown fatally, for whatever reason. Perhaps we should
fix this in the future with a patch and/or more robust tests, but for now,
let's just fix the issue.

[0]: #233511 (comment)

(cherry picked from commit a1cfd90)
winterqt added a commit that referenced this pull request Jun 4, 2023
Previously, we never actually built the SQLite binding, causing The Lounge
to bail when attempting to load SQLite logs [0]. It wasn't caught before
because it wasn't thrown fatally, for whatever reason. Perhaps we should
fix this in the future with a patch and/or more robust tests, but for now,
let's just fix the issue.

[0]: #233511 (comment)

(cherry picked from commit a1cfd90)
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.

Build failure: thelounge
7 participants