Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes #36574 - Update fast_gettext to ~> 2.1 #9770

Merged
merged 2 commits into from
Mar 4, 2024

Conversation

ekohl
Copy link
Member

@ekohl ekohl commented Jul 7, 2023

No description provided.

@ekohl ekohl requested a review from a team as a code owner July 7, 2023 14:50
@ekohl ekohl force-pushed the 36574-update-fast_gettext branch from 505d278 to 1252a16 Compare July 7, 2023 15:28
@ekohl
Copy link
Member Author

ekohl commented Jul 7, 2023

I needed to avoid translating during startup because it led to infinite recursion. #9771 is that PR and included here.

@ekohl
Copy link
Member Author

ekohl commented Jul 7, 2023

There's another case here:

apipie :class, desc: "A class representing #{model_name.human} object" do

The human method is here:
https://github.com/grosser/gettext_i18n_rails/blob/master/lib/gettext_i18n_rails/active_model/name.rb

I'll dig a bit deeper.

@ehelms
Copy link
Member

ehelms commented Aug 1, 2023

Glad to see this getting tackled, for both Ruby / Rails reasons and to fix a conflict we've had in packaging for a while now:

Found conflict for rubygem-fast_gettext: 1.8.0 (foreman) != 2.3.0 (hammer-cli)

@ekohl ekohl marked this pull request as draft October 3, 2023 11:57
@ekohl ekohl force-pushed the 36574-update-fast_gettext branch 2 times, most recently from 5eb9e7a to 3c54db7 Compare October 12, 2023 10:09
Copy link
Member

@ofedoren ofedoren left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ekohl, I think I found why we're not compatible with the new version of library: grosser/fast_gettext@v1.8.0...v2.0.0#diff-8798cc5941a4b48c4af929b84b9f804cbaca42547d48dc66471f7e0c4e12d1cf (lib/fast_gettext/translation.rb)

I suggest to update few things: ofedoren@eb42a44

That's not the best or prettiest way, but the logic is kinda in this. Similar changes would be needed for Foreman::Gettext::Debug as well.

lib/foreman/gettext/support.rb Show resolved Hide resolved
@ekohl
Copy link
Member Author

ekohl commented Oct 16, 2023

Thanks. Given the trace of the infinite loop I saw it makes sense. I'll see about giving it a spin. If that works, then I can also drop the 36757 commits from this PR. They'll still be good, but no longer a blocker to this PR.

@ekohl ekohl force-pushed the 36574-update-fast_gettext branch from 3c54db7 to 43ea6a3 Compare October 17, 2023 16:08
# include this module to translate in all domains by default
module Foreman
module Gettext
module AllDomains
class Localizator
prepend FastGettext::TranslationMultidomain
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried to use include here and then it crashes. I also had the impression this file was loaded twice. At least debug appears to be:

/home/ekohl/dev/foreman/lib/foreman/gettext/debug.rb:7: warning: already initialized constant Foreman::Gettext::Debug::DL
/home/ekohl/dev/foreman/lib/foreman/gettext/debug.rb:7: warning: previous definition of DL was here
/home/ekohl/dev/foreman/lib/foreman/gettext/debug.rb:8: warning: already initialized constant Foreman::Gettext::Debug::DR
/home/ekohl/dev/foreman/lib/foreman/gettext/debug.rb:8: warning: previous definition of DR was here

So perhaps that's related to the infinite recursion.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, when I was checking (not debug version), I've noticed that due to changes in the library, we ended up calling our _() method instead of the lib's one due to including stuff wrongly. I don't think double loading has something to do with the recursion. Might be wrong though...

@ekohl ekohl force-pushed the 36574-update-fast_gettext branch from 43ea6a3 to 63e931d Compare November 16, 2023 17:58
@ekohl
Copy link
Member Author

ekohl commented Nov 16, 2023

In theory this is now ready, but I'd like to postpone this until after branching.

@ekohl ekohl force-pushed the 36574-update-fast_gettext branch from 63e931d to c04d773 Compare February 29, 2024 10:28
@ekohl ekohl marked this pull request as ready for review February 29, 2024 10:41
@ekohl
Copy link
Member Author

ekohl commented Feb 29, 2024

We've branched twice. Let's get this in.

@ekohl ekohl requested a review from ofedoren February 29, 2024 10:42
@ofedoren
Copy link
Member

@ekohl, could you please fix rubocop failures? So we can see if the tests are passing at least.

ekohl and others added 2 commits March 1, 2024 12:51
The code defines an additional method on Fastgettext but there's no
reason to do so. It can just as well define a method on itself.
@ekohl ekohl force-pushed the 36574-update-fast_gettext branch from c04d773 to c8f67ae Compare March 1, 2024 11:52
@ekohl
Copy link
Member Author

ekohl commented Mar 1, 2024

Updated, but didn't have time to test this myself. I'll check back what CI says after lunch.

@ofedoren
Copy link
Member

ofedoren commented Mar 1, 2024

Everything seems to be green, except that it cannot be built for some reason :/

@evgeni
Copy link
Member

evgeni commented Mar 1, 2024

Yeah, RPM will fail as it has no 2.1 package yet.

/usr/share/rubygems/rubygems/dependency.rb:313:in `to_specs': Could not find 'fast_gettext' (~> 2.1) - did find: [fast_gettext-1.8.0] (Gem::MissingSpecVersionError)

@evgeni
Copy link
Member

evgeni commented Mar 1, 2024

Let's see: theforeman/foreman-packaging#10515

Copy link
Member

@evgeni evgeni left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Packaging ACK

@evgeni
Copy link
Member

evgeni commented Mar 4, 2024

@ofedoren can I get an ACK on the code? I have not the slightest idea about gettext :/

Copy link
Member

@ofedoren ofedoren left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have not the slightest idea about gettext :/

Me neither :/ But looks correct, tests are green, the app is booting and the translations are working, so I'd say GTG. Haven't tested on Ruby 2.7 though.

@evgeni evgeni merged commit bbca473 into theforeman:develop Mar 4, 2024
36 of 40 checks passed
@ekohl ekohl deleted the 36574-update-fast_gettext branch March 4, 2024 11:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

5 participants