Skip to content

Make upip installable #56

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

Merged
merged 2 commits into from
Oct 14, 2021
Merged

Conversation

wnienhaus
Copy link
Collaborator

I added a setup.py to build a upip distribution package of py-esp32-ulp. I called the package "micropython-py-esp32-ulp" trying to follow the convention that micropython packages in pypi start with the "micropython-" prefix.

Setup.py uses the "sdist_upip" module (included in the commit) to create a package in the correct format for upip. The main "correct" part is that upip requires a smaller (non-default) gzip dictionary size, so that the archives can actually be uncompressed on memory-constrained devices.

Two questions/concerns:

  • sdist_upip.py originally comes from the micropython-lib project. They removed this recently without any explanation, but it lives on as part of the pycopy project in pycopy-lib. I have taken a copy there and included this in this project. It's MIT licensed and begins with a long attribution header, so it should be ok, but I am not entirely sure about including files from other projects. The alternative could be a build/make script that downloads that file when needed, but it seems a lot more complicated. So, for now I chose the path of including the file (as I have seen some other micropython modules have also done). Is this ok?

  • Who should publish this to pypi? I have created an account on test.pypi.org to test how this all works, but perhaps it's better that you @ThomasWaldmann actually publish it, as owner of the project and github repo we reference? Perhaps later we can automate the publishing with Github Actions, as PyPi appears to allow the creation of API tokens for this purpose, but even here it might be better if you own this?

setup.py (setuptools) uses the sdist_upip module (included in this
commit) to generate packages in the correct format for use with upip.

To generate a distribution package, make sure the VERSION is set as
desired in setup.py and then run `python3 setup.py sdist`. The result
will be a tar.gz file in the dist/ subdirectory.
@wnienhaus wnienhaus mentioned this pull request Oct 13, 2021
gzip_4k(outbuf, self.archive_files[0])

return r

Copy link
Collaborator

Choose a reason for hiding this comment

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

code quality here could be better.

unclear: is this the usual code used for this purpose / is there anything better?

Copy link
Collaborator

Choose a reason for hiding this comment

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

also: do we need to bundle this or can we just pip install something to get this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I used the file as-is. Other than the comments at the topic referring to "pycopy-lib" the file is essentially the same as the file which was part of "micropython-lib". I would keep the file "as-is" (including to allow comparing it to the source in the future, if the source might have changed/improved).

About bundling: I searched hard to find an installable version of this. I didnt manage. I did find a number of micropython modules (e.g. micropython-umqtt.simple2) that do have a setup.py, which import sdist_upip, but that don't ship along that file. So there could be a way yet to install that file in a way I couldn't figure out. I did find one module that shipped the file along.

Interestingly micropython-umqtt.simple2 imports sdist_upip from the parent directory .. (see here), which doesn't feel very "installed". Instead it feels like the author cloned the "pycopy-lib" repo, which has sdist_upip in its root, and then created a subdirectory for his module (or copying the existing umqtt.simple dir), and following the same pattern as other modules in the "pycopy-lib" repo. All modules in that repo import sdist_upip from .. which is the root of that repo.

So to summarise: I could not find a place to pip install it from and others seem to also rely on the file from either micropython-lib or pycopy-lib, so I chose the bundling option.

Copy link
Collaborator

Choose a reason for hiding this comment

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

OK. I agree to use 3rd party code "as is" to simplify updates (if any).

If there is no alternative, this is as good as it gets.

HERE = Path(__file__).parent
README = (HERE / 'README.rst').read_text()

VERSION = "1.0.0"
Copy link
Collaborator

Choose a reason for hiding this comment

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

over time, we'll need to improve how this is handled.

either by using a tool that updates versions everywhere (here, code, docs, ...) or by deriving version from git (for other "normal python" projects i use setuptools_scm).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Are you saying, it's ok to have it hard coded for now, and only improve later? (It might be good to not take on too much more before getting to v1.0.0)

But I like the approach of deriving the version from git. Especially when putting the build/publish into a Github Action, it would allow understanding where each build artifact actually came from. Also, given that this setup.py is run with python3 (the dependencies alone make it not run on MicroPython), I would expect setuptools_scm to work just fine (i.e. that there's nothing MicroPython specific here).

Let me know if you feel this should be addressed before launch of v1.0.0

Copy link
Collaborator

Choose a reason for hiding this comment

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

ok for now.

not sure if setuptools_scm can be used, it is made for normal python code in a git repo.
we have half of that (== git) on the dev machine, but none of that on the target device.

setup.py Outdated
'License :: OSI Approved :: MIT License',
'Programming Language :: Python :: Implementation :: MicroPython',
],
platforms=["esp32"],
Copy link
Collaborator

Choose a reason for hiding this comment

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

well, guess we also have linux/unix here. just leave it away? not sure what the valid values here are.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

added "linux" and "darwin". (pushed as extra commit on top)
It appears this field is not used by any tools, so we could also omit it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

as long as it is valid and not rejected by pypi, we can keep it.

It appears the "platforms" fields it not actually used by any tools but
can be used to communicate to users what platforms the package is meant
to work on. (https://stackoverflow.com/a/34994245)

Including linux and darwin here, because the code runs well on the unix
port of MicroPython, which itself runs well on Linux and macOS (darwin).
In fact most of the testing had been done using the unix port.
@ThomasWaldmann ThomasWaldmann merged commit 8e2dde1 into micropython:master Oct 14, 2021
@wnienhaus wnienhaus deleted the upip-installable branch November 6, 2021 12:24
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.

2 participants