Generate sdist by hand #2811

tomjakubowski · 2024-10-28T20:57:56Z

This updates the perspective-python build to create an sdist tarball by
hand, not going through maturin sdist. This bypasses some issues we
have seen with Maturin mispackaging the perspective Cargo workspace in
the sdist.

Maturin is still used as the build-backend to build a wheel from the
sdist. CI still uses maturin build to build wheels. Local
development builds still use maturin develop.

The sdist now includes the .data directory so that Maturin will place it
in the wheel correctly. Building an sdist with PSP_BUILD_SDIST=1 will
now error if it appears the jupyterlab plugin was not built into the
.data directory.

Most of the work is in generating a PKG-INFO file. Its output matches
what's in the 3.1.2 sdist on pypi, minus a correction to fix the
Home-page field miscapitalized by Maturin and some unimportant
whitespace changes in Require-Dist.

Pull Request Checklist

Description which clearly states what problems the PR solves.
Description contains a link to the Github Issue, and any relevent
Discussions, this PR applies to. (N/A)
Include new tests that fail without this PR but passes with it.
Include any relevent Documentation changes related to this change.
Verify all commits have been signed in accordance with the DCO policy.
Reviewed PR commit history to remove unnecessary changes.
Make sure your PR passes build, test and lint steps completely.

tomjakubowski · 2024-10-28T22:16:36Z

test_js failure looks familiar, I think it's a known flaky test

In the sdist artifact, looks like the labextension is packaged as expected (just the one copy):

~/Downloads › tar tzvf perspective_python-3.1.2.tar.gz | grep labextension
-rw-r--r--  0 1001   127       203 Oct 28 14:34 perspective_python-3.1.2/perspective/labextension/install.json
-rw-r--r--  0 1001   127      2513 Oct 28 14:34 perspective_python-3.1.2/perspective/labextension/package.json
-rw-r--r--  0 1001   127   5073815 Oct 28 14:34 perspective_python-3.1.2/perspective/labextension/static/253.33663433de87e488ba09.js
-rw-r--r--  0 1001   127      2452 Oct 28 14:34 perspective_python-3.1.2/perspective/labextension/static/253.33663433de87e488ba09.js.LICENSE.txt
-rw-r--r--  0 1001   127    105550 Oct 28 14:34 perspective_python-3.1.2/perspective/labextension/static/295.b558c522e11647489867.js
-rw-r--r--  0 1001   127      8860 Oct 28 14:34 perspective_python-3.1.2/perspective/labextension/static/remoteEntry.1b5e905e1f26c06be4dd.js
-rw-r--r--  0 1001   127       193 Oct 28 14:34 perspective_python-3.1.2/perspective/labextension/static/style.js
-rw-r--r--  0 1001   127      2453 Oct 28 14:34 perspective_python-3.1.2/perspective/labextension/static/third-party-licenses.json
~/Downloads › tar tzvf perspective_python-3.1.2.tar.gz | grep remoteEntry
-rw-r--r--  0 1001   127      8860 Oct 28 14:34 perspective_python-3.1.2/perspective/labextension/static/remoteEntry.1b5e905e1f26c06be4dd.js

And the wheel artifact still has the one copy of the labextension, in the .data directory, as expected

tomjakubowski · 2024-10-29T23:55:49Z

rust/perspective-python/Cargo.toml

@@ -52,9 +52,12 @@ pyo3-build-config = "0.20.2"
 python-config-rs = "0.1.2"

 [dependencies]
+# NOTE: when building from the git repo, these perspective-* dependencies are
+# overridden with path dependencies in .cargo/config.toml.  This is done to
+# support the sdist, which doesn't include these packages.


there may be pitfalls in doing it this way I'm not aware of, but it seemed less messy than patching the Cargo.toml file in the tarball.

Since the new test_python_sdist job in CI runs in a git checkout, the path directives in .cargo/config.toml will apply when that job builds the perspective-python crate (and wheel) from the sdist. That means in test_python_sdist, changes in the git repo to perspective-client and perspective-server which haven't yet been published to crates.io would be tested too. That's what we want, I think: CI tests coherent versions of perspective-client/perspective-server/perspective-python all from the same git commit, which previews how the release sdist will behave.

End users building from that same sdist artifact, if it's from between releases, would get a different result, instead building against the most recent release of the perspective-server and -client crates on crates.io. Workarounds for them would be to set up a .cargo/config.toml (maybe one in ~? I don't know if pip's build isolation would defeat that) which uses paths pointing into a local perspective git clone. This is a (minor) downside of bundling only perspective-python's source in the sdist

Oh, uh, I stand corrected. The sdist build in CI is downloading perspective-client and -server from crates.io https://github.com/tomjakubowski/perspective-oneoff-tests/actions/runs/11604564693/job/32313519748#step:8:187

I think that's because of pip's build isolation -- the build is happening in some other working directory, so Cargo doesn't find .cargo/config.toml

will want to adjust the CI job. I think, like I mentioned, setting it in $HOME/.cargo/config.toml will work.

Another option would be including a .cargo/config.toml in the sdist which patches the perspective-server and perspective-client dependencies to use a git commit, something like:

[patch.crates-io] perspective-client = { git = "https://github.com/finos/perspective.git", rev = "deadbeef" } perspective-server = { git = "https://github.com/finos/perspective.git", rev = "deadbeef" }

where deadbeef is the git commit SHA that CI ran on. We would want to avoid doing this when the job is run on a release tag, of course.

tomjakubowski · 2024-10-30T01:09:04Z

Did a manual run in my oneoffs repo to verify that the sdist installs on macOS/aarch64: https://github.com/tomjakubowski/perspective-oneoff-tests/actions/runs/11584975281

