Fix `IllegalArgumentException` with non-ascii paths #1195

tachyonicClock · 2024-06-16T23:21:21Z

codecov · 2024-06-16T23:25:49Z

Codecov Report

All modified and coverable lines are covered by tests ✅

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%)`	⬆️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Thrameos

LTGM

Thrameos · 2024-06-16T23:49:10Z

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

tachyonicClock · 2024-06-17T00:56:48Z

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

tachyonicClock · 2024-06-17T02:24:11Z

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

Thrameos · 2024-06-17T03:11:36Z

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.

tachyonicClock · 2024-06-17T04:04:17Z

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.

test/jpypetest/test_non_ascii_paths.py

marscher · 2024-08-22T10:48:18Z

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

marscher · 2024-08-27T12:30:04Z

@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!

tachyonicClock · 2024-08-28T00:48:27Z

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 · 2024-09-15T21:01:18Z

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.

marscher · 2024-09-29T11:31:31Z

@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.

tachyonicClock · 2024-09-30T03:28:01Z

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.

native/java/org/jpype/pkg/JPypePackageManager.java

+      // by re-encoding the URI after decoding it.
+      uri = new URI(
+        uri.getScheme(),
+        URLDecoder.decode(uri.getSchemeSpecificPart()),


Thrameos · 2024-10-19T00:38:52Z

jpype/_core.py

+    """Prior versions of JPype used the jvmargs to setup the class paths via 
+    JNI (Java Native Interface) option strings:
+    i.e -Djava.class.path=... 
+    See: https://docs.oracle.com/javase/7/docs/technotes/guides/jni/spec/invocation.html
+
+    Unfortunately, unicode is unsupported by this interface on windows, since
+    windows uses wide-byte (16bit) character encoding.
+    See: https://stackoverflow.com/questions/20052455/jni-start-jvm-with-unicode-support
+
+    To resolve this issue we add the classpath after initialization since jpype
+    itself supports unicode class paths.
+    """
+    for cp in _handleClassPath(classpath):
+        addClassPath(Path.cwd() / Path(cp).resolve())
+


It appears this code is going to cause some serious issues because certain types of classpath absolutely must be loaded prior to starting the JVM. The two that come to mind are database drivers and anything containing a classloader.

I believe there is a solution in that we just need to send a bytes with the correct encoding, but I need to test it.

I'm sorry this ended up breaking things.

I believe there is a solution in that we just need to send a bytes with the correct encoding, but I need to test it.

I wish you luck. I spent some time struggling to get this to work, but did not try exhaustively since I found the stack-overflow response suggesting I set the properties at runtime. I found some discussion of this problem from 2017 but it does not look like it went anywhere: https://mail.openjdk.org/pipermail/jdk10-dev/2017-May/000191.html.

tachyonicClock force-pushed the bug_1194 branch from 83a2105 to aed7745 Compare June 16, 2024 23:22

tachyonicClock changed the title ~~Fix with non-ascii paths~~ Fix IllegalArgumentException with non-ascii paths Jun 16, 2024

Thrameos approved these changes Jun 16, 2024

View reviewed changes

tachyonicClock force-pushed the bug_1194 branch from ae5cd1a to f4b6d88 Compare June 17, 2024 03:55

Thrameos requested a review from marscher June 19, 2024 15:04

marscher reviewed Aug 20, 2024

View reviewed changes

test/jpypetest/test_non_ascii_paths.py Outdated Show resolved Hide resolved

tachyonicClock force-pushed the bug_1194 branch from f4b6d88 to af03e98 Compare August 20, 2024 23:50

marscher self-requested a review August 20, 2024 23:58

marscher approved these changes Aug 20, 2024

View reviewed changes

tachyonicClock force-pushed the bug_1194 branch 4 times, most recently from 9463137 to 693c6f5 Compare August 28, 2024 00:39

tachyonicClock force-pushed the bug_1194 branch 5 times, most recently from a242500 to dea04b7 Compare September 30, 2024 02:52

Fix exception caused by non-ascii paths

e2e60a5

tachyonicClock force-pushed the bug_1194 branch from dea04b7 to e2e60a5 Compare September 30, 2024 03:13

Merge branch 'master' into bug_1194

67940bf

marscher merged commit 354968c into jpype-project:master Oct 2, 2024
4 of 6 checks passed

github-advanced-security bot found potential problems Oct 2, 2024

View reviewed changes

astrelsky mentioned this pull request Oct 11, 2024

#1195 broke use of -Djava.system.class.loader #1222

Open

Thrameos reviewed Oct 19, 2024

View reviewed changes

Thrameos added a commit to Thrameos/jpype that referenced this pull request Oct 19, 2024

Backing out jpype-project#1195 and working on replacement

9d66c27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix `IllegalArgumentException` with non-ascii paths #1195

Fix `IllegalArgumentException` with non-ascii paths #1195

tachyonicClock commented Jun 16, 2024

codecov bot commented Jun 16, 2024 •

edited

Loading

Thrameos left a comment

Thrameos commented Jun 16, 2024

tachyonicClock commented Jun 17, 2024

tachyonicClock commented Jun 17, 2024

Thrameos commented Jun 17, 2024

tachyonicClock commented Jun 17, 2024

marscher commented Aug 22, 2024

marscher commented Aug 27, 2024

tachyonicClock commented Aug 28, 2024

tachyonicClock commented Sep 15, 2024 •

edited

Loading

marscher commented Sep 29, 2024

tachyonicClock commented Sep 30, 2024 •

edited

Loading

Thrameos Oct 19, 2024

tachyonicClock Oct 21, 2024

Fix IllegalArgumentException with non-ascii paths #1195

Fix IllegalArgumentException with non-ascii paths #1195

Conversation

tachyonicClock commented Jun 16, 2024

codecov bot commented Jun 16, 2024 • edited Loading

Codecov Report

Thrameos left a comment

Choose a reason for hiding this comment

Thrameos commented Jun 16, 2024

tachyonicClock commented Jun 17, 2024

tachyonicClock commented Jun 17, 2024

Thrameos commented Jun 17, 2024

tachyonicClock commented Jun 17, 2024

marscher commented Aug 22, 2024

marscher commented Aug 27, 2024

tachyonicClock commented Aug 28, 2024

tachyonicClock commented Sep 15, 2024 • edited Loading

marscher commented Sep 29, 2024

tachyonicClock commented Sep 30, 2024 • edited Loading

Thrameos Oct 19, 2024

Choose a reason for hiding this comment

tachyonicClock Oct 21, 2024

Choose a reason for hiding this comment

Fix `IllegalArgumentException` with non-ascii paths #1195

Fix `IllegalArgumentException` with non-ascii paths #1195

codecov bot commented Jun 16, 2024 •

edited

Loading

tachyonicClock commented Sep 15, 2024 •

edited

Loading

tachyonicClock commented Sep 30, 2024 •

edited

Loading