-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
base: master
Are you sure you want to change the base?
Generate sdist by hand #2811
Conversation
59c7597
to
afa3804
Compare
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):
And the wheel artifact still has the one copy of the labextension, in the .data directory, as expected |
14832ea
to
a8b7bef
Compare
@@ -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 was a problem hiding this comment.
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]>
a8b7bef
to
5e94533
Compare
Did a manual run in my 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 |
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 |
This updates the perspective-python build to create an sdist tarball by
hand, not going through
maturin sdist
. This bypasses some issues wehave seen with Maturin mispackaging the perspective Cargo workspace in
the sdist.
Maturin is still used as the
build-backend
to build a wheel from thesdist. CI still uses
maturin build
to build wheels. Localdevelopment 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 unimportantwhitespace changes in
Require-Dist
.Pull Request Checklist
Discussions, this PR applies to. (N/A)