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

Declare metadata in pyproject.toml, remove hatch-nodejs-version plugin #427

Merged
merged 4 commits into from
Nov 17, 2023

Conversation

consideRatio
Copy link
Member

@consideRatio consideRatio commented Sep 25, 2023

When I made a release I bumped to a dev version after, but that had pyproject-build start to fail. This was because the hatch-nodejs-version plugin didn't allow us to use a version in package.json that was 4.1.1-0.dev which is a valid semver2 version. In helm chart's etc that require semver2 compliant versions like package.json also wants, we've ended up using the -0.dev version suffix as it allows us to sort that version before alpha if we make something like that sometime.

In this PR I've instead of adjusting the version 4.1.1-0.dev to something that worked with hatch-nodejs-version plugin adjusted to no longer use it. I've as part of this removed some metadata from package.json and put it directly in pyproject.toml. The version field wasn't put directly there, but in _version.py, allowing it to be inspected via an import statement by other packages.

labextension/package.json Outdated Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved
@bollwyvl
Copy link
Collaborator

bollwyvl commented Oct 4, 2023

Wherever possible, sticking to static fields in PEP 621 whenever possible is going to be a lot more stable over time. I think dynamic was meant more as an ejection site for legacy stuff, rather than something to be embraced so that the declarative metadata is barren of anything actionable.

Somewhat off-topic... `poetry` is _particularly_ bad at the above (and has plugins that won't work as a proper [PEP 517](https://peps.python.org/pep-0517/) `build-system`), so three cheers for this project not using that!

On the main, I've found simple and dumb is better at the actual build-an-archive stage... but the lab community has seemed to gel around the sandwich of a python build system building a huge virtual environment, calling nodejs (which pip can't, and really shouldn't provide), calling python scripts inside that, using plugins that might or might not break with the next release of pip. When, not if, things break, this becomes a hard-to-debug mess of incomprehensible python tracebacks mixed with mountains of yarn output. jupyterlab is also a huge build-time dependency to haul along in the build-system, which complicates the matter further.

Indeed, I actually prefer the anti-extensible flit, running jlpm && jlpm build before pyproject-build, separating these concerns, but now that it works, I'm certainly not advocating for its adoption here.

@consideRatio consideRatio force-pushed the pr/pyproject.toml-metadata branch from 7057e0d to 1234bab Compare November 1, 2023 15:11
@consideRatio
Copy link
Member Author

consideRatio commented Nov 1, 2023

Test failures assumed to be unrelated

We have test errors now, but I don't think I broke it in this PR. This PR is on the other hand fixing our test suite in other regards so I suggest it gets merged to enable easier focus on resolving the other test failure about "socket already in use"

@consideRatio
Copy link
Member Author

Thank you @bollwyvl for reviewing this!!! Sorry for the slow followup - ready for review again!

Copy link
Member

@manics manics left a comment

Choose a reason for hiding this comment

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

Seems OK to me, but I'll let @bollwyvl make the final decision

@@ -1,5 +1,6 @@
from jupyter_server.utils import url_path_join as ujoin

from ._version import __version__ # noqa
Copy link
Collaborator

Choose a reason for hiding this comment

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

Perhaps add an __all__ so this doesn't have to be noqa

Copy link
Member Author

Choose a reason for hiding this comment

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

Would you like the __all__ list everythign currently exported or only some attributes (possibly breaking)?

Ideally, we'd have __all__ listing only what was needed to be exported historically, but I'm not confident on constraining what we export at this point.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I guess once something has been hoisted to __init__.py, for whatever reason, it is part of the public API. Doing in-the-wild searching sounds more tiresome.

This might be an appropriate time to consider getting on the recent push to get more of the stack working with mypy, with a backstop of the unused-imports rules from ruff, which can more generally be informed by sp-repo-review. This is likely better than making on-the-fly, case-by-case calls on specific repos.

Copy link
Member Author

Choose a reason for hiding this comment

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

That sounds interesting @bollwyvl! I'm not able to drive that effort as part of this PR though, feeling somewhat overloaded atm :/

@consideRatio
Copy link
Member Author

@bollwyvl I'll go for a merge at this point, I hope that is considered acceptable. I'd like to see us get the projects CI system back and functional and this is just one step along the way it seems.

@consideRatio consideRatio reopened this Nov 17, 2023
@consideRatio consideRatio merged commit 55d4a9f into jupyterhub:main Nov 17, 2023
12 of 20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Test suite is failing in main branch
3 participants