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

Add windows support #180

Open
wants to merge 5 commits into
base: dev
Choose a base branch
from
Open

Add windows support #180

wants to merge 5 commits into from

Conversation

NoamDev
Copy link

@NoamDev NoamDev commented Sep 21, 2020

Add support for windows.

Build requirements:

  • windows or semi-windows with:
    • python 3.6.8

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

@NoamDev
Copy link
Author

NoamDev commented Sep 21, 2020

cc @krisgesling would you review this please?
I'm here for any question ;-)
Sorry it took me so long...

@NoamDev
Copy link
Author

NoamDev commented Sep 21, 2020

cc @forslund as well
Btw, #174 should merged first and then I'd need to adapt my PR a little.

@NoamDev
Copy link
Author

NoamDev commented Sep 21, 2020

I built windows artifacts as an example in:
https://github.com/NoamDev/mycroft-precise/actions/runs/265365726

@NoamDev NoamDev closed this Sep 21, 2020
@NoamDev NoamDev reopened this Sep 21, 2020
@krisgesling krisgesling self-requested a review September 22, 2020 05:02
Copy link
Contributor

@krisgesling krisgesling left a 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):
Copy link
Contributor

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

Copy link
Author

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)
Copy link
Contributor

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?

Copy link
Author

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

@krisgesling krisgesling added enhancement CLA: Yes Contributor License Agreement exists (see https://github.com/MycroftAI/contributors) labels Sep 22, 2020
@NoamDev
Copy link
Author

NoamDev commented Sep 22, 2020

I did as you asked:)

@NoamDev
Copy link
Author

NoamDev commented Sep 22, 2020

Oh, apparently even though precise-engine works perfectly, the scripts used for training do not.
This isn't a reason to delay merging this, as precise-engine is the most important.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA: Yes Contributor License Agreement exists (see https://github.com/MycroftAI/contributors) enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants