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

Reduce (and test) sdist size #263

Merged
merged 8 commits into from
Apr 18, 2021
Merged

Conversation

bollwyvl
Copy link
Collaborator

@bollwyvl bollwyvl commented Mar 27, 2021

The 3.0.2 sdist clocks in at 43.3M.

This PR explicitly removes the node_modules from the sdist by updating MANIFEST.in, bringing the .tar.gz down to 82k, and adds a check in the publish CI (with a >2x margin for future growth).

To maintain reproducibility, it removes yarn.lock from .gitignore, which despite the commit thrash, is actually quite useful in tracking down where something might have changed, and will also save re-solving in CI.

@bollwyvl
Copy link
Collaborator Author

Looks like having yarn.lock cuts about ~30s off the build. It can be more impactful to do one of the builds first, then run the second with --install-option="--skip-npm", which avoids the re-run of the build machinery. Happy to do so on this PR, but otherwise nothing planned here.

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.

One comment, but looks good to me.

- name: Check Package Sizes
run: |
ls -lathr dist
find dist -type f -size +200k | grep dist || echo ok
Copy link
Member

Choose a reason for hiding this comment

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

Is this just for info, or should it fail if there's a file > 200k ? At the moment the exit code will always be 0 regardless.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yeah, i need to figure out how to make it fail properly. will try some more stuff locally.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

should be fully resolved by 33b424c

@bollwyvl
Copy link
Collaborator Author

Sorry for the thrash (feel free to squash merge):

@bollwyvl bollwyvl requested a review from manics April 4, 2021 16:37
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.

Sorry for the delay, LGTM.

Is everyone else happy with the addition of yarn.lock? dependabot will update it, but they'll ber loads of PRs, e.g. see https://github.com/manics/jupyter-offlinenotebook/pulls?q=is%3Apr+is%3Aclosed+label%3Ajavascript

@bollwyvl
Copy link
Collaborator Author

ber loads of PRs, e.g.

This hardly seems like a bad thing... anyhow, any thoughts here, folks? That big ol' tarball is still out there burning bits-on-the-wire...

@manics
Copy link
Member

manics commented Apr 18, 2021

I guess silence is acquiescence? 😄

Could you help we with one question- what's the expectation when it comes to merging dependabot PRs to yarn.lock? Does the presence of pinned dependencies in this repo imply a guarantee to developers that they work? We don't have any JupyterLab UI tests so unless it breaks the build CI will always be green. Should we always do a manual UI test of the JupyterLab extension, or is it OK to just merge and assume it's fine?

@bollwyvl
Copy link
Collaborator Author

The cheapest acceptance test is dev-focused binder, which seems useful anyway.

If you want full acceptance testing, I'm happy to spin up some robotframework-jupyterlibrary which can test classic and lab (and watch the server logs) and generate nice reports.

If this is a blocking concern, I can take out the yarn.lock, and have even less assurance that the extension works.

@manics
Copy link
Member

manics commented Apr 18, 2021

I've added #268
If you've got time to add even some basic UI tests that would be great! I've been waiting for jupyterlab/extension-examples#45 to find out how to do it properly.

@manics manics merged commit 708557a into jupyterhub:master Apr 18, 2021
@welcome
Copy link

welcome bot commented Apr 18, 2021

Congrats on your first merged pull request in this project! 🎉
congrats
Thank you for contributing, we are very proud of you! ❤️

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.

2 participants