ejabberd: 26.02 → 26.04#504293
Conversation
|
@chuangzhu … I think it might be failing on getting Twitter Bootstrap & jQuery for |
nixpkgs-ci
Bot
added
8.has: package (update)
We actually have two options: ifeq (, $(shell which npm))
INSTALL_INVITES_DEPS=tools/dl_invites_page_deps.sh priv/mod_invites/static
else
INSTALL_INVITES_DEPS=npm install
endifIf the deps get complicated, we can use something like |
diff --git a/pkgs/by-name/ej/ejabberd/package.nix b/pkgs/by-name/ej/ejabberd/package.nix
index bdec28abbf8a..3ec1606bf4af 100644
--- a/pkgs/by-name/ej/ejabberd/package.nix
+++ b/pkgs/by-name/ej/ejabberd/package.nix
@@ -20,6 +20,8 @@
fetchFromGitHub,
fetchgit,
beamPackages,
+ fetchurl,
+ unzip,
nixosTests,
withMysql ? false,
withPgsql ? false,
@@ -136,6 +138,17 @@ let
"ezlib"
];
+ invitePageDeps = {
+ jquery = fetchurl {
+ url = "https://code.jquery.com/jquery-4.0.0.min.js";
+ sha256 = "39a546ea9ad97f8bfaf5d3e0e8f8556adb415e470e59007ada9759dce472adaa";
+ };
+ bootstrap = fetchurl {
+ url = "https://github.com/twbs/bootstrap/releases/download/v5.3.8/bootstrap-5.3.8-dist.zip";
+ sha256 = "3258c873cbcb1e2d81f4374afea2ea6437d9eee9077041073fd81dd579c5ba6b";
+ };
+ };
+
in
stdenv.mkDerivation (finalAttrs: {
pname = "ejabberd";
@@ -150,6 +163,7 @@ stdenv.mkDerivation (finalAttrs: {
rebar3_hex
];
})
+ unzip # invitePageDeps
];
buildInputs = [
@@ -189,13 +203,19 @@ stdenv.mkDerivation (finalAttrs: {
]
++ lib.optional withSqlite "--with-sqlite3=${sqlite.dev}";
- enableParallelBuilding = true;
+ # It wants to execute two dl_invites_page_deps.sh in parallel
+ # Causing unzip conflicts
+ enableParallelBuilding = false;
postPatch = ''
patchShebangs .
mkdir -p _build/default/lib
touch _build/default/lib/.got
touch _build/default/lib/.built
+ sed -i \
+ -e 's|curl .* $jquery .*|cp ${invitePageDeps.jquery} $jquery|' \
+ -e 's|curl .* $bootstrap .*|cp ${invitePageDeps.bootstrap} $bootstrap|' \
+ tools/dl_invites_page_deps.sh
'';
env.REBAR_IGNORE_DEPS = 1; |
|
That’s one solution… doesn’t scale the best. Since they use SRIs, if we don’t have our versions in sync with upstream, we won’t know about breakage until runtime where we don’t match their hashes—which could mean a NixOS upgrade could make the invites not work entirely. Had they not included SRIs on the page, it would probably be fine/safe to be slightly out of sync on versions. I opened these upstream to see if they have any takes:
We could run the script in IFD perhaps to fetch (slow, but small deps) or use the npm Hooks (pulls in a massive, questionable toolchain just to fetch deps). Additionally, since |
I tested the patch I posted above with this: diff --git a/pkgs/by-name/ej/ejabberd/package.nix b/pkgs/by-name/ej/ejabberd/package.nix
index 3ec1606bf4af..160967eed12e 100644
--- a/pkgs/by-name/ej/ejabberd/package.nix
+++ b/pkgs/by-name/ej/ejabberd/package.nix
@@ -140,8 +140,8 @@ let
invitePageDeps = {
jquery = fetchurl {
- url = "https://code.jquery.com/jquery-4.0.0.min.js";
- sha256 = "39a546ea9ad97f8bfaf5d3e0e8f8556adb415e470e59007ada9759dce472adaa";
+ url = "https://code.jquery.com/jquery-3.0.0.min.js";
+ sha256 = "266bcea0bb58b26aa5b16c5aee60d22ccc1ae9d67daeb21db6bad56119c3447d";
};
bootstrap = fetchurl {
url = "https://github.com/twbs/bootstrap/releases/download/v5.3.8/bootstrap-5.3.8-dist.zip";And it totally noticed the checksum mismatch: I only changed |
I would argue it's not that bad (at least for now...). I've seen much worse |
|
I see what you did, but was I hoping we can have a better solution. However, having thought on it a bit, this could be good enough for now as it likely won’t be addressed in the near term. Hopefully it can trip on the version with I’d rather they ripped the SRIs out so I could recompile Bootstrap for a specific target (meaning Browserslist) or in line with 5.x upstream generally vs. Ejabberd’s specific version. Subresource integrity is best for using with third-party scripts (never ideal) & since these deps are already vendored & checked with their hashes, I don’t know what they are getting out of checking again at runtime.
True, but it also doesn’t make a lot of sense since a) you can disable the module & b) the docs say you can point to your own endpoint… both of which don’t need it. |
toastal
force-pushed
the
ejabberd-26.03
branch
2 times, most recently
from
499007c to
8e64623
Compare
|
While I have little skin in this game, I would recommend accepting that there's now nodejs in this package and using standard npm fetchers/builders.Using fetchers like this is a bit too much of a hack around the problem, in my opinion. |
| # despite every indication that it should be. Hopefully this is temporary. | ||
| # See: https://github.com/processone/ejabberd/issues/4554 | ||
| invitePageDeps = { | ||
| # Ideally this could be removed too as jQuery isn’t Bootstrap 5.x requirement |
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
|
I had looked into the npm option in the WIP (now reverted), but I didn’t like the way it was going just to fetch 2 dependencies. Since I thought this was not very good design, I reported it on their bug tracker & would rather see upstream’s response. At least we could gate Node.js behind Sorry, this whole issue really frustrated me being a, well I guess now ex-, front-end developer. |
toastal
force-pushed
the
ejabberd-26.03
branch
3 times, most recently
from
aed32d4 to
ca14d62
Compare
|
@chuangzhu @adamcstephens I patched with their open merge request to make Bootstrap optional (+ removing jQuery <3). This means users can opt out of the heft of the npm ecosystem if desired. What I mostly need help with: what is the default? |
toastal
force-pushed
the
ejabberd-26.03
branch
2 times, most recently
from
33bea0a to
6615532
Compare
|
I personally prefer to ship things as upstream intends with all features available, and let users that want to optimize override to do so. So I’d recommend enabled by default. |
toastal
force-pushed
the
ejabberd-26.03
branch
2 times, most recently
from
d0066ec to
ac798c2
Compare
| (fetchpatch2 { | ||
| # Makes Bootstrap optional, drops jQuery | ||
| # https://github.com/processone/ejabberd/pull/4558 | ||
| url = "https://patch-diff.githubusercontent.com/raw/processone/ejabberd/pull/4558.patch"; |
There was a problem hiding this comment.
This is not merged, we must vendor the patch
There was a problem hiding this comment.
I think the consensus is that this release isn’t going to be merged at all since that patch isn’t in the version.
| ++ lib.optional withRedis allBeamDeps.eredis; | ||
|
|
||
| npmDeps = | ||
| if npmToolingUsed then |
There was a problem hiding this comment.
| if npmToolingUsed then |
There was a problem hiding this comment.
This doesn’t work. The current setup allows Nixpkgs Ejabberd users to opt out (override { withBootstrap = false; }) of the npm toolchain if not using Bootstrap. This boolean gates pulling in such a large (but maintainable) toolchain.
There was a problem hiding this comment.
Just use withBootstrap instead of npmToolingUsed. Also, maybe use lib.optionalDrvAttr to make the code cleaner
There was a problem hiding this comment.
I don’t agree with that style since withBootstrap might not be the only long-term flag for if npm tooling is used & this boolean needs to be in 3 locations. I prefer descriptive names that think about the future when XXX & YYY feature is added that also needs npm added than
{
preBuild = lib.optionalString (withBootstrap || withXXX || withYYY) "…";
nativeBuildInputs = lib.optionals (withBootstrap || withXXX || withYYY) [ ];
npmDeps = lib.optionalDrvAttr (withBootstrap || withXXX || withYYY) [ ];
}npmToolingUsed is easy to extend in the future while being a self-documenting name.
There was a problem hiding this comment.
I did use lib.optionalDrvAttr which I did not know of.
| ++ lib.optional withRedis allBeamDeps.eredis; | ||
|
|
||
| npmDeps = | ||
| if npmToolingUsed then |
There was a problem hiding this comment.
Just use withBootstrap instead of npmToolingUsed. Also, maybe use lib.optionalDrvAttr to make the code cleaner
toastal
force-pushed
the
ejabberd-26.03
branch
3 times, most recently
from
48f48d3 to
0f2dffb
Compare
Not ideal to pull in the npm toolchain just for getting some static files (this is upstream’s call), but is less maintenance than trying to track the versions ourselves. This also means now that if you don’t plan to use the built-in mod_invites page, you can opt out of the toolchain + dependency.
as the patches are merged, the patch juggling can be removed
nixpkgs-ci
Bot
added
12.approvals: 1
|
|
5 participants
Things done
passthru.tests.nixpkgs-reviewon this PR. See nixpkgs-review usage../result/bin/.1
Footnotes
Please consider giving up MS GitHub or offering a non-proprietary, non-US-corporate-controlled mirror for this free software project. I wish to delete this Microsoft account in the future, but I need more projects like this to support alternative methods to send patches & contribute. ↩