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

Improve packager #55

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

Conversation

ftsfranklin
Copy link

Behavior changes:

  • Be more aggressive about ignoring files. It used to ignore files if either the file or its direct dir had a leading dot. Now it checks every ancestor, up to the app_root.
  • Copy files to a temporary directory and then builds that.
  • Remove the need to halt on carriage return (Carriage return (CR) handling is burdensome #45).
  • Fix a regex which (I believe) would have caused user's myfile.cupyd to be deleted during build.
  • Add a rudimentary requirements.txt for the packager's libraries.
  • Add a rudimentary .gitignore.
  • There may be some behavior changes for the prints for "Did not include ...".

Possible future improvements:

  • Better way to handle temporary files. Currently, it dumps the files into the build/ folder (the use of which was removed in a previous version), and never cleans up.
  • I don't know what to do about copying the file metadata. Will it be okay if the py files get copied into the router with or without global r/w/x?
  • The normcase call (to canonicize the path before splitting) MIGHT cause problems with dots (but it shouldn't).
  • Clean up shouldinclude2, which was added for how the code used os.walk (instead of glob.iglob('**/*', recursive=True)).
  • Allow explicit file list or ignore list to be passed in.
  • I wasn't sure whether we could use pathlib.

- The previous logic created a new uuid even if it didn't succeed.
- Now short-circuits for missing sdk_key (but not for the other keys).
... instead of reading it from app_root or "section".

Plus other minor refactoring.
Features:
- Copy files to a temporary folder first, and only copy files of interest.
- Strip carriage returns during the copy (removing the need to check for them).
- Ignore files with ANY dots in the path.
- Don't need to clean bytecode anymore, since we just don't copy bytecode to the new working directory.
- .tar.gz in one step.
While I don't know much about requirements.txt, I do know that these two libraries are required for tools.
@amickel
Copy link
Contributor

amickel commented Apr 20, 2022

Hi @ftsfranklin, thanks so much for taking the time to make these improvements! I did notice two issues I would appreciate if you could update the code and re-submit the pull request:

  1. Comment out line 199 on make.py.
  2. Fix issue with copy_without_cr being passed sub-directories using '/' instead of ''. You can see an example of the issue by building the cli_sample app:
    Packaging cli_sample Traceback (most recent call last): File "tools\bin\package_application.py", line 243, in package_application(sys.argv[1], pkey) File "tools\bin\package_application.py", line 212, in package_application new_paths = copy_app_folder(app_root, temp_root) File "tools\bin\package_application.py", line 79, in copy_app_folder copy_without_cr(fpath, outpath) File "tools\bin\package_application.py", line 88, in copy_without_cr open(dst, 'w', newline='\n') as dstfile: FileNotFoundError: [Errno 2] No such file or directory: 'build\cli_sample\pexpect/ANSI.py' Error packaging cli_sample: Command 'python tools\bin\package_application.py cli_sample' returned non-zero exit status 1.

Thanks!

@ftsfranklin
Copy link
Author

Resubmit, instead of adding the changes to the existing PR?

@amickel
Copy link
Contributor

amickel commented Apr 28, 2022

Resubmit, instead of adding the changes to the existing PR?

That works, too!

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