-
Notifications
You must be signed in to change notification settings - Fork 232
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
Add windows support #180
base: dev
Are you sure you want to change the base?
Add windows support #180
Conversation
cc @krisgesling would you review this please? |
I built windows artifacts as an example in: |
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.
Wow congrats!!
I'll need to get a Windows install setup to test it properly but I think it's a good move to keep the batch scripts small and do as much as possible in Python.
A few quick things based on a first glance, and would be great to add docstrings to help document the various functions, their arguments and return values. See #6 in this list from our contributing guide
win-package.py
Outdated
except FileNotFoundError: | ||
return False | ||
|
||
def package_scripts(tar_prefix, combined_folder, scripts, train_libs): |
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.
it looks like it could be useful to split this function up a little bit for readability
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.
Ok, can do.
Basically it's a port of build.sh, but why not :)
@@ -29,11 +30,13 @@ if train_libs: | |||
] | |||
hidden_imports += ['h5py'] | |||
|
|||
datas = collect_data_files('astor', subdir=None, include_py_files=False) |
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.
What does this return? A list of data files?
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.
The astor lib uses an internal file called VERSION.l, which pyinstaller does not include by default.
Pyinstaller does allow you to specify data files which should be bundled along.
See
https://pyinstaller.readthedocs.io/en/stable/spec-files.html#adding-data-files
The function returns a list of tuples where the first is a file name and the second is the folder to insert it to.
Here it will collect all yhe non python files in the root directory of astor.
If that's preferable I think I can specify the exact file instead as shown here:
https://pyinstaller.readthedocs.io/en/stable/spec-files.html#using-data-files-from-a-module
I did as you asked:) |
Oh, apparently even though precise-engine works perfectly, the scripts used for training do not. |
Add support for windows.
Build requirements:
Build instructions:
Pretty simple, setup.bat and build.bat are the windows equivalent of setup.sh/build.sh respectively.
Because batch is a nightmare, setup.bat uses a script called win-package.py.
Related: #163