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

Brought back from an old commit #428

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

dstromberg
Copy link

Restoring the Makefile, to facilitate installing all of micropython-lib.

@stinos
Copy link

stinos commented Jun 10, 2021

This uses max-depth=1, does this still work?
In any case imo ths should be replaced by Python code so it runs on Windows as well. And the code should allow selecting which of the subdirectories to install.

In a later installment I guess upip should allow installation from a local respository (and from github as well perhaps) like pip and then this tool here would call into upip to do the installation of each module.

@dstromberg
Copy link
Author

I've rewritten it in Python 3.4.

Thanks :)

@jimmo
Copy link
Member

jimmo commented Jun 11, 2021

Thanks @dstromberg

So I can understand this better, what's the underlying workflow that you're using this for? I'm guessing you have the unix build in your PATH, and you want to be able to use micropython-lib packages from anywhere? i.e. you're using micropython's unix port as a cpython-replacement, and ~/.micropython/lib as your site-packages equivalent? Or something else?

My experience is far more embedded-focused, so this isn't a scenario that comes up very often.

@dstromberg
Copy link
Author

dstromberg commented Jun 11, 2021 via email

"""
Copy modules from here to --prefix.

Note that we do not scan for directories; instead we have a hardcoded list in all_directories.
Copy link

Choose a reason for hiding this comment

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

Why?

Copy link
Author

Choose a reason for hiding this comment

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

No particular reason. One way you could accidentally copy a new doc/, the other you could accidentally not copy a new from-github.

all_directories = ['micropython', 'python-ecosys', 'python-stdlib', 'tools', 'unix-ffi']


def usage(retval):
Copy link

Choose a reason for hiding this comment

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

Minor, but imo clearer to pass the write argument directly and have it default to sys.stdout.write: obvious, clearer below in the case stderr gets used beause one doesn't have to wonder what passing 1 means

Copy link
Author

Choose a reason for hiding this comment

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

It's like sys.exit(0)

sys.exit(retval)


# The getopt-like module du jour would be a little more concise, but that's much harder for a static analyzer to check.
Copy link

@stinos stinos Jun 11, 2021

Choose a reason for hiding this comment

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

argparse is hardly 'du jour' and arguably your static analyzer is not good if it cannot handle that

Copy link
Author

Choose a reason for hiding this comment

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

If you know of a static analyzer that does this, I'll probably want to switch.

Copy link

Choose a reason for hiding this comment

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

What needs to be checked, and what problem are you having with argparse?

Copy link
Author

Choose a reason for hiding this comment

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

Code needs to be checked, and I haven't used an argument parser since getopt.

Copy link

Choose a reason for hiding this comment

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

Can you be more concrete? I.e. this comment sounds like something doesn't work for a particular static analyzer. As such it raises more questions that it answers. My question really is: what does not work, and for which analyzer.

"""
Recursively copy a directory tree.

This is a slightly-modified copy of copytree from CPython 3.4. The motivation was to allow dst to preexist.
Copy link

Choose a reason for hiding this comment

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

Surely there are simpler ways, no? Doesn't seem right to introduce dependency on such a large function simply to copy some files

Copy link
Author

Choose a reason for hiding this comment

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

It's not really a dependency, but this is a library function that lacks a dirs_exist_ok option. That's not present until Python 3.8+ though.

Copy link

Choose a reason for hiding this comment

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

Ok. But it's a dependncy on a lot of non-trivial code unrelated to the actual task at hand. Plus I'm not sure one can just copy from CPython without mentioning its license? Anyway, dirs_exist_ok is actaully a problem in my experience: what typcially happens is a file gets deleted from the source (this repository) but then it just remains in the target and causes havoc there. Deleting all directories first (as in: loop over the first level and delete everything encounterd in prefix) is better and safer. Plus makes copy_tree usable again.

Copy link
Author

Choose a reason for hiding this comment

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

The code can be trivially removed when 3.8 is adopted.

CPython is fully opensource.

Goodness, no, deleting a hierarchy isn't even close to safer.

Copy link

@stinos stinos Jun 15, 2021

Choose a reason for hiding this comment

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

The code can be trivially removed when 3.8 is adopted.

Fair enough I guess, still a pitty.

CPython is fully opensource.

That doesn't mean one can copy without mentioning license. MicroPython is fully open source but requiers its license to be dsistributed with copies.

Goodness, no, deleting a hierarchy isn't even close to safer.

How so (in this particular case)? Do you know another way to deal with the problem I mentioned?

installer Outdated
copytree(source_dir, dest_dir, symlinks=True, ignore=ignore, ignore_dangling_symlinks=False)


os.makedirs(prefix, exist_ok=True)
Copy link

@stinos stinos Jun 11, 2021

