discourse: update 3.1.0.beta4 -> 3.1.0

[TheNeikos]

Fixes #249901: "discourse Build failure"

I've also built discourseAllPlugins without issues

Description of changes

The SASS_EMBEDDED env variable was not visible later in the build. This patch moves it to the preBuild phase, so that it is visible when needed.

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:
  • NixOS test(s) (look inside nixos/tests)
  • and/or package tests
  • or, for functions and "core" functionality, tests in lib/tests or pkgs/test
  • made sure NixOS tests are linked to the relevant packages
• 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.11 Release Notes (or backporting 23.05 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.

[matthiasbeyer]

Also @Ma27 and @kira-bruneau patched this expression lately... pinging you two here.

[TheNeikos]

I have no idea why the variable is not visible later after a postPatch. Putting it in the attrset as well as preBuild works though...

[emilylange]

This is because of #240000 🫠

[emilylange]

We should probably bump discourse from 3.1.0.beta4 to 3.1.0, which has been released in the meantime, first.
It looks to me like the patch (and overwrite) I added for #228716 might no longer be necessary.

[kira-bruneau]

Also @Ma27 and @kira-bruneau patched this expression lately... pinging you two here.

Oh yep, the change I made was to account for building dart-sass-embedded from source.

This is because of #240000 🫠

🫠 I like that general approach, but not really sure why it'd break discourse. It also looks like dart-sass-embedded isn't needed anymore either (now that the embedded part was merged into dart-sass). We should probably deprecate it once discourse is updated.

[TheNeikos]

Alright, I'm fairly new to this process but will gladly help out! Should I close this and instead try to update discourse? @emilylange

[emilylange]

@TheNeikos would be awesome if you could give updating discourse a try!
See pkgs/servers/web-apps/discourse/how_to_update.md

(It's rather difficult, but I am here if you need any help)

Feel free to either re-use this PR or create a new one :)

[emilylange]

Are you still interested @TheNeikos?

[TheNeikos]

Yes! I did not see that you replied two weeks ago... Lemme get to it

[TheNeikos]

PHEW

@emilylange

Okay, this was a wild ride. The update.py script had some assumptions about my system, so I had to fix that first. But today I took the time to do all that.

I followed the how_to_update guide, and will test it now on my own discourse instance. And report back

[TheNeikos]

Looks like it works?

I tried posting, and checked some of the plugins I had. All worked

Critically, I don't have the prometheus plugin, and don't see how test it faithfully.

[emilylange]

Wow, thank you a lot!

---

Just a few things:

I believe you can drop the whole dart-sass section now

diff --git a/pkgs/servers/web-apps/discourse/default.nix b/pkgs/servers/web-apps/discourse/default.nix
index 23f09f897a6f..b5064cd078ac 100644
--- a/pkgs/servers/web-apps/discourse/default.nix
+++ b/pkgs/servers/web-apps/discourse/default.nix
@@ -38,7 +38,6 @@
 , fixup_yarn_lock
 , nodePackages
 , nodejs_16
-, dart-sass-embedded
 , jq
 , moreutils
 
@@ -189,20 +188,6 @@ let
               cp $(readlink -f ${libpsl}/lib/libpsl.so) vendor/libpsl.x86_64.so
             '';
           };
