jpype-project / jpype

JPype is cross language bridge to allow Python programs full access to Java class libraries.
http://www.jpype.org
Apache License 2.0
1.12k stars 183 forks source link

Fix `IllegalArgumentException` with non-ascii paths #1195

Closed tachyonicClock closed 1 month ago

tachyonicClock commented 5 months ago

Fix #1194

codecov[bot] commented 5 months ago

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 ```diff @@ 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](https://app.codecov.io/gh/jpype-project/jpype/pull/1195/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=jpype-project) | Coverage Δ | | |---|---|---| | [](https://app.codecov.io/gh/jpype-project/jpype/pull/1195/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=jpype-project) | `87.21% <100.00%> (+<0.01%)` | :arrow_up: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=jpype-project#carryforward-flags-in-the-pull-request-comment) to find out more.

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

Thrameos commented 5 months ago

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

tachyonicClock commented 5 months ago

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

tachyonicClock commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

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.

marscher commented 3 months ago

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

marscher commented 2 months ago

@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 commented 2 months ago

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 commented 2 months ago

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 commented 1 month ago

@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 commented 1 month ago