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

Releases/1.5.1 #1234

Merged
merged 45 commits into from
Nov 18, 2024
Merged

Releases/1.5.1 #1234

merged 45 commits into from
Nov 18, 2024

Conversation

Thrameos
Copy link
Contributor

Still working on a release

Copy link

codecov bot commented Nov 16, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.17%. Comparing base (8e0abe3) to head (98d7acf).
Report is 41 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1234      +/-   ##
==========================================
- Coverage   87.18%   87.17%   -0.01%     
==========================================
  Files         113      113              
  Lines       10287    10296       +9     
  Branches     4045     4051       +6     
==========================================
+ Hits         8969     8976       +7     
- Misses        726      727       +1     
- Partials      592      593       +1     

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


🚨 Try these New Features:

@Thrameos
Copy link
Contributor Author

@marscher I really don't know how to proceed. I made some progress but it took many hours and it still isn't done and I have to get the release out.

The issues appear mostly linked to poetry which seems to have no compatibility with earlier behaviors. The same pipeline we used last release doesn't work because they are forcing changes which break our scripts. We aren't the first to complain that they took a PEP and deciding it was mandatory for everyone to follow it. I have tried forcing the azure to publish the bad wheels so I can try to triage the source. I know the jar is in the sdist, but somehow is missing from the wheel. I will try next to merge in pelson patch to see if it addresses it.

I know it may seem like a good idea to modernize, but this tool is not a replacement in that it has very limited capabilities compared to setup.py and tons of downsides. Locally forcing builds through pip is a huge drag when I want to debug windows issues mean it activity impedes progress.

I have had no luck getting an arm target in azure so I am going to give up on that one.
I also need some way to get Python 3.13 which still is not on Azure.

@marscher
Copy link
Member

Sorry to hear that is is so troublesome. Since when did we use poetry to build wheels? I thought pip wheel would be used.

@marscher
Copy link
Member

marscher commented Nov 17, 2024

setuptools is missing in the windows job (stage install-test)

@Thrameos
Copy link
Contributor Author

I am not sure why we would ever run so long!? Our builds should finish in 5 of less.

For for poetry isn't that the name of the pyproject.toml build system. The issues that I am tracking all matched their gitbuh issue list. Maybe I am confused between the build systems using that tool.

@Thrameos
Copy link
Contributor Author

Oh I see. It is total time. Well that is a problem. I guess the only choice will be to split into release1, release2, release3. We could cut down on the tests I suppose. Perhaps the gc fix will give us a speed boost on the windows system.

@glatterf42
Copy link

Thanks for your efforts on this new release :)
From my side at least, take all the time you need, no stress.
You are probably aware of it already, but if poetry is causing issues, you may want to consider uv instead, which also offers significant speed-ups compared to both poetry and pip.

@Thrameos
Copy link
Contributor Author

Thrameos commented Nov 18, 2024

Nice work @marscher I was having a hard time getting this over the finish line. Too many random errors (scripts breaking on case changes, new exceptions that don't reproduce locally, stuff that I have no familiarity with). It looks like you got through the remaining work.

Are we going to merge the memory fix into this branch?

So what is left to do or should we copy the artifacts to release? Is there any way to prebuild custom docker images with all the python tools we need in another pipeline and just get the artifacts?

It does look like we should just split the release script into windows, osx, linux if for nothing but making debugging faster. I can do that post release.

Also a side note on future features. For work I need to complete some bridge bindings to javafx and javascript. I may also try bridging to golang via the jvm copy. I think they will just be a bunch of customizers but as they are optional should I keep them as submodules jpype.javafx, jpype.javascript, jpype.wasm, jpype.golang etc? Any thoughts on organization?

@marscher
Copy link
Member

marscher commented Nov 18, 2024

Thanks! You actually did most of the (great) job.

Are we going to merge the memory fix into this branch?

we can do that.

It does look like we should just split the release script into windows, osx, linux if for nothing but making debugging faster. I can do that post release.

I think it'd be sufficient to use comments and the conditions defined already in the matrix. Splitting everything up individually sounds appealing, but then we would have to collect all build artifacts manually (every release).

Also a side note on future features.

Please start a new issue or discussion on that one ;-) But sounds promising!

commit d837337
Merge: 1a56a3b c474ee8
Author: Martin K. Scherer <marscher@users.noreply.github.com>
Date:   Mon Nov 18 08:34:35 2024 +0100

    Merge pull request #1235 from Thrameos/win_gc

    Fix for GC on windows

commit c474ee8
Author: Karl Nelson <nelson85@llnl.gov>
Date:   Sun Nov 17 20:25:19 2024 -0800

    Fix tab/spaces

commit 3d2577a
Author: Karl Nelson <nelson85@llnl.gov>
Date:   Sat Nov 16 18:15:44 2024 -0800

    Fix for GC on windows
@Thrameos
Copy link
Contributor Author

I put the idea in discussions under #1237. Please comment on whether it is in scope.

@marscher
Copy link
Member

now we are suffering from the readme rendering behaviour on PyPI... pypa/readme_renderer#304

Checking jpype1-1.5.1-cp39-cp39-manylinux_2_17_x86_64.manylinux2014_x86_64.whl: FAILED
ERROR    `long_description` has syntax errors in markup and would not be rendered on PyPI.                                                                                                        
         line 1: Warning: Cannot scale image!                                                                                                                                                     
           Could not get size from "doc/logo.png":                                                                                                                                                
           Reading external files disabled.

It seems the only workaround is to remove the scale argument for now.

@Thrameos
Copy link
Contributor Author

Wow. When it rains, it pours. Seems like we have hit every possible road bump on our way.

@marscher
Copy link
Member

Indeed: Python-3.13-nogil

native/common/jp_tracer.cpp:153:51: error: ‘PyObject’ {aka ‘struct _object’} has no member named ‘ob_refcnt’
153 | str << msg << " " << (void*) ref << " " << ref->ob_refcnt << " " << Py_TYPE(ref)->tp_name;
| ^~~~~~~~~

We need to guard this as in a previous PR

Comment on lines +566 to +569
// sending -1 will cause issues on Windows
if (lineNum<0)
lineNum = 0;

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@marscher this was a hidden two line fix. The problem it is addressing is that pytest is trying to do arithmetic. On linux systems we get -1 they subtract one then add one. No problem.

On windows system with the same version when we pass -1 to an integer field, we get None in the field. -2 works, 0 works, but -1 gives None. Then Pytest fails.

I think that we can fix this back in Java where we pass a line number 0 rather than -1. But it was easier to patch at this level in case there was some other way that we get invalid line numbers.

This seems like a typical left hand/right hand. Making -1 which was supposed to mean "line number not available" as None is completely reasonable. Trying to check the line numbers in the file (which happens to be 0 counting) for the audit display is perfectly reasonable in pytest. But both changes can't live together.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why the heck is this platform dependent? Oo

@marscher
Copy link
Member

marscher commented Nov 18, 2024

After hitting every possible road bump, we are finally there. The release is on PyPI. Thank you all very much.

@marscher marscher merged commit 7b1abed into master Nov 18, 2024
43 of 44 checks passed
.azure/scripts/wheels.yml Show resolved Hide resolved
.azure/scripts/wheels.yml Show resolved Hide resolved
.azure/scripts/wheels.yml Show resolved Hide resolved
@petrushy
Copy link

Don't know where to write this, but many thanks for this release and for the work on Jpype!

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.

6 participants