Page MenuHomeSolus

Initial commit of python-orjson
ClosedPublic

Authored by Staudey on Apr 11 2021, 6:16 PM.

Details

Summary

Initial commit of python-orjson

Dependency of anki

Test Plan

Built and ran anki with this version

Diff Detail

Branch
master
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 1435
Build 1435: arc lint + arc unit

Event Timeline

Staudey created this revision.Apr 11 2021, 6:16 PM
Staudey requested review of this revision.Apr 11 2021, 6:16 PM
Staudey updated this revision to Diff 26126.Apr 15 2021, 5:48 PM

Update to 3.5.2

JoshStrobl added inline comments.
package.yml
19

Is there a specific reason it needs to do this?

Staudey marked an inline comment as done.Apr 16 2021, 4:37 AM
Staudey added inline comments.
package.yml
19

It uses the "-Z" option in the build, which is only available in nightly versions. The official build instructions mention that this particular rust nightly is a known good version.

Staudey marked an inline comment as done.Apr 16 2021, 4:51 AM
Staudey added inline comments.
package.yml
19

I can further investigate when I get home if this is a no go. Maybe there is a chance that our Rust is new enough that with a modification of the build scripts it will compile anyway. But then again anki itself also downloads a nightly version during its build.

Staudey added inline comments.Apr 16 2021, 6:32 PM
package.yml
19

So, the orjson build makes use of the "mutable-noalias=yes" option, which is only available in nightly versions of Rust. If we were on LLVM 12 I could simply remove that option since Rust defaults to mutable noalias with LLVM >= 12, but right now the nightly version of Rust is the only way to set it as orjson wants it. Of course, I could simply patch that out and ignore it, since it's just an optimization. Thoughts?

DataDrake requested changes to this revision.Apr 23 2021, 11:11 AM
DataDrake added a subscriber: DataDrake.

@Staudey If patching it out is the difference between using our Rust and installing a nightly, absolutely patch it out.

This revision now requires changes to proceed.Apr 23 2021, 11:11 AM

@DataDrake So I tried to patch it out but as it turns out it doesn't just depend on the nightly toolchain for the minor "mutable-noalias=yes" option, but in a more subtle way by using dependencies that require features of Rust nightly. Patching some of those features out went a little further, but now I have arrived at dependencies of dependencies that use nightly features, and also at a point where I can no longer guarantee that some of those nightly features aren't necessary for the proper function of python-orjson in some regard.

@Staudey Would it make sense to target an older version of python-orjson which matches the nightly which eventually became our current Rust? Just considering options.

Good point. I hadn't considered this. Will take a look when I get home.

So, since I either have to test and patch several very old orjson versions or download the rust nightly toolchain, I have decided to not package this after all. During my research I found out that anki includes a workaround for systems without orjson, and will work fine. pip will still complain about an unmatched dependency, but there should be no user-visible issue (and no issue ever manifested itself in my tests)
As soon as Rust implements some of the required features in stable and our LLVM is updated for the no mutable aliases feature to work, I will take a look at packaging it again.

Staudey abandoned this revision.Apr 28 2021, 6:09 PM

Erm, how do I remove this from the anki stack? ^^

algent added a subscriber: algent.Apr 28 2021, 8:38 PM

Erm, how do I remove this from the anki stack? ^^

"Edit Related Revisions..." > "Edit Parent Revisions"

Staudey reclaimed this revision.Aug 17 2021, 5:06 PM
This revision now requires changes to proceed.Aug 17 2021, 5:06 PM
Staudey updated this revision to Diff 28289.Aug 17 2021, 5:08 PM

Update to 3.6.2 and re-open diff now that it builds with Rust stable

Staudey edited the summary of this revision. (Show Details)Aug 17 2021, 5:08 PM
JoshStrobl accepted this revision.Aug 22 2021, 11:47 AM

LGTM, thanks!

This revision was not accepted when it landed; it landed in state Needs Review.Aug 22 2021, 11:48 AM
This revision was automatically updated to reflect the committed changes.