Update Makefile and README to improve usability and fix issues on Windows #168
Conversation
- Standardized Python interpreter usage via `PYTHON_EXEC` for cross-platform builds.
- Addressed Windows-specific issues:
- Fixed `UnicodeEncodeError` by enabling Python's UTF-8 mode.
- Ensured correct module and asset discovery through robust `PYTHONPATH` handling (e.g., `./src`, `./kolibrisrc`).
- Improved `get-whl` target logic for more reliable wheel downloads.
| get-whl: clean-whl | ||
| # The eval and shell commands here are evaluated when the recipe is parsed, so we put the cleanup | ||
| # into a prerequisite make step, in order to ensure they happen prior to the download. | ||
| $(eval DLFILE = $(shell wget --content-disposition -P whl/ "${whl}" 2>&1 | grep "Saving to: " | sed 's/Saving to: ‘//' | sed 's/’//')) |
There was a problem hiding this comment.
Just flagging that we've lost something in the refactor here - this currently deliberately uses the --content-disposition flag in order to save the file with wget using the content disposition header, when available.
An alternative to this refactor would be to invoke the original recipe if not on Windows, and use this powershell recipe otherwise: https://github.com/learningequality/kolibri-installer-windows/blob/main/.github/workflows/build_exe.yml#L82
There was a problem hiding this comment.
So -O flag here wget --content-disposition -O "$(OUTPUT_PATH)" "$(whl)" would override the output filename and defeat the purpose of --content-disposition flag right?
There was a problem hiding this comment.
I removed the --content-disposition, since currently it's not needed.
Makefile
Outdated
| PYTHONPATH=$$PYTHONPATH:./kolibrisrc python3 -m kolibri manage loadingpage --output-dir src/kolibri_app/assets --version-text "${KOLIBRI_VERSION}-${APP_VERSION}" | ||
| # -X utf8 ensures Python uses UTF-8 for I/O, fixing UnicodeEncodeError on Windows. | ||
| ifeq ($(OS),Windows_NT) | ||
| PYTHONPATH="./kolibrisrc;%PYTHONPATH%" $(PYTHON_EXEC) -X utf8 -m kolibri manage loadingpage --output-dir src/kolibri_app/assets --version-text "${KOLIBRI_VERSION}-${APP_VERSION}" |
There was a problem hiding this comment.
This might be overkill but we could consider defining a PYTHONPATH_BASE variable at the top of the Makefile based on the OS (similar to how PYTHON_EXEC is set). That way, we avoid repeating PYTHONPATH="./kolibrisrc:$$PYTHONPATH" ?
There was a problem hiding this comment.
Yep, seems like a good idea, I'll do it
There was a problem hiding this comment.
Could even do it as PYTHON_EXEC_WITH_PATH, as we only need PYTHONPATH in the context of executing Python - with the PYTHON_EXEC_WITH_PATH being:
PYTHONPATH="./kolibrisrc;%PYTHONPATH%" $(PYTHON_EXEC)
on windows?
There was a problem hiding this comment.
I introduced the PYTHON_EXEC_WITH_PATH variable as rtibbles said.
ozer550
left a comment
ozer550
left a comment
There was a problem hiding this comment.
Apart from adding some more instructions around wget that we talked about, everything seems to be working as expected! Nice work.
|
Build still builds, and dmg file is correctly named, so I have no concerns here! |
3 participants
Summary
This pull request introduces a set of changes to enhance the development and build process for the
kolibri-app, with a strong focus on improving Windows compatibility and clarifying the developer documentation.The changes are as follows:
Makefile:
PYTHON_EXECvariable to dynamically usepythonon Windows andpython3on Unix-like systems, eliminating windows command failures.get-whltarget has been refactored to be more reliable. It now uses simplified filename extraction using basename to get the filename from the URL and sed to remove query parameters.wgetnow downloads directly to this precalculated clean path using the -O option.UnicodeEncodeErrorFix: Resolved aUnicodeEncodeErroron Windows during themake loading-pagesstep by forcing Python to use UTF-8 mode via the-X utf8flag.PYTHONPATHHandling: Implemented OS-awarePYTHONPATHlogic forloading-pagesandrun-devtargets. This ensures that modules fromkolibrisrcand local app code fromsrcare correctly discovered on both Windows (using semicolons) and Unix-like systems (using colons).README.md:
Has been updated to improve ease of use for developers
wgethas been added as a requirement for downloading Kolibri wheels , with a suggested source for Windows users.Manual Verification:
These changes have been manually tested on:
venvvirtual environmentThe following commands were successfully executed:
make dependencies,make get-whl,make pyinstaller, andmake run-dev.Additionally, the
build_mac.ymlworkflow was used to ensure that these changes did not introduce any issues for the macOS build process.References
This PR fixes #155
Reviewer guidance
To test the updated Makefile, please use the updated README.md