Skip to content

[WIP] Startup: restore locale after importing core #551

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

Draft
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

HuidaeCho
Copy link
Member

@HuidaeCho HuidaeCho commented Apr 25, 2020

from grass.script import core as gcore installs a new _() function? Better solve this problem in the core module, IMO.

The address of the _() function changes after importing the core module. The setup module is OK...

See below. The exiting messages are all translated into Korean, but they are printed in English because a new _() is installed with no language information.

image

This PR fixes it.
image

@HuidaeCho HuidaeCho added the bug Something isn't working label Apr 25, 2020
@HuidaeCho HuidaeCho changed the title Startup: restore locale from importing core Startup: restore locale after importing core Apr 25, 2020
@HuidaeCho HuidaeCho requested a review from wenzeslaus April 25, 2020 20:56
@HuidaeCho
Copy link
Member Author

The same problem in clean_default_db() in lib/python/script/setup.py. Cleaning up default sqlite database ... is not translated.

@HuidaeCho HuidaeCho changed the title Startup: restore locale after importing core [WIP] Startup: restore locale after importing core Apr 26, 2020
@HuidaeCho
Copy link
Member Author

HuidaeCho commented Apr 26, 2020

Again, IMO, it needs to be fixed in the core module because importing it causes the problem. Importing the setup module is OK. But internally in the setup module, _() is not translated.

@HuidaeCho
Copy link
Member Author

@HuidaeCho
Copy link
Member Author

and maybe this ticket from 6 years ago: https://trac.osgeo.org/grass/ticket/1739

@neteler
Copy link
Member

neteler commented Apr 29, 2020

@pmav99 do you have any comment for us on this PR?

Copy link
Member

@wenzeslaus wenzeslaus left a comment

Choose a reason for hiding this comment

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

The half-translated strings definitively sounds a lot like what was observed at times of Trac 1739 you are mentioning, although from what you are saying it seems like only startup procedure is an issue. I can't recall all the details of what I went through at that time (all the dead ends for example), but it seem you got good understanding of what is happening in the code right now.

As for this PR, it seems your current solution is hacky, but you probably know that already. I did not dive into this for now, so I can't comment on what it does.

I have the same opinion when I did ee95f55 and 1423e79 (now reverted by bb779c2) and opened Trac 2425: The grass package should use an import, not a buildin, for the translation function. (See details in the tickets.) I'm not sure if that alone would solve the possible initialization problem here, probably not, but the fix should be at least more straightforward without workarounds.

@wenzeslaus
Copy link
Member

Another note about a broader context of the suggested changes: In an ideal case, we will move things from grass.py to the grass package. This may increase the number of places where this fix is needed if the change is gradual and may change the nature of the problem if grass.py will really just call some main function for the library. See grass-dev: move everything from /lib/init/grass.py to /lib/python/init (nabble) which should be now easier also thanks to, e.g., 2246f5b.

@HuidaeCho
Copy link
Member Author

it seem you got good understanding of what is happening in the code right now.

I have some understanding of what is happening here. I see two different problems: 1. importing the core module changes the _() function (I have no idea why). 2. imported modules (such as setup) don't have access to the _() function previously installed by its parent automatically, so it needs to be installed again. What is not clear to me is how gettext is supposed/designed to be used in these cases.

As for this PR, it seems your current solution is hacky, but you probably know that already. I did not dive into this for now, so I can't comment on what it does.

I'm aware that this PR is hacky and believe there should be a better and cleaner solution.

@wenzeslaus
Copy link
Member

What is not clear to me is how gettext is supposed/designed to be used in these cases.

You can read through Trac 1739, Trac 2425, and Trac 3790. My comments there capture my understanding at the time of writing which is possibly better than the one I have now. One "dead end" I vaguely remember which may not be mentioned anywhere is that installing the translation function multiple times is against what the gettext manual says. (I'm sorry for not being more specific. Except for the discussion last year, I worked on this at a sprint in 2013.)

@HuidaeCho
Copy link
Member Author

HuidaeCho commented Dec 30, 2020

For our records, this problem became worse. Even the welcome message is partially translated. Actually, it's only the first line because _() is reset in line 1191.

image

def can_start_in_gisrc_mapset(gisrc, ignore_lock=False):
    """Check if a mapset from a gisrc file is usable for a new session"""
    from grass.grassdb.checks import can_start_in_mapset

Again, it can be fixed using the same hack:

def can_start_in_gisrc_mapset(gisrc, ignore_lock=False):
    """Check if a mapset from a gisrc file is usable for a new session"""
    global _
    __ = _
    from grass.grassdb.checks import can_start_in_mapset
    _ = __

image

But, it would be better to fix this issue more fundamentally.

@neteler neteler added this to the 8.0.1 milestone Dec 9, 2021
@ninsbl
Copy link
Member

ninsbl commented Feb 20, 2022

Is this stil in the scope for the 8.0.1 milestone? Or should it be bumped up to 8.0.2?

@wenzeslaus wenzeslaus modified the milestones: 8.0.1, 8.0.2, 8.4.0 Feb 23, 2022
@nilason nilason added the Python Related code is in Python label Feb 15, 2023
@neteler
Copy link
Member

neteler commented Feb 18, 2023

This PR needs to be rebased (and bump to 8.4.0 milestone?).

@wenzeslaus wenzeslaus modified the milestones: 8.3.0, 8.4.0 Mar 17, 2023
@wenzeslaus wenzeslaus modified the milestones: 8.4.0, Future Apr 27, 2024
@echoix echoix added the conflicts/needs rebase Rebase to or merge with the latest base branch is needed label Oct 16, 2024
@echoix echoix marked this pull request as draft October 16, 2024 23:17
@echoix
Copy link
Member

echoix commented Oct 17, 2024

Solved conflicts. Make sure the changes still make sense (I ported the changes from the deleted setup.py file to the current setup.py)

@echoix echoix removed the conflicts/needs rebase Rebase to or merge with the latest base branch is needed label Oct 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working libraries Python Related code is in Python
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants