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. |
This should be optional, but instead Bootstrap + jQuery are added to as dependency for building Ejabberd. jQuery is no longer a dependency of Bootstrap so it should be gone. The docs allow you to opt out of mod_invites or point to your own endpoint but the dependencies are now mistakenly required. The script to get the dependencies fetches with cURL which won’t run in the sandbox. Since SRIs are required for the runtime, it also means we can _only_ use this specific version of Bootstrap too.
|
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. |
3 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. ↩