ejabberd: 26.02 → 26.03 #504293
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||
|---|---|---|---|---|---|---|---|---|
|
|
@@ -17,8 +17,11 @@ | |||||||
| gd, | ||||||||
| autoreconfHook, | ||||||||
| gawk, | ||||||||
| applyPatches, | ||||||||
| fetchFromGitHub, | ||||||||
| fetchgit, | ||||||||
| fetchNpmDeps, | ||||||||
| fetchpatch2, | ||||||||
| beamPackages, | ||||||||
| nixosTests, | ||||||||
| withMysql ? false, | ||||||||
|
|
@@ -35,6 +38,9 @@ | |||||||
| withRedis ? false, | ||||||||
| withImagemagick ? false, | ||||||||
| imagemagick, | ||||||||
| withBootstrap ? true, # used for the built-in mod_invites page | ||||||||
| nodejs, | ||||||||
| npmHooks, | ||||||||
| }: | ||||||||
|
|
||||||||
| let | ||||||||
|
|
@@ -76,7 +82,6 @@ let | |||||||
| builder = lib.makeOverridable buildRebar3; | ||||||||
|
|
||||||||
| overrides = final: prev: { | ||||||||
| cache_tab = prev.cache_tab.override { buildPlugins = [ beamPackages.pc ]; }; | ||||||||
| mqtree = prev.mqtree.override { buildPlugins = [ beamPackages.pc ]; }; | ||||||||
| stringprep = prev.stringprep.override { buildPlugins = [ beamPackages.pc ]; }; | ||||||||
|
|
@@ -137,10 +142,11 @@ let | |||||||
| "ezlib" | ||||||||
| ]; | ||||||||
|
|
||||||||
| npmToolingUsed = withBootstrap; | ||||||||
| in | ||||||||
| stdenv.mkDerivation (finalAttrs: { | ||||||||
| pname = "ejabberd"; | ||||||||
| version = "26.03"; | ||||||||
|
|
||||||||
| nativeBuildInputs = [ | ||||||||
| makeWrapper | ||||||||
|
|
@@ -151,6 +157,10 @@ stdenv.mkDerivation (finalAttrs: { | |||||||
| rebar3_hex | ||||||||
| ]; | ||||||||
| }) | ||||||||
| ] | ||||||||
| ++ lib.optionals npmToolingUsed [ | ||||||||
| nodejs | ||||||||
| npmHooks.npmConfigHook | ||||||||
| ]; | ||||||||
|
|
||||||||
| buildInputs = [ | ||||||||
|
|
@@ -166,13 +176,41 @@ stdenv.mkDerivation (finalAttrs: { | |||||||
| ++ lib.optional withLua allBeamDeps.luerl | ||||||||
| ++ lib.optional withRedis allBeamDeps.eredis; | ||||||||
|
|
||||||||
| 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 | ||||||||
|
Comment on lines
+184
to
+186
Member
There was a problem hiding this comment.
Suggested change
|
||||||||
| # A bit of a hack, but if the package*.json files are patched, | ||||||||
| # fetchNpmDeps will be out of sync | ||||||||
|
Comment on lines
+187
to
+188
Member
There was a problem hiding this comment.
Suggested change
I think this is expected
Contributor
Author
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”. |
||||||||
| applyPatches { | ||||||||
| inherit (finalAttrs) src patches; | ||||||||
| name = "${finalAttrs.pname}-${finalAttrs.version}-patched"; | ||||||||
| }; | ||||||||
| hash = "sha256-MTyoc8ozrCi3W0CXmxyLpyU8v+vlUjcbLnv/1ev/Qqo="; | ||||||||
| }) | ||||||||
| else | ||||||||
| null; | ||||||||
|
Comment on lines
+179
to
+196
Contributor
There was a problem hiding this comment. I understand your concern of jQuery being unnecessary, but I would rather ship jQuery than this...
Contributor
Author
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.
What we get is the ability to not think about the version the are shipping, changing the update script.
Contributor
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
Contributor
Author
There was a problem hiding this comment. I think 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 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
Member
There was a problem hiding this comment. This is ugly and complex. Either we remove it unconditional or not. Either is fine for me. |
||||||||
|
|
||||||||
| src = fetchFromGitHub { | ||||||||
| owner = "processone"; | ||||||||
| repo = "ejabberd"; | ||||||||
| tag = finalAttrs.version; | ||||||||
| hash = "sha256-M38niXEW++SPAvqQ2cqEd23+w7lBDO5EPgu/QRdYbXo="; | ||||||||
| }; | ||||||||
|
|
||||||||
| patches = [ | ||||||||
| (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"; | ||||||||
|
Member
There was a problem hiding this comment. This is not merged, we must vendor the patch
Contributor
Author
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. |
||||||||
| hash = "sha256-ETl2Zf7O6roxtf7DthJqL+tj4RvEfW94735sGM8x/GM="; | ||||||||
| }) | ||||||||
| ]; | ||||||||
|
|
||||||||
| passthru.tests = { | ||||||||
| inherit (nixosTests) ejabberd; | ||||||||
| }; | ||||||||
|
|
@@ -187,6 +225,7 @@ stdenv.mkDerivation (finalAttrs: { | |||||||
| (lib.enableFeature withLua "lua") | ||||||||
| (lib.enableFeature withTools "tools") | ||||||||
| (lib.enableFeature withRedis "redis") | ||||||||
| (lib.enableFeature withBootstrap "bootstrap") | ||||||||
| ] | ||||||||
| ++ lib.optional withSqlite "--with-sqlite3=${sqlite.dev}"; | ||||||||
|
|
||||||||
|
|
@@ -199,6 +238,10 @@ stdenv.mkDerivation (finalAttrs: { | |||||||
| touch _build/default/lib/.built | ||||||||
| ''; | ||||||||
|
|
||||||||
| preBuild = lib.optionalString npmToolingUsed /* sh */ '' | ||||||||
|
Member
There was a problem hiding this comment.
Suggested change
|
||||||||
| npm run postinstall | ||||||||
| ''; | ||||||||
|
|
||||||||
| env.REBAR_IGNORE_DEPS = 1; | ||||||||
|
|
||||||||
| postInstall = '' | ||||||||
|
|
||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.