I think we probably want at least the option to test in CI that (1) installs from the sdist (2) runs the pytest test suite on the installed package (3) verifies that the labextension is listed in jupyter labextension list. Maybe a manual-only (workflow_dispatch) workflow, or one which runs only on release tags? I'll plug away at something like that.

tomjakubowski · 2024-10-30T02:29:05Z

Another CI job with a diff of the sdist from this branch compared to the 3.1.2 sdist on pypi: https://github.com/tomjakubowski/perspective-oneoff-tests/actions/runs/11585843746/job/32255425110

link to diff artifact https://github.com/tomjakubowski/perspective-oneoff-tests/actions/runs/11585843746/artifacts/2120979686

One thing that stood out is that the sdist has lost the release profile flags from the workspace root's Cargo.toml. May want to add those back

tomjakubowski · 2024-10-30T16:37:31Z

rust/perspective-python/build.mjs

+    const readme_md = fs.readFileSync("./README.md");
+    const pkg_info = generatePkgInfo(pyproject, cargo, readme_md);
+    fs.writeFileSync("./PKG-INFO", pkg_info);
+    const include_paths = Array.from(cargo["package"]["include"]).concat([


this is excluding some files from the perspective Python folder that are otherwise added by Maturin, like test arrows

https://github.com/PyO3/maturin/blob/main/src/source_distribution.rs#L624-L646

will port that

decided against porting, instead I just expanded the set of files included with the crate in Cargo.toml. that way is more explicit and it's less code, to boot

This updates the perspective-python build to create an sdist tarball by hand, not going through `maturin sdist`. This bypasses some issues we have seen with Maturin mispackaging the perspective Cargo workspace in the sdist. Maturin is still used as the `build-backend` to build a wheel from the sdist. CI still uses `maturin build` to build wheels. Local development builds still use `maturin develop`. The sdist now includes the .data directory so that Maturin will place it in the wheel correctly. Building an sdist with PSP_BUILD_SDIST=1 will now error if it appears the jupyterlab plugin was not built into the .data directory. Most of the work is in generating a PKG-INFO file. Its output matches what's in the 3.1.2 sdist on pypi, minus a correction to fix the `Home-page` field miscapitalized by Maturin and some unimportant whitespace changes in `Require-Dist`. Signed-off-by: Tom Jakubowski <tom@prospective.dev>

tomjakubowski · 2024-10-31T02:36:04Z

Superceed by #2817

tomjakubowski force-pushed the pack-prebuilt-sdist-minimal branch from 59c7597 to afa3804 Compare October 28, 2024 20:58

tomjakubowski force-pushed the pack-prebuilt-sdist-minimal branch from 14832ea to a8b7bef Compare October 29, 2024 23:26

tomjakubowski changed the title ~~Package labextension with sdist~~ Generate sdist by hand Oct 29, 2024

tomjakubowski commented Oct 29, 2024

View reviewed changes

tomjakubowski force-pushed the pack-prebuilt-sdist-minimal branch from a8b7bef to 5e94533 Compare October 30, 2024 00:17

tomjakubowski marked this pull request as ready for review October 30, 2024 01:01

tomjakubowski commented Oct 30, 2024

View reviewed changes

tomjakubowski force-pushed the pack-prebuilt-sdist-minimal branch from 2667962 to 84c32e5 Compare October 30, 2024 21:22

tomjakubowski mentioned this pull request Oct 31, 2024

Fix perspective-python sdist and add sdist tests to CI #2817

Merged

tomjakubowski closed this Oct 31, 2024

tomjakubowski reopened this Oct 31, 2024

tomjakubowski closed this Oct 31, 2024

tomjakubowski mentioned this pull request Nov 5, 2024

CI sdist fixes, port conda patch, fix ODR violation #2826

Open

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Generate sdist by hand #2811

Generate sdist by hand #2811

tomjakubowski commented Oct 28, 2024 •

edited

Loading

tomjakubowski commented Oct 28, 2024 •

edited

Loading

tomjakubowski Oct 29, 2024

tomjakubowski Oct 31, 2024 •

edited

Loading

tomjakubowski Oct 31, 2024 •

edited

Loading

tomjakubowski Oct 31, 2024 •

edited

Loading

tomjakubowski commented Oct 30, 2024

tomjakubowski commented Oct 30, 2024 •

edited

Loading

tomjakubowski Oct 30, 2024

tomjakubowski Oct 31, 2024

tomjakubowski commented Oct 31, 2024

Generate sdist by hand #2811

Generate sdist by hand #2811

Conversation

tomjakubowski commented Oct 28, 2024 • edited Loading

Pull Request Checklist

tomjakubowski commented Oct 28, 2024 • edited Loading

tomjakubowski Oct 29, 2024

Choose a reason for hiding this comment

tomjakubowski Oct 31, 2024 • edited Loading

Choose a reason for hiding this comment

tomjakubowski Oct 31, 2024 • edited Loading

Choose a reason for hiding this comment

tomjakubowski Oct 31, 2024 • edited Loading

Choose a reason for hiding this comment

tomjakubowski commented Oct 30, 2024

tomjakubowski commented Oct 30, 2024 • edited Loading

tomjakubowski Oct 30, 2024

Choose a reason for hiding this comment

tomjakubowski Oct 31, 2024

Choose a reason for hiding this comment

tomjakubowski commented Oct 31, 2024

tomjakubowski commented Oct 28, 2024 •

edited

Loading

tomjakubowski commented Oct 28, 2024 •

edited

Loading

tomjakubowski Oct 31, 2024 •

edited

Loading

tomjakubowski Oct 31, 2024 •

edited

Loading

tomjakubowski Oct 31, 2024 •

edited

Loading

tomjakubowski commented Oct 30, 2024 •

edited

Loading