-          sass-embedded = gems.sass-embedded // {
-            dontBuild = false;
-            # `sass-embedded` depends on `dart-sass-embedded` and tries to
-            # fetch that as `.tar.gz` from GitHub releases. That `.tar.gz`
-            # can also be specified via `SASS_EMBEDDED`. But instead of
-            # compressing our `dart-sass-embedded` just to decompress it
-            # again, we simply patch the Rakefile to symlink that path.
-            patches = [
-              ./rubyEnv/sass-embedded-static.patch
-            ];
-            preBuild = ''
-              export DART_SASS=${dart-sass-embedded}/bin
-            '';
-          };
         };
 
     groups = [
diff --git a/pkgs/servers/web-apps/discourse/rubyEnv/sass-embedded-static.patch b/pkgs/servers/web-apps/discourse/rubyEnv/sass-embedded-static.patch
deleted file mode 100644
index 86c343e30fd1..000000000000
--- a/pkgs/servers/web-apps/discourse/rubyEnv/sass-embedded-static.patch
+++ /dev/null
@@ -1,21 +0,0 @@
-diff --git a/ext/sass/Rakefile b/ext/sass/Rakefile
-index 4ca11d4f82ea..c0450ad6f8f3 100644
---- a/ext/sass/Rakefile
-+++ b/ext/sass/Rakefile
-@@ -18,15 +18,7 @@ file 'protoc.exe' do |t|
- end
- 
- file 'dart-sass' do |t|
--  raise if ENV.key?('DART_SASS')
--
--  gem_install 'sass-embedded', SassConfig.gem_version, SassConfig.gem_platform do |dir|
--    cp_r File.absolute_path("ext/sass/#{t.name}", dir), t.name
--  end
--rescue StandardError
--  archive = fetch(ENV.fetch('DART_SASS') { SassConfig.default_dart_sass })
--  unarchive archive
--  rm archive
-+  symlink(ENV.fetch('DART_SASS'), t.name)
- end
- 
- file 'cli.rb' => %w[dart-sass] do |t|

and drop your original commit titled discourse: fix missing SASS_EMBEDDED during build (fb267fcbe98823470653a3b62e261628cdbbb1c0).

Commit titles (current -> suggestion):

• discourse: Run update script and fix patch for it ->
  discourse: 3.1.0.beta4 -> 3.1.0
• discourse: Do not use deprecated field in discourse test ->
  nixosTests.discourse: do not use deprecated field
  (or nixos/tests/discourse: do not use deprecated field)
• discourse: Update discourse plugins ->
  discourseAllPlugins: update all
• discourse: Update ABI patch for discourse-prometheus ->
  discourseAllPlugins.plugins.discourse-prometheus: update ABI patch
  (or just keep it, I dunno -- no strong feelings. my suggestion feels a bit ridiculous, but this would be the proper attr name but oh well /shrug)

[TheNeikos]

@emilylange I don’t mind removing the patch, but given the comment it still feels applicable? Symlinking seems better than copying and unpacking something from the store.

I’ll fix the commit messages, thanks!

[emilylange]

@emilylange I don’t mind removing the patch, but given the comment it still feels applicable? Symlinking seems better than copying and unpacking something from the store.

That patch is redundant since #240000.
And given the one from #240000 is global, I'd argue we should just remove our local patch :)

[TheNeikos]

@emilylange Alright, here we go I think that's all your requests

[emilylange]

Thanks!

---

Result of nixpkgs-review pr 250735 run on x86_64-linux 1

2 packages built:

• discourse
• discourseAllPlugins

[talyz]

Thanks a lot for working on upgrading discourse!

nodejs-16.20.2 is marked as insecure, so it needs to be updated to nodejs_18 to work without permittedInsecurePackages = .... The tests don't pass on ofborg because of this.

This works for me:

diff --git a/pkgs/servers/web-apps/discourse/default.nix b/pkgs/servers/web-apps/discourse/default.nix
index c86725f1e49b..ed67a36d52fa 100644
--- a/pkgs/servers/web-apps/discourse/default.nix
+++ b/pkgs/servers/web-apps/discourse/default.nix
@@ -37,7 +37,7 @@
 , yarn
 , fixup_yarn_lock
 , nodePackages
-, nodejs_16
+, nodejs_18
 , dart-sass-embedded
 , jq
 , moreutils
@@ -162,9 +162,9 @@ let
                 cd ../..
 
                 mkdir -p vendor/v8/${stdenv.hostPlatform.system}/libv8/obj/
-                ln -s "${nodejs_16.libv8}/lib/libv8.a" vendor/v8/${stdenv.hostPlatform.system}/libv8/obj/libv8_monolith.a
+                ln -s "${nodejs_18.libv8}/lib/libv8.a" vendor/v8/${stdenv.hostPlatform.system}/libv8/obj/libv8_monolith.a
 
-                ln -s ${nodejs_16.libv8}/include vendor/v8/include
+                ln -s ${nodejs_18.libv8}/include vendor/v8/include
 
                 mkdir -p ext/libv8-node
                 echo '--- !ruby/object:Libv8::Node::Location::Vendor {}' >ext/libv8-node/.location.yml
@@ -212,7 +212,7 @@ let
       nodePackages.terser
       nodePackages.patch-package
       yarn
-      nodejs_16
+      nodejs_18
       jq
       moreutils
     ];

We usually update to the latest beta release, and 3.2.0.beta1 was just released. That's probably more work than just running the updater, though, so let's do that in a separate PR.

[TheNeikos]

@talyz Alrighty! Updated

[drupol]

I'm looking forward for this update!

Done

[TheNeikos]

Will this be backported into 23.05 or only available for 23.11 in the future? (not sure what the criteria is for updates like this)

[github-actions]

Backport failed for release-23.05, because it was unable to cherry-pick the commit(s).

Please cherry-pick the changes locally.

git fetch origin release-23.05
git worktree add -d .worktree/backport-250735-to-release-23.05 origin/release-23.05
cd .worktree/backport-250735-to-release-23.05
git checkout -b backport-250735-to-release-23.05
ancref=$(git merge-base 0987c80f48b63cfa0e9a12aafd99474079495ae5 bbdccb8eb9a4964f43c12e6eff8e35c714190f47)
git cherry-pick -x $ancref..bbdccb8eb9a4964f43c12e6eff8e35c714190f47

[matthiasbeyer]

Added the backport label. IMO this does clearly qualify for a backport, as it updates from a beta release to a stable release.

Lets see whether the bot can backport this. 😢

[drupol]

This is fine but the backport needs to be done manually :(