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

Fix JDK17 #2634

Merged
merged 2 commits into from Jul 11, 2022
Merged

Fix JDK17 #2634

merged 2 commits into from Jul 11, 2022

Conversation

ferdnyc
Copy link
Contributor

@ferdnyc ferdnyc commented Jul 11, 2022

This PR uses the copy/rename Maven plugin to copy two translation files to their updated names, when building under JDK17+. This allows the tests to pass.

Now that JDK17 builds are successful, run them on all three platforms in CI, same as other versions.

@ferdnyc
Copy link
Contributor Author

ferdnyc commented Jul 11, 2022

bullwinkle-this_time_for_sure

@ferdnyc ferdnyc mentioned this pull request Jul 11, 2022
@ferdnyc
Copy link
Contributor Author

ferdnyc commented Jul 11, 2022

While everything is now passing, to make Windows and macOS more happy it seems like it would be a really good idea to sort out automatic inclusion of the libOSXAccess_10.5.jnilib / aereg64.dll native files into the final packaging.

@ferdnyc
Copy link
Contributor Author

ferdnyc commented Jul 11, 2022

This PR uses the copy/rename Maven plugin to copy two translation files to their updated names, when building under JDK17+. This allows the tests to pass.

Hmmm. Now I'm wondering whether we should always do this, for the packaging. The question is, if the code is compiled under JDK 8/11, but then run under JDK 17, which names will it expect those translation files to have? If it's the legacy names, that's fine. If it's the new names, then this should be an always-step, not a JDK17+-only step.

I guess what we really need is a Hebrew- or Indonesian-language user who can run the app under JDK 17 and let us know whether the interface is properly localized.

@parg
Copy link
Contributor

parg commented Jul 11, 2022

Internationalisation is supported by CrowdIn - https://crowdin.com/project/biglybt

There is already an issue that the Flemish translations end up in a file named "vls_BE" whereas the JDK expects "nl_BE". This is hacked around in

https://github.com/BiglySoftware/BiglyBT/blob/master/core/src/com/biglybt/core/internat/MessageText.java#:~:text=%7D-,private%20static%20final%20Map%3CString%2CLocale%3E%20substitutes%20%3D,%7D,-public%20static%20interface

So rather than copying/renaming files I would prefer a solution that did something similar and would therefore work for multiple JDK versions

@ferdnyc
Copy link
Contributor Author

ferdnyc commented Jul 11, 2022

@parg: This would, at a slight cost of extra space taken up in the JAR, since the two renamed translation files will be present under both names in the final JAR. (It literally just copies the file to the alternate name, leaving the old naming in place.)

There are code changes that can be made under JDK17 to have to continue using the legacy language codes; I sort of hate to do that because it feels backwards, but it's probably the only workable solution that doesn't involve duplicating the data. (The link I posted to the other PR about the language-code changes has info on how to continue using the legacy codes in JDK 17+.)

@parg parg merged commit ef1498d into BiglySoftware:master Jul 11, 2022
@ferdnyc
Copy link
Contributor Author

ferdnyc commented Jul 11, 2022

(Actually, I should check the artifact sizes — depending how smart zip compression is, two files with different names but identical contents might not even increase the archive size by more than a few bytes for the extra dictionary entry.)

Edit: Nope, it's not that smart. Extra 300K for the two duplicated files. (Still, that's out 20MB total, so it's not even a 2% increase.)

@ferdnyc ferdnyc deleted the hebrew-translations branch July 11, 2022 12:11
TuxPaper pushed a commit that referenced this pull request Oct 24, 2022
* JDK17: Copy some translations to new names

* CI: Build JDK17 on all platforms, expect success
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 this pull request may close these issues.

None yet

2 participants