Choose a reason for hiding this comment

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

can you wrap this in a if __name__ == "__main__": so it doesn't always execute? Much easier for testing

Copy link
Author

Choose a reason for hiding this comment

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

if'd.

installer Outdated
"""
Copy modules to a lib location.

This was a shell command:
Copy link

Choose a reason for hiding this comment

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

Not really interesting to know, but I wouldn't repeat this here because it's a matter of time before it becomes obsolete, because more ignore patterns need to be added. Just calling this copy_module and mentioning it will exclude anything but module files is clear enough?

Copy link
Author

Choose a reason for hiding this comment

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

It's gone.

@stinos
Copy link

stinos commented Jun 11, 2021

So I can understand this better, what's the underlying workflow that you're using this for? I'm guessing you have the unix build in your PATH, and you want to be able to use micropython-lib packages from anywhere?

That's what we do as well, roughly: to deploy applications which use select items from the micropyton-lib repository we copy over those directories to a lib directory. Which can then get redistributed as-is to other computers. All of this is simpler and faster than using pip, and works without global internet access as well. But as said, in the end it would probably be better to have upip handle this.

I've been patching my micropython binaries

You can set the MICROPYPATH environment variable for that, equivalen of PYTHONPATH, no need for patching.

@dstromberg
Copy link
Author

I'd really rather patch the binary, so anyone on the system can use it conveniently.

installer Outdated
@@ -0,0 +1,131 @@
#!/usr/local/cpython-3.4/bin/python3
Copy link

Choose a reason for hiding this comment

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

Too strict probably, and for consistency with other tools should be #!/usr/bin/env python3, and the filename should have the .py extension, and perhaps even be placed in tools/ directory though that makes it harder to discover.

Also the file should be formatted using tools/autoformat.py so that one might need to be changed as well (didn't test though).

Copy link
Author

Choose a reason for hiding this comment

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

I do a specific #! sometimes, to document what version of Python I was testing against. I agree that using /usr/bin/env is better for portability, even though it's less clear, and shows up in top less nicely.

installer Outdated

prefix = os.path.expanduser('~/.micropython/lib')
directories = []
all_directories = ['micropython', 'python-ecosys', 'python-stdlib', 'tools', 'unix-ffi']
Copy link

Choose a reason for hiding this comment

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

tools shouldn't be copied probably, since it copies it codeformat.py which only works for this repository

Copy link
Author

Choose a reason for hiding this comment

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

It's gone now.

installer Outdated

def copy(source_dir, dest_dir):
"""Copy modules to a lib location."""
ignore = shutil.ignore_patterns('test_*', 'setup.py', 'dist', '*.ogg-info', '__pycache__')
Copy link

Choose a reason for hiding this comment

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

ogg-info -> egg-info

But there are some other problems: the original copies only .py files, now everything gets copies, which results in metadata.txt getting copies, and the README.md of all subdirectories, but they are all copies into the same location so one ends up with unix-ffi's readme.md in the root of the prefix.

Copy link
Author

Choose a reason for hiding this comment

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

Typo fixed.

As far as what to copy, I've just been rewriting the Makefile. Maybe we should handle things like the stray README.md as a separate commit? This is already more arduous than it should be.

Copy link

Choose a reason for hiding this comment

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

But the stray README is because this does something very different from the Makefile, which used to copy *.py only.


for directory in directories:
print('Copying directory {}'.format(directory))
os.chdir(directory)
Copy link

Choose a reason for hiding this comment

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

Would be better to use full paths here, less chance of things not working as expected.

Copy link
Author

Choose a reason for hiding this comment

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

No, not really. Relative paths have their place. So do absolute paths. But converting all to the other is not a good thing.

Copy link

Choose a reason for hiding this comment

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

Please explain. Right now the tool is only usable from the micropython-lib directory which is quite limiting. As in, first thing I tried was to just run it from another place, like code-format.py, and that didn't work.

@dbrignoli
Copy link

The original Makefile, which I use to install micropython-lib, doesn't really use or need any features of make and I agree it would be nice to have something equivalent in Python. However, after reading this conversation and comparing the original shell script from the Makefile and the proposed solution in Python, I wonder if the practical and concise solution could be to have a shell script for bash/posix and one for windows.

@stinos
Copy link

stinos commented Jun 15, 2021

Note that if it weren't for the copy-tree implementation combined with using argparse this would be pretty concise.

But the more I think about it, the more I feel the way forward is to use upip. It already knows which files to skip and how to copy the files etc, just needs some extra bits to not unpack a tar from a URL but to use files on disk. Then unless I'm missing something the installation here would be just 'for subdir in listdir(mainDirs): upip(subdir)'. But changing upip might be non-trivial, don't know.

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.

None yet

4 participants