jpype icon indicating copy to clipboard operation
jpype copied to clipboard

Fix `IllegalArgumentException` with non-ascii paths

Open tachyonicClock opened this issue 1 year ago • 9 comments

Fix #1194

tachyonicClock avatar Jun 16 '24 23:06 tachyonicClock

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 87.21%. Comparing base (09f325f) to head (e2e60a5).

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #1195   +/-   ##
=======================================
  Coverage   87.21%   87.21%           
=======================================
  Files         113      113           
  Lines       10277    10280    +3     
  Branches     4088     4088           
=======================================
+ Hits         8963     8966    +3     
  Misses        718      718           
  Partials      596      596           
Flag Coverage Δ
87.21% <100.00%> (+<0.01%) :arrow_up:

Flags with carried forward coverage won't be shown. Click here to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov[bot] avatar Jun 16 '24 23:06 codecov[bot]

Looks like I have some maintenance tasks before the CI will run.

Thrameos avatar Jun 16 '24 23:06 Thrameos

@Thrameos I'm working on a test and it seems emoji still don't work although that appears to be a different reason.

tachyonicClock avatar Jun 17 '24 00:06 tachyonicClock

The emoji thing seems to be a problem with java itself

 java -classpath "😊/mypackage.jar" mypackage.MyClass
Error: A JNI error has occurred, please check your installation and try again
Exception in thread "main" java.lang.IllegalArgumentException: Error decoding percent encoded characters
        at java.base/sun.net.www.ParseUtil.decode(ParseUtil.java:218)
        at java.base/jdk.internal.loader.FileURLMapper.getPath(FileURLMapper.java:64)
        ...

https://youtrack.jetbrains.com/issue/IJPL-19330/Scratch-Java-file-wont-run-if-theres-an-emoji-in-the-project-name

tachyonicClock avatar Jun 17 '24 02:06 tachyonicClock

I am hoping it is safe to ignore emoji. Although a coworker of mine had great fun using the poop emoji when we added internationalization to string conversions, I think that it is an edge case.

It is also a good idea to add a line to "doc/CHANGELOG.rst" describing the change so that others will know it is fixes. Thanks for taking the time to add tests.

Thrameos avatar Jun 17 '24 03:06 Thrameos

Since Java appears to have difficulties with emojis in classpaths, it's probably impossible to fix this issue. The important thing is to support non-English character sets that are likely in user paths. That is the problem that prompted me to open this issue in the first place. Although a poop emoji directory does sound like a good place to store certain parts of java :D.

I've added my changes to the changelog. Let me know if its not in the right place or the wording is bad.

tachyonicClock avatar Jun 17 '24 04:06 tachyonicClock

Honestly I do not understand right now, why the pipeline isnt converging. On the main branch everything was fine.

marscher avatar Aug 22 '24 10:08 marscher

@tachyonicClock I think the hung tests are due to the process creation within the new regression test. Could you please check, if using https://pypi.org/project/pytest-forked/ and marking the test with the provided marker could be a solution?

@pytest.mark.forked
def test_with_leaky_state():
    run_some_monkey_patches()

This would require of course to install pytest-forked for the test suite. It should go to test-requirements.txt. Thank you!

marscher avatar Aug 27 '24 12:08 marscher

I fixed the hanging issue and packaged it with the related tests in test_startup.py. These are run as sub-processes using `subrun'. I'm running into an issue with them failing on Windows. I'll try to fix this later this week.

tachyonicClock avatar Aug 28 '24 00:08 tachyonicClock

Last week I tried fixing this on windows but ran into some other issues related to UTF-8 vs UTF-16 character encoding on Windows in the JNI. It will likely take me a little longer to figure out a solution or workaround.

tachyonicClock avatar Sep 15 '24 21:09 tachyonicClock

@tachyonicClock Thanks for your work on this so far! Any updates on this one? We would like to conclude a set of changes for the next release and I would like to include this fix if it also works on Windows.

marscher avatar Sep 29 '24 11:09 marscher

  • Launching Java with non-ascii class paths appears impossible on Windows (https://stackoverflow.com/questions/20052455/jni-start-jvm-with-unicode-support). To resolve this issue, I tried adding the classpath after initialization since this supports Unicode. But the change to how jpype starts up is significant.
  • I fixed a problem in Python 3.8 with JDK8 todo with "zip file system provider returns doubly % encoded URIs" (native/java/org/jpype/pkg/JPypePackageManager.java). A pre-existing fix removed % signs if '%2520' was present. This did not fix doubly encoded Unicode characters and risked removing % signs that were intentional. The new version will work in these cases.
  • Emoji now work.
  • I added project/jars/unicode_à😎 to describe how to build the Unicode jar test case. I tried to follow the pattern of other examples, but I'm not confident I did this correctly.

tachyonicClock avatar Sep 30 '24 03:09 tachyonicClock