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. |
pkgs/by-name/ej/ejabberd/package.nix
Outdated
| # 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. |
| npmDeps = | ||
| if npmToolingUsed then | ||
| (fetchNpmDeps { | ||
| name = "${finalAttrs.pname}-${finalAttrs.version}-npm-deps"; | ||
| src = | ||
| if builtins.isNull finalAttrs.patches || builtins.length finalAttrs.patches <= 0 then | ||
| finalAttrs.src | ||
| else | ||
| # A bit of a hack, but if the package*.json files are patched, | ||
| # fetchNpmDeps will be out of sync | ||
| applyPatches { | ||
| inherit (finalAttrs) src patches; | ||
| name = "${finalAttrs.pname}-${finalAttrs.version}-patched"; | ||
| }; | ||
| hash = "sha256-MTyoc8ozrCi3W0CXmxyLpyU8v+vlUjcbLnv/1ev/Qqo="; | ||
| }) | ||
| else | ||
| null; |
There was a problem hiding this comment.
I understand your concern of jQuery being unnecessary, but I would rather ship jQuery than this...
There was a problem hiding this comment.
I would assume these flags + dropping jQuery are going to be merged for the next release since someone else opened the merge request… which to me would mean adding support for jQuery for 1 patch isn’t super useful.
pkgs.exatorrent.override { withUI = false; } takes this same approach (but doesn’t handle if patches are applied). The applyPatches could potentially be dropped once merged under the assumption folks generally won’t be needing to patch the package.json or package-lock.json.
What we get is the ability to not think about the version the are shipping, changing the update script.
There was a problem hiding this comment.
We could wait the upstream to release a new version. Cleaning their code isn't our job. Patches are used for something more critical. Your attempt to smuggle the patch just looks terrible
There was a problem hiding this comment.
I think seding the build script & adding dependencies to manually track is worse as even upstream has recognized the issue we had to hack around—& I would argue the patch unblocking the build while allowing Nix users to opt it/out is important (tho maybe waiting til it is merged makes more sense).
I’m actively already using this patch to test out the feature for my use case which I why I want it working without sending useless jQuery to my users (my users will be on metered mobile data). I can continue using this patch for my own setup until the next release, but I think this npm-based solution is going to be the easiest to manage long term as @adamcstephens was also suggesting—even if I hate Node.js now needing to be a nativeBuildInputs (otherwise trying to keep sync with their front-end deps is going to be unfun).
I’m not really sure what is wrong with this code block tho as it does exactly what it needs to do: 1) don’t pull in Node.js/npm if not used/needed (currently just withBootstrap), 2) use the package*.json files from the src, 3) patch the source if there are patches since AFAICT, there is no finalAttrs.finalResolvedSrc. Even if/when merged & with a new release, I don’t know that this block would want to change.
There was a problem hiding this comment.
This is ugly and complex. Either we remove it unconditional or not. Either is fine for me.
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.
| old_version=$(nix-instantiate --eval -A ejabberd.version | jq -e -r) | ||
|
|
||
| if [[ $version == "$old_version" ]]; then | ||
| if [ "$version" = "$old_version" ]; then |
There was a problem hiding this comment.
Why do we remove bashism in a script that is always executed by bash?
There was a problem hiding this comment.
It’s better to wrap the variable in a string in scripts. If touching the line, I don’t see why not increase compatibility in favor of Bash—what is Bash even getting you over POSIX [?
| touch _build/default/lib/.built | ||
| ''; | ||
|
|
||
| preBuild = lib.optionalString npmToolingUsed /* sh */ '' |
There was a problem hiding this comment.
| preBuild = lib.optionalString npmToolingUsed /* sh */ '' | |
| preBuild = lib.optionalString npmToolingUsed /* bash */ '' |
| (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.
| if builtins.isNull finalAttrs.patches || builtins.length finalAttrs.patches <= 0 then | ||
| finalAttrs.src | ||
| else |
There was a problem hiding this comment.
| if builtins.isNull finalAttrs.patches || builtins.length finalAttrs.patches <= 0 then | |
| finalAttrs.src | |
| else |
| # A bit of a hack, but if the package*.json files are patched, | ||
| # fetchNpmDeps will be out of sync |
There was a problem hiding this comment.
| # A bit of a hack, but if the package*.json files are patched, | |
| # fetchNpmDeps will be out of sync |
I think this is expected
There was a problem hiding this comment.
Everyone I have asked seems to have questions about this line since it lacks immediate intuition for “why”.
| ++ 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.
4 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. ↩