discourse: update 3.1.0.beta4 -> 3.1.0 #250735

TheNeikos · 2023-08-22T10:06:42Z

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

matthiasbeyer · 2023-08-22T10:11:07Z

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

TheNeikos · 2023-08-22T10:12:23Z

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 · 2023-08-22T14:22:53Z

This is because of #240000 🫠

emilylange · 2023-08-22T14:41:03Z

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 · 2023-08-22T14:47:38Z

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 · 2023-08-23T07:43:29Z

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 · 2023-08-23T16:46:51Z

@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 · 2023-09-02T22:59:41Z

Are you still interested @TheNeikos?

TheNeikos · 2023-09-04T11:36:38Z

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

TheNeikos · 2023-09-10T11:55:10Z

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 · 2023-09-10T12:11:58Z

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 (fb267fc).

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 · 2023-09-11T05:56:47Z

@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 · 2023-09-12T16:33:28Z

@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 :)

The update.py script would error out if the spec had a trailing new line. The split that follows it would try to split it into two, and error out as it can't.

TheNeikos · 2023-09-13T13:02:39Z

@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

pkgs/servers/web-apps/discourse/rubyEnv/sass-embedded-static.patch

This was fixed globally in NixOS#240000 and thus is not needed here anymore.

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.

Suggested-by: talyz <kim.lindberger@gmail.com>

TheNeikos · 2023-09-13T18:32:41Z

@talyz Alrighty! Updated

drupol · 2023-09-14T07:55:26Z

I'm looking forward for this update!

Done

TheNeikos · 2023-09-14T14:38:18Z

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 · 2023-09-14T14:55:50Z

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 · 2023-09-14T14:55:54Z

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 · 2023-09-14T15:00:32Z

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

matthiasbeyer requested a review from emilylange August 22, 2023 10:09

ofborg bot requested a review from talyz August 22, 2023 11:07

ofborg bot added 10.rebuild-darwin: 0 10.rebuild-linux: 1-10 labels Aug 22, 2023

emilylange marked this pull request as draft August 22, 2023 16:09

NixOSInfra added the 12. first-time contribution label Aug 23, 2023

github-actions bot added the 6.topic: nixos label Sep 10, 2023

TheNeikos force-pushed the fix/discourse branch from ed86dcf to d9f86d4 Compare September 10, 2023 12:16

TheNeikos changed the title ~~discourse: fix missing SASS_EMBEDDED during build~~ discourse: update 3.1.0.beta4 -> 3.1.0 Sep 10, 2023

emilylange requested changes Sep 10, 2023

View reviewed changes

TheNeikos added 5 commits September 13, 2023 14:48

discourse: 3.1.0.beta4 -> 3.1.0

c87ad97

nixosTests.discourse: Do not use deprecated field

52f2257

discourse: Fix update.py handling newlines

77b7dc5

The update.py script would error out if the spec had a trailing new line. The split that follows it would try to split it into two, and error out as it can't.

discourseAllPlugins: Update all

c28a6bf

discourseAllPlugins.plugins.discourse-prometheus: Update ABI patch

48f63c5

TheNeikos force-pushed the fix/discourse branch from d9f86d4 to 674a38b Compare September 13, 2023 13:02

emilylange marked this pull request as ready for review September 13, 2023 13:10

emilylange requested changes Sep 13, 2023

View reviewed changes

pkgs/servers/web-apps/discourse/rubyEnv/sass-embedded-static.patch Outdated Show resolved Hide resolved

discourse: Remove sass-embedded patch

8e8a6ed

This was fixed globally in NixOS#240000 and thus is not needed here anymore.

TheNeikos force-pushed the fix/discourse branch from 674a38b to 8e8a6ed Compare September 13, 2023 16:03

talyz previously requested changes Sep 13, 2023

View reviewed changes

ofborg bot requested a review from talyz September 13, 2023 17:43

discourse: Use nodejs18

bbdccb8

Suggested-by: talyz <kim.lindberger@gmail.com>

emilylange approved these changes Sep 14, 2023

View reviewed changes

drupol merged commit d30fde0 into NixOS:master Sep 14, 2023
23 checks passed

TheNeikos deleted the fix/discourse branch September 14, 2023 14:29

matthiasbeyer added the backport release-23.05 label Sep 14, 2023

pdg137 mentioned this pull request Oct 21, 2023

Draft: discourse 3.1.0 backport to nixos-23.05 #262450

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

discourse: update 3.1.0.beta4 -> 3.1.0 #250735

discourse: update 3.1.0.beta4 -> 3.1.0 #250735

TheNeikos commented Aug 22, 2023 •

edited

Loading

matthiasbeyer commented Aug 22, 2023

TheNeikos commented Aug 22, 2023

emilylange commented Aug 22, 2023

emilylange commented Aug 22, 2023

kira-bruneau commented Aug 22, 2023 •

edited

Loading

TheNeikos commented Aug 23, 2023

emilylange commented Aug 23, 2023 •

edited

Loading

emilylange commented Sep 2, 2023

TheNeikos commented Sep 4, 2023

TheNeikos commented Sep 10, 2023

TheNeikos commented Sep 10, 2023

emilylange left a comment

TheNeikos commented Sep 11, 2023

emilylange commented Sep 12, 2023

TheNeikos commented Sep 13, 2023

emilylange left a comment

talyz left a comment

TheNeikos commented Sep 13, 2023

drupol commented Sep 14, 2023

TheNeikos commented Sep 14, 2023

github-actions bot commented Sep 14, 2023

matthiasbeyer commented Sep 14, 2023 •

edited

Loading

drupol commented Sep 14, 2023

discourse: update 3.1.0.beta4 -> 3.1.0 #250735

discourse: update 3.1.0.beta4 -> 3.1.0 #250735

Conversation

TheNeikos commented Aug 22, 2023 • edited Loading

Description of changes

Things done

matthiasbeyer commented Aug 22, 2023

TheNeikos commented Aug 22, 2023

emilylange commented Aug 22, 2023

emilylange commented Aug 22, 2023

kira-bruneau commented Aug 22, 2023 • edited Loading

TheNeikos commented Aug 23, 2023

emilylange commented Aug 23, 2023 • edited Loading

emilylange commented Sep 2, 2023

TheNeikos commented Sep 4, 2023

TheNeikos commented Sep 10, 2023

TheNeikos commented Sep 10, 2023

emilylange left a comment

Choose a reason for hiding this comment

TheNeikos commented Sep 11, 2023

emilylange commented Sep 12, 2023

TheNeikos commented Sep 13, 2023

emilylange left a comment

Choose a reason for hiding this comment

talyz left a comment

Choose a reason for hiding this comment

TheNeikos commented Sep 13, 2023

drupol commented Sep 14, 2023

TheNeikos commented Sep 14, 2023

github-actions bot commented Sep 14, 2023

matthiasbeyer commented Sep 14, 2023 • edited Loading

drupol commented Sep 14, 2023

TheNeikos commented Aug 22, 2023 •

edited

Loading

kira-bruneau commented Aug 22, 2023 •

edited

Loading

emilylange commented Aug 23, 2023 •

edited

Loading

matthiasbeyer commented Sep 14, 2023 •

edited

Loading