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

Generate sdist by hand #2811

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

tomjakubowski
Copy link
Contributor

@tomjakubowski tomjakubowski commented Oct 28, 2024

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
Copy link
Contributor Author

tomjakubowski commented Oct 28, 2024

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 tomjakubowski changed the title Package labextension with sdist Generate sdist by hand Oct 29, 2024
@@ -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.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

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 <[email protected]>
@tomjakubowski tomjakubowski marked this pull request as ready for review October 30, 2024 01:01
@tomjakubowski
Copy link
Contributor Author

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
Copy link
Contributor Author

tomjakubowski commented Oct 30, 2024

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

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.

1 participant