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 pymongo.patch for more pymongo versions #449

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

XenoAmess
Copy link

@XenoAmess XenoAmess commented Oct 30, 2024

(tested on 4.3.3, 4.10.1)

reason:

  1. the original patch compares too many lines, thus quite some pymongo versions actually failed the patch examination, thus we reduce it into a one liner. well according to history, this line nearlly always unchanged, and seems no reason for anyone to add another same line in anyway.
  2. since pymongo 4.8 they move things in setup.py to _setup.py, thus the patch shall suite too.

@oracle-contributor-agreement oracle-contributor-agreement bot added the OCA Verified All contributors have signed the Oracle Contributor Agreement. label Oct 30, 2024
Copy link
Contributor

@msimacek msimacek left a comment

Choose a reason for hiding this comment

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

Thank you for the contribution, I'll look into merging it shortly.

@XenoAmess
Copy link
Author

XenoAmess commented Oct 30, 2024

Thank you for the contribution, I'll look into merging it shortly.

I'm still not quite clear for sereral things.

  1. should we split it according to versions? (like I did in the second commit)
  2. is there a version range mechanism in these patch files? like for [4.8,) or something. or the version 4.8 means it be already would cover 4.8+
  3. is there some way to let it work in graalvm+mavenplugin+python-resources dependency toolchains?
    I modified the jar resources and made a local install and changed the dependency version in pom, and sadly to see it failed, but graalpy release can success(by modify the patch file in that folder.)

@XenoAmess
Copy link
Author

I know question 3 can be resolved by manually modify files generated at /target/classes/org.graalvm.python.vfs/home/lib-graalpython/patches, then clean pip-graal wheel caches, then rebuild.
but just think there shall be a...better way.

@XenoAmess
Copy link
Author

XenoAmess commented Oct 30, 2024

for question 2 I tried, but seems 4.8.0 cannot cover 4.10.1. maybe we have to suffer the patch failing error warning(though it can work)
or, if you want. I would write a small version range mechanism for this repo when weekend.

@msimacek
Copy link
Contributor

Yes, splitting the versions would be better. The patches are not applied by the filename, but there's a metadata file with rules that determines what package version should be patched with which patch and the version can be a range. There is a README explaining the metadata format. The existing entry for pymongo doesn't specify a version, so it applies to all versions, but you can see other entries that have version ranges for inspiration. When you're done editing the metadata file, you can run python mx.graalpython/verify_patches.py graalpython/lib-graalpython/patches/ to check its syntax.

As for getting it into existing released jar, unfortunately there is no nice mechanism in the current release without repacking the jar manually. I think it's simpler to build the wheel outside of maven with pip wheel pymongo==4.3.3 and then put the wheel file path as a dependency (into <package> tag). In the next release there will be a way to override the patch directory with an env var and also a way for us to ship patch changes post-release (#429).

@msimacek
Copy link
Contributor

Hi @XenoAmess, do you want to work on splitting the versions or should I merge it as it is?

@XenoAmess
Copy link
Author

Hi @XenoAmess, do you want to work on splitting the versions or should I merge it as it is?

merge it and when weekend I would find some time to start another pr for the version range.
or just keep this pr open and wait for my commit at this weekend.
sorry for having a too busy job. sigh.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
OCA Verified All contributors have signed the Oracle Contributor Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants