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

Internationalization change broke macOS build #1154

Closed
a2k-hanlon opened this issue Jan 29, 2021 · 11 comments · Fixed by #1153
Closed

Internationalization change broke macOS build #1154

a2k-hanlon opened this issue Jan 29, 2021 · 11 comments · Fixed by #1153

Comments

@a2k-hanlon
Copy link
Collaborator

While tinkering with the macOS build, I found a change that broke the build; the packaged app crashes on startup. If I revert the change, the app does start. The change is commit 95e6830. I don't yet know exactly what part of this change is causing the problem. I made a branch on my fork with that commit reverted here that produces a macOS app that runs (through github actions). Just to give the full picture, this branch also includes the changes in #1153.

@a2k-hanlon
Copy link
Collaborator Author

a2k-hanlon commented Jan 29, 2021

This is the code in question. With this code, the packaged macOS app crashes at startup:

def install_locale(domain):
shared_locale_dir = os.path.join(DATADIR, 'locale')
translation = None
lang = locale.getdefaultlocale()
if os.path.exists(shared_locale_dir):
translation = gettext.translation(domain, shared_locale_dir, languages=[lang[0]], fallback= True)
else:
translation = gettext.translation(domain, './locale', languages=[lang[0]], fallback= True)
translation.install()

and here is the same function from before the change. With this code, the packaged macOS app works:

def install_locale(domain):
shared_locale_dir = os.path.join(DATADIR, 'locale')
if os.path.exists(shared_locale_dir):
gettext.install(domain, shared_locale_dir)
else:
gettext.install(domain, './locale')

@DivingDuck
Copy link
Collaborator

Oh, this will break at least the windows version.

Can you please check if this will work for macOS:

def install_locale(domain):
translation = None
lang = locale.getdefaultlocale()
if os.path.exists('/usr/share/pronterface/locale'):
translation = gettext.translation(domain, '/usr/share/pronterface/locale', languages=[lang[0]], fallback= True)
elif os.path.exists('/usr/local/share/pronterface/locale'):
translation = gettext.translation(domain, '/usr/local/share/pronterface/locale', languages=[lang[0]], fallback= True)
else:
translation = gettext.translation(domain, './locale', languages=[lang[0]], fallback= True)
translation.install()

@a2k-hanlon
Copy link
Collaborator Author

Ok, I tried that, packaging the app locally on macOS. The app still crashes.

@DivingDuck
Copy link
Collaborator

Well, this is bad. Can you tell me what exactly cause this crash? I have no macOS and can't debug this myself.

@kliment, I can do a improvised workaround for macOS that needs to be tested once more, it simply combine both methods depending if the system is macOS or not:

def install_locale(domain):
    shared_locale_dir = os.path.join(DATADIR, 'locale')
    translation = None
    lang = locale.getdefaultlocale()
    plat = platform.system()
    if plat == "Darwin": #workaround for macOS crash with gettext.translation
        #do darwin
        if os.path.exists(shared_locale_dir): 
            gettext.install(domain, shared_locale_dir) 
        else: 
            gettext.install(domain, './locale') 
    else:
        if os.path.exists('./locale'):
            translation = gettext.translation(domain, './locale', languages=[lang[0]], fallback= True)
        else:
            translation = gettext.translation(domain, shared_locale_dir, languages=[lang[0]], fallback= True)
        translation.install()

Over all I would feel me much more comfortable, if we find out, what exactly cause the crash, but this will at least prevent a showstopper for 2.0.0.

@kliment
Copy link
Owner

kliment commented Jan 30, 2021

Workaround is fine. I don't have a device I can debug this on either but I strongly suspect it's because of trying to access locations that macos considers part of the system, and protected.

@DivingDuck
Copy link
Collaborator

@a2k-hanlon, can you please try the workaround? You need in addition to the modification above an additional line with 'import platform' below 'import os'. Check if plat = platform.system() reports 'Drawin' or maybe something else that need to be used.

I will then push an update if this works for you.

@a2k-hanlon
Copy link
Collaborator Author

Ok, with that change it works, no crash. I'm not sure what I can do to debug a built app, but I can try to find out. I can also debug running source of course. I'll let you know if I have a chance to do so and I find something.

plat = platform.system() does indeed report 'Darwin'.

@a2k-hanlon
Copy link
Collaborator Author

@DivingDuck if you like, I can commit your last suggested change onto #1153. The change certainly fits in with the theme of that PR. However, I respect you said you would push an update and I don't mean to ignore that! I'm just asking in case I can save you some trouble :)

@DivingDuck
Copy link
Collaborator

Good to hear, I will push an update for this workaround.
I guess the problem have to do with what kliment mentioned. Anyway I combine both and hope it will work for some time.
(... up to the moment a new macOS update will see the world, LOL)

Go ahead, I'm ok, if you want to push the change. I made two little changes on it to remember me that this need a further inspection some when in the future. One is the comment and the other is a renaming from plat to osPlatform. Would be nice to change this in your request too.

def install_locale(domain):
    shared_locale_dir = os.path.join(DATADIR, 'locale')
    translation = None
    lang = locale.getdefaultlocale()
    osPlatform = platform.system()

    if osPlatform == "Darwin":
        # improvised workaround for macOS crash with gettext.translation, see issue #1154
        if os.path.exists(shared_locale_dir): 
            gettext.install(domain, shared_locale_dir) 
        else: 
            gettext.install(domain, './locale') 
    else:
        if os.path.exists('./locale'):
            translation = gettext.translation(domain, './locale', languages=[lang[0]], fallback= True)
        else:
            translation = gettext.translation(domain, shared_locale_dir, languages=[lang[0]], fallback= True)
        translation.install()

@kliment, I'm ok with the change.

Best regards,
DD

@kliment
Copy link
Owner

kliment commented Jan 31, 2021

Yes, @a2k-hanlon please add @DivingDuck's suggestion to #1153

a2k-hanlon added a commit to a2k-hanlon/Printrun that referenced this issue Jan 31, 2021
@a2k-hanlon
Copy link
Collaborator Author

Done. Thank you @DivingDuck and @kliment for your help with this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants