Conversation
|
Minimum app.c supporting code: |
|
In my opinion this looks a little bit too specific to one particular platform at the moment (Xtensa), while there are many other possible platforms. For example, Linux. The design will likely need significant changes to work more universally. |
It's more universal than you might think, though. I first developed c-archive to build a static lib for macOS, I confirmed it worked by linking that .a to a test.c program. My real target is esp32 which is why there's xtensa-specific workarounds in there. But for Linux I think it would mostly "just-work". |
aykevl
left a comment
There was a problem hiding this comment.
In my opinion this is a bit too messy to really review and form an opinion on. See the comments below.
Some things I definitely want to see improved before reviewing again:
- It needs tests. There currently are none. Without tests, it will break eventually. In particular, it needs tests to show CGo still works correctly (or alternatively CGo support needs to be disabled - this might be difficult since we rely on CGo for a bunch of features).
- It should not build the C library and should not add C header files to the C include path, since in that case we'll be mixing two C libraries which will break in "interesting" ways.
About this, the reason I still build the libc but don't link it, is because my CGo modules won't compile unless it can find the header files. And in the case of xtensa, the libc is picolibc, which is also available in esp-idf. So the same libc can be used. |
Not necessarily. There are many configuration knobs when building a libc. Are you sure they're all the same, and won't cause any problems later due to ABI differences? The reason why we don't use external C libraries is exactly this reason. Every system is different, and it's a huge hassle to figure out the correct header paths. (We used to use them in the past, but over time I've removed all of those - solving a lot of platform specific issues in the process). If you want to use an external C library, you need to pass the right include flags in CGO_CFLAGS and such (not sure if they're supported right now in TinyGo, but that would be the correct way to do it and wouldn't be very difficult to implement). |
I understand the nuances here. But my CGo code is running without issues. The calling convention/ABI is the same. ESP-IDF even switched to picolibc by default in v6.0, but newlib and picolib and very similar anyway. Anyway my CGo code isn't calling any exotic C functions, its just using C standard library. |
|
I want to keep moving forward in the right direction with this. My current thinking is that it would be a good idea and sufficient to limit the scope of this change to the esp32 family of boards, for now. I have tested this buildmode with a real Go module, that itself uses CGo, on the esp32s3 (xtensa) and esp32c3 (riscv32). Both working as expected, flawlessly. This shows: using the builtin libc headers to build the CGo dependencies does not cause any issues. We still build libc to get the headers, we simply avoid linking the static library at the end. I am not sure what tests are needed that you wanted - would you mind giving a hint of what you would code as a test? I think it's worth looking at this PR again because a lot has been changed & simplified. I think this PR is a lot closer to being ready now - it's moved a huge step in the right direction. Please take another look. EDIT: latest commit removes libc from the dependencies, so ignore what I wrote about libc. |
|
In fact, you will notice that the changes to the builder proper are very tiny now. The majority of the changes are just separating what needs to be built depending on whether buildmode.c-archive is active or not. |
|
Okay, I regrouped, took the advice of implementing CGO_CFLAGS, and I can now set the headers include directory. I could therefore disable building the libc dependency. With this, it has to be perfect now (at least for the limited scope of esp32 boards). Please re-review. |
…avoid dangerous relocation error (branch target out of range).
|
#5334 is an alternative PR for building static libraries for ESP-IDF. If that one is merged I will close this one. |
This is - not fully polished - but is opened here in the hope that suggestions and improvements can be made. Fixes #254
It does work - pretty well actually - for Xtensa. There, a component can be made with a simple: