-
Notifications
You must be signed in to change notification settings - Fork 1
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
feat: add platformio #104
base: main
Are you sure you want to change the base?
feat: add platformio #104
Conversation
|
||
CWD = os.getcwd() | ||
|
||
fqbn_to_board = { # Mapping from fqbn to PlatformIO board |
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.
we also have rp2040.
from deps.logs import logger | ||
from models import Sketch, PythonProgram, Messages | ||
|
||
from deps.sketch import _install_libraries, _compile_sketch, startup |
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.
these functions should not be private (_), since they are used outside the module.
if not await check_for_internet(): | ||
return | ||
logger.info("Updating library index...") | ||
global library_index_json, library_indexed_json # pylint: disable=global-statement |
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.
if you put all these functions in a class, use don't need a global. The global will become a private property of the class.
return | ||
logger.info("Updating library index...") | ||
global library_index_json, library_indexed_json # pylint: disable=global-statement | ||
async with httpx.AsyncClient() as client: |
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.
if this becomes a class, you can store the httpx client an re-use it instead of re-creating it for every request
return dict(line.split("=") for line in properties.split("\n") if "=" in line) | ||
|
||
|
||
async def _install_library_zip( |
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.
why not use pio pkg install instead of this manual stuff? And then link the lib with something like this:
[env:myenv]
lib_deps =
FooLib=symlink://../../shared-libraries/FooLib
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.
Using pio pkg install still requires some weird stuff since we'd have to create a enviroment to install the packages in and handle it must be compatible with multiple platforms etc.
deps = [] | ||
deps_arches = {} | ||
in_deps = {} | ||
for file in library_zip.namelist(): |
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.
this is all blocking IO and will hang the webserver for as long as it takes to unzip the files. Is there an async version of these calls?
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.
This isn't blocking IO, the zip is never saved it's in memory
for i, dep in enumerate(deps): | ||
deps[i] = dep.strip() | ||
in_deps = await _install_libraries(deps) | ||
for dep in deps: |
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.
you're already going through all the deps 2 lines above (enumerate), why not just do this in that loop?
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.
Because the line above we do the recursively called _install_libraries call from which we get the variable installed_deps (in_deps) which we need for this loop
if board not in deps_arches[dep] and "*" not in deps_arches[dep]: | ||
continue | ||
dir_path = f"../{dep}@{in_deps[dep]}/" | ||
dir_deps[board] += f"\t\t\t{dir_path}src\n" |
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.
see above, why not just let platformio handle all this and use the regular deps mechanism. We're introducing platformio but don't actually use it? all this code seems fragile and messy to maintain. Esp. when considering this also has to run on a PI and in the desktop client. Less == more.
async with aiofiles.open(platformio_config_path, "w+") as _f: | ||
libs = "" | ||
includes = "" | ||
for lib in installed_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.
do we need to rebuild the whole project for every compile? Can't we have one project per board type (or even one project with several envs), and always compile from there? That way caching of compiled artifacts also works like it should.
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.
No because some projects require other libs then others, and libs could override (like having the same named headers) each other which would make it dangerous for stability
|
||
logger.info("Installing libraries: %s@%s", library, version_required) | ||
|
||
repo = list( |
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.
javascript-ism: please use a list comprehension. It's more readable and faster.
library_dir = f"./arduino-libs/{repo['name']}@{repo['version']}" | ||
async with httpx.AsyncClient() as client: | ||
library_zip = zipfile.ZipFile( | ||
io.BytesIO((await client.get(repo["url"])).content) |
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.
no error handling here? Also, please add timeouts for httpx calls.
No description provided.