Redmine

Thank you for posting the patches.

I think we have to wait until the release of Redmine 5.1.0 to upgrade Rails because 5.1.0 should be Rails 6.1 based.

https://endoflife.date/rails

6.1
(01 Jun 2024)

https://github.com/rails/rails/pull/46895#issuecomment-1673353127

6.1 is not getting anymore releases unless it's a severe vulnerability and will be EOL next June.

I think Redmine needs to upgrade to Rails 7 before Rails 6.1 EOL.

Not a bad effort there Takashi Kato, I gave your patch a quick go with Rails 7.1 and it seemed to work with standard Redmine 5.1 release branch.

However one thing that got broke is the autoloading of classes and modules from plugin folders, not sure why. Perhaps you could check this out... I know there have been some subtle changes in this area in Rails. (Also is there a reason you deleted the zeitwerk config line?)

Cheers
MT

Thanks!

Can you take a look also on the failing tests from here? I will do it as well this week.

Takashi Kato wrote in #note-16:

Sorry for your time.
It is difficult to fix autoload_paths after initialize, so I've changed the method.

• This somewhat seems to converge with #24007 (albeit not as a user config option directly) and as such has the issue mentioned in #24007#note-13 that 'plugins' directory remains hard-coded in source:/trunk/Gemfile@22489#L125.
• The comments included in this patch contain a typo (s/tesr/test).

Takashi Kato wrote in #note-16:

Sorry for your time.
It is difficult to fix autoload_paths after initialize, so I've changed the method.

I was conducting an investigation without realizing that a patch had already been posted. Akira Matsuda assisted me in this research.
The problem is likely to have been caused by the changes to https://github.com/rails/rails/commit/ebfca905db .

The following changes will also solve the problem:

diff --git a/lib/redmine/plugin_loader.rb b/lib/redmine/plugin_loader.rb
index 2151e949b..e1b58baa7 100644
--- a/lib/redmine/plugin_loader.rb
+++ b/lib/redmine/plugin_loader.rb
@@ -126,7 +126,7 @@ module Redmine
         # Add the plugin directories to rails autoload paths
         engine_cfg = Rails::Engine::Configuration.new(directory.to_s)
         engine_cfg.paths.add 'lib', eager_load: true
-        engine_cfg.eager_load_paths.each do |dir|
+        engine_cfg.all_eager_load_paths.each do |dir|
           Rails.autoloaders.main.push_dir dir
           Rails.application.config.watchable_dirs[dir] = [:rb]
         end

https://github.com/redmica/redmica_ui_extension plugin can reproduce autoloading-related problems.

Thanks Takashi Kato and Mizuki ISHIKAWA for investigating this issue, I've committed the fix with all_eager_load_paths because it seems to call both methods eager_load_paths + paths.eager_load.

Committed also the test and the changes required for the test to pass, thanks!

Mischa The Evil wrote in #note-18:

[...]

Just my two cents on this patch:

• This somewhat seems to converge with #24007 (albeit not as a user config option directly) and as such has the issue mentioned in #24007#note-13 that 'plugins' directory remains hard-coded in source:/trunk/Gemfile@22489#L125.

It looks like we are close to support changing the "plugins" directory path from the configuration file, but we still need more time to invest in order to properly support and document this change.

• https://www.redmine.org/builds/logs/build_trunk_postgresql_ruby-3.2_878.html
• https://www.redmine.org/builds/logs/build_trunk_mysql_ruby-3.2_877.html
• https://www.redmine.org/builds/logs/build_trunk_mysql_ruby-3.2_878.html (pass)

Is someone able to reproduce the problem on the local environment?

Marius BALTEANU wrote in #note-25:

Is someone able to reproduce the problem on the local environment?

I encountered a test failure in project_nested_set_concurrency_test.rb on my M2 chip MacBook Air using a Docker environment (with postgres:14.9 and ruby:2.7.8).

I'm not sure of the reason, but it seems to be related to https://github.com/rack/rack-session#compatibility. After modifying the Gemfile as follows and running bundle update, the test for project_nested_set_concurrency_test.rb passed with the seed value that previously led to failure.

diff --git a/Gemfile b/Gemfile
index 908c32343..d5870ad95 100644
--- a/Gemfile
+++ b/Gemfile
@@ -3,6 +3,7 @@ source 'https://rubygems.org'
 ruby '>= 2.7.0', '< 3.3.0'

 gem 'rails', '7.1.2'
+gem 'rack-session', '~> 1.0'
 gem 'rouge', '~> 4.2.0'
 gem 'mini_mime', '~> 1.1.0'
 gem "actionpack-xml_parser" 

Minoru Maeda wrote in #note-27:

I'm not sure of the reason, but it seems to be related to https://github.com/rack/rack-session#compatibility. After modifying the Gemfile as follows and running bundle update, the test for project_nested_set_concurrency_test.rb passed with the seed value that previously led to failure.

I've discovered the reason for the failure of the test in project_nested_set_concurrency_test.rb. The issue appears to be that assert_difference is evaluating Project.count based on cached SQL values, leading to the test failure.

Implementing the following modifications to trunk@ r22518 should resolve the issue, allowing the test in project_nested_set_concurrency_test.rb to pass. Note that my environment with PostgreSQL 14 and Ruby 2.7.

diff --git a/Gemfile b/Gemfile
index d5870ad95..908c32343 100644
--- a/Gemfile
+++ b/Gemfile
@@ -3,7 +3,6 @@ source 'https://rubygems.org'
 ruby '>= 2.7.0', '< 3.3.0'

 gem 'rails', '7.1.2'
-gem 'rack-session', '~> 1.0'
 gem 'rouge', '~> 4.2.0'
 gem 'mini_mime', '~> 1.1.0'
 gem "actionpack-xml_parser" 
diff --git a/test/unit/project_nested_set_concurrency_test.rb b/test/unit/project_nested_set_concurrency_test.rb
index e835d2b50..077731040 100644
--- a/test/unit/project_nested_set_concurrency_test.rb
+++ b/test/unit/project_nested_set_concurrency_test.rb
@@ -38,7 +38,7 @@ class ProjectNestedSetConcurrencyTest < ActiveSupport::TestCase
     p = generate_project!
     p.destroy

-    assert_difference 'Project.count', 60 do
+    assert_difference 'Project.async_count.value', 60 do
       threads = []
       3.times do |i|
         threads << Thread.new(i) do

Minoru Maeda wrote in #note-29:

Minoru Maeda wrote in #note-27:

I'm not sure of the reason, but it seems to be related to https://github.com/rack/rack-session#compatibility. After modifying the Gemfile as follows and running bundle update, the test for project_nested_set_concurrency_test.rb passed with the seed value that previously led to failure.

I've discovered the reason for the failure of the test in project_nested_set_concurrency_test.rb. The issue appears to be that assert_difference is evaluating Project.count based on cached SQL values, leading to the test failure.

Implementing the following modifications to trunk@ r22518 should resolve the issue, allowing the test in project_nested_set_concurrency_test.rb to pass. Note that my environment with PostgreSQL 14 and Ruby 2.7.

[...]

Thanks, committed!

Go MAEDA wrote in #note-36:

A test is failing after updating Rails to 7.1.3 in r22665.

[...]

This is caused by the following Rails issue. I am waiting for the release of Rails 7.1.4.
Rails 7.1.3: async_count returns a completed Promise instead of the value · Issue #50776 · rails/rails

I've reverted the update to 7.1.3 until Rails 7.1.4 is released in order to have all the tests pass.