-
Notifications
You must be signed in to change notification settings - Fork 113
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
Fix packaging #1766
Fix packaging #1766
Conversation
Signed-off-by: Juan Luis Cano Rodríguez <[email protected]>
Fix gh-1527 Co-authored-by: Mehdi Naderi Varandi <[email protected]> Signed-off-by: Juan Luis Cano Rodríguez <[email protected]>
Signed-off-by: Juan Luis Cano Rodríguez <[email protected]>
7f8c622
to
54f6b21
Compare
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
Signed-off-by: ravi-kumar-pilla <[email protected]>
Signed-off-by: Juan Luis Cano Rodríguez <[email protected]>
ff46fba
to
e22a6ba
Compare
This is amazing!!!! -- thanks @astrojuanlu ! liking the nerd-snipe you. |
Amazing PR! This is a HUGE improvement. Just two things I'm curious about:
|
|
Signed-off-by: ravi-kumar-pilla <[email protected]>
Signed-off-by: ravi-kumar-pilla <[email protected]>
Signed-off-by: ravi-kumar-pilla <[email protected]>
Signed-off-by: ravi-kumar-pilla <[email protected]>
Signed-off-by: ravi-kumar-pilla <[email protected]>
Signed-off-by: ravi-kumar-pilla <[email protected]>
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.
This already looks great and I tried to resolve conflicts with main and fixed few issues. As per my understanding, this PR resolves -
- PyPI sdists are not self-contained #1267
- Migrate from '...requirements.txt' to 'pyproject.toml' #1584
There needs to be some work to resolve - #1611
Thank you for the PR @astrojuanlu . I followed the QA notes and it works as expected.
Thanks @ravi-kumar-pilla ! I’ll double check the final state after you solved the conflicts |
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 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.
LGTM.
Description
Make sdists self-contained, hence fix #1267
Development notes
Commits:
setup.py
, which have been deprecated for about 2 years https://blog.ganssle.io/articles/2021/10/setup-py-deprecated.htmlpyproject.toml
using @MehdiNV work in Migrate from '...requirements.txt' to 'pyproject.toml' #1584 as baseline with some updates, hence fix Move to pyproject.toml #1527setuptools
tohatchling
https://hatch.pypa.io/, same backend vizro uses (cc @antonymilne), as a preparation to eventually address Can the editable install come withpackage data
? i.e.pip install -e .
#1611 and also fix PyPI sdists are not self-contained #1267README.md
topackage/
and create root symlink to retain GitHub previewQA notes
make package
inmain
and with this PR, save the resulting sdist and wheel indist.old
anddist
respectively.tar.gz
, and compare thePKG-INFO
files of both sdists - there should be only lines out of order and other non consequential changes directories withmain
and with this PR.tree
structure of both sdists - only metadata files should have changed.*.dist-info
directories - there should be only lines out of order but the number of files inRECORD
should be basically the same.pip install ./dist.old/...tar.gz
doesn't work (hence PyPI sdists are not self-contained #1267) andpip install ./dist/...tar.gz
fixes the issuekedro viz run
on a test project after installing the new sdist, verify that everything works as expected (the HTML files are included)Checklist
RELEASE.md
file