-
Notifications
You must be signed in to change notification settings - Fork 14
building from source task #65
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
base: main
Are you sure you want to change the base?
Conversation
UncleGrumpy
left a comment
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 a very cool feature! One that has been on my todo list to add to the atomvm_rebar3_plugin for a while. I work much more in Erlang, so my Elixir is not that great, but I did spot a few problems, and have some suggestions on improving usability.
|
It might also be good to consider allowing a ESP-IDF docker image. I suspect many more Elixir developers will already have docker installed (or at least be more familiar with it than ESP-IDF), so it would be extra cool to pull in a docker image of ESP-IDF if they don't already have it installed. |
|
Hey @UncleGrumpy , when i try to build AtomVM , the build fails because we are using the the |
|
Added docker feat based on this |
This is not something I have seen before, and I have been working almost exclusively with main branch for at least the last few months. Was this testing a docker build, or did this happen with an installed ESP-IDF? It has been a few ESP-IDF releases since I last used an esp-idf docker image, although we do use them in the AtomVM CI, so I don’t think that would be the problem. |
|
@UncleGrumpy , this issue from the missing header happened with both toolchains. Esp idf installed and docker |
|
Interesting. Which ESP-IDF version? I think I am a point release behind locally (still using 5.5.1). I will try updating my toolchain this evening and see if I can reproduce the problem, I believe the AtomVM CI may also need to be bumped to 5.5.2, I will look into that too. This may be a new issue that you were the first to discover. I think you should open an issue in the AtomVM repo, and be sure to include Host OS, and ESP-IDF version in the report, along with the error and any other details that might be relevant. |
|
I tried with the last ESP version 6.X and then I tried with 5.4.1 which afaik is the last supported by AtomVM but not sure. It happens the same with both versions. |
I have not tried 6.0 yet, but I believe a PR was recently merged to allow ESP-IDF 6.0 builds. ESP-IDF 5.5.1 is definitely working for main branch, we might not have updated the main branch docs to reflect this, that is something we will need to make sure is up to date before a 0.7.0 release of AtomVM. Since you are hitting this error with ESP-IDF 5.4.1 and 6.0 then I don’t think just switching to 5.5.x will be any different. |
|
Okay @UncleGrumpy , today I will post an issue in AtomVM with this error and how to reproduce it. Meanwhile could you review the code and see if everything is fine ? |
|
Hey @UncleGrumpy , I tried 4 times to compile the AtomVM cloning each time just in case and it seems to be fixed. this command but with each version mix atomvm.esp32.build --use-docker --chip esp32s3 --idf-version latest
it seems i had something weird in the cache or it has been fixed. by default |
That is great! I would not necessarily expect any of the 6.x IDF versions to work at the moment. I think AtomVM itself will need some updating. If it works with stable releases from 5.1 (or 5.2) to 5.5 that is all that would be expected at the moment. The AtomVM main branch uses 5.5.x for release builds so that should probably be the default here too. I believe 5.5.2 is the latest, we still need to bump that in the AtomVM CI, which I believe is still using 5.5.1. There shouldn’t be any breakage between these versions since the point releases are usually just bug fixes. |
I discovered the problem with the docker container. I am working on a fix for the AtomVM repo that will change the configuration of the mkimage.sh and mkimage.config. The problem is that the paths to the binary files are configured by cmake in the container and then need to be executed in the host environment to use OTP to execute the escript. I can change the mkimage.sh script to find the esp32/build dir at runtime (rather than have it set by cmake) and pass that directly to the escript to use as the base for finding the partition binaries. We should fix this there so users, other tools, and IDEs can use docker builds. |
|
Amazing I had a mini fix which was patching the paths for the container build but it seems that is not working. |
|
Happy new year ! 🎆 |
|
You can try using this fix atomvm/AtomVM#2052 for the AtomVM repo and see if that fixes your problems with the docker builds. |
|
@UncleGrumpy just tried your changes about that PR and the docker build is working !!! @petermm , could you try to build the atomvm with the task with the custom mbed tls path ?? |
Great! That is not a complete fix, though. I targeted that fix to main, I probably should have done that on release-0.6 branch (maybe we can back port it, or I can submit a separate fix for release-0.6)… but that still won’t cover previous release tags, like v0.6.6. There really isn’t a way to push the fix back to cover those, so you may need to work out the path rewrite for those older tags, or we can just say this feature is supported for main branch (and release-0.6 branch if we apply the fix there too) and tagged releases starting from 0.7.0 (or 0.6.7 if we back port… and do a 0.6.7 release). Another option might be to use git and cherry-pick the fix on top of whatever older checkout that the user might building from. |
I believe this feature is oriented to people who want to build and experiment with newer releases, anyways, if they want to build 0.6.6 they can always make the build with Esp IDF installed in the host instead of docker build. I will document that docker build is supported since the moment you merged that PR in main branch. I think adding this feature for older AtomVM builds doesn't give much more value. |
|
I agree. If they want older tagged releases they can just install the official release download. It will be devs wanting to test the newest features that will be using this task. |
Amazing, I will commit soon with the docs comment and addressing Peter comment as well. |
|
@UncleGrumpy , added Note about docker usage. add docker usage note, and address build generic_unix options passing i was thinking we can add a guide of how to install the ESP-IDF in nix flake since it is a reproduceable environment is cool. (for telling the users who want to build old releases how to make them). |
This would be great, but this repo would not be the best place for that. I would suggest either submitting “Tutorial” here: This can be done from the atomvm/atomvm.org repo. I would add a new page and link to it from the Tutorials page. Or you could add a new section to the end of the esp32 build instructions here: Which is generated from atomvm/AtomVM/doc/src/build-instructions.md. |
i believe tutorials section is better, do you think this is ready to be merged ? |
|
@bettio , could you review please ? |
Signed-off-by: Pepe Márquez Romero <pepe.marquezromero@protonmail.com>
| clean = Keyword.get(opts, :clean, false) | ||
|
|
||
| # Get mbedtls_prefix from option or environment variable | ||
| mbedtls_prefix = |
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.
In the interest of calling a spade a spade - I suggest calling this cmake_prefix as that is what it is..
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 reason is called mbed_prefix is because it edits in the cmake for building mbed_tls the cmake prefix
Maybe rename to mbedtls_cmake_prefix ?
Another reason it was called mbed_prefix is bc the user interfacing option, this variable holds the path for the custom mbed_tls. That's why the current name is not bad.
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.
I agree, I think medtls should be in the name. At some point in the future someone may need to also expose an option to set the path to zlib, which might be installed in a completely different location.
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.
yeah I understand , but I believe this would work for multiple prefixes:
--cmake-prefix "/usr/local/opt/mbedtls@3;/usr/local/opt/zlib"
I think we should also expose build flags.. at least in future PR..
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.
That might be better for a future PR. I can see a lot of future enhancements for this task, but we should get a minimal task that can build for all supported host platforms first.
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 makes the user interface task attached to cmake knowhow , if you are an Elixir user , you probably don't know how to build the cmake_prefix, I think we should simplify this with each lib option (AtomVM does not have infinite libs which link with cmake). Then internally we will make a function which takes all these options and builds the proper cmake_prefix.
This is also nice bc if AtomVM decides at some point to change the build system (for example, building with Zig compiler which maybe in some years is not something crazy), the user API for this task will remain more less the same.
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.
I think this API is better
mix esp32.build --mbedtls /path/mbedtls --zlib /path/zlib Than
mix --cmake-prefix /path/mbedtls;/path/zlibThere 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.
I think this API is better
mix esp32.build --mbedtls /path/mbedtls --zlib /path/zlib
Than
mix --cmake-prefix /path/mbedtls;/path/zlib
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, let's stick with that direction then..
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.
Thats fine, I think as long as there is a brief explanation of the option and its purpose in the help and docs it doesn’t matter much either way.
| end | ||
| end | ||
|
|
||
| defp copy_avm_libraries(atomvm_path) do |
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.
I'm pretty sure we don't want to populate avm_deps in the context of esp32 - eg. a "hello world" my_project yields a 1,3 Mb my_project.avm file with the avm_deps folder populated..
an empty avm_deps yields a 768 bytes my_project.avm file.. massive difference.
we may need this functionality for other platforms like stm32 or pico - this also highlights how we might want the atomvmlib building to be a separate mix task (or module at least) (esp32.build can still call it for identical functionality, but split code up..)
the purpose from this task is to make easy to test Elixir AtomVM with main branch code from the AtomVM repository for ESP32.
this task requires
idf.pyinstalled ordockerfor compiling the AtomVM from source.feel free to comment any issue in the implementation as well.
Thank you for your attention.
Workflow with custom build
Using ESP-IDF Docker container