arduino / arduino-builder

A command line tool for compiling Arduino sketches
GNU General Public License v2.0
458 stars 114 forks source link

"Improve precompiled libraries handling" broke "mixed code" libraries #353

Closed sierraatomic closed 4 years ago

sierraatomic commented 4 years ago

When you have library with both precompiled and source code (precompiled!=source code), Arduino v1.8.12 will skip compilation phase, and you will get missing code error at linker phase. If you change "precompiled=true" to "precompiled=false" it will compile the code, but will not add precompiled code to linker, and you will get missing code error (different code missing). Example: https://github.com/BoschSensortec/BSEC-Arduino-library It is compiling just fine on Arduino v1.8.11. Looks like the reason for this behaviour is from: General change:

facchinm commented 4 years ago

Hi @sierraatomic , I totally ignored there were usecases like the repo you linked "in the wild" , sorry for that. I can see two possible solutions:

  1. precompiled=true could mean that the library is binary only + headers. ldflags=... should then be applied in any case (also if precompiled=false) to allow mixed (precompiled != source) libraries. This needs a (tiny) patch in the builder but would make IDE 1.8.12 incompatible with such libraries forever.
  2. leave the builder as is and change the specifications to only allow precompiled libraries if they are completely binary + headers ( + same sources). The BSEC library, for example, could be turned into 2 libraries, one precompiled and the other source only, depending upon each other via depends= specification (https://github.com/arduino/Arduino/wiki/Arduino-IDE-1.5:-Library-specification#libraryproperties-file-format)

@cmaglie any though on this?

matthijskooijman commented 4 years ago

@facchinm, IIUC option #1 would mean that you always include binary files in the link, and precompiled=no means also including source files?

I guess this all depends on the question whether there exists a usecase where you would ship sources files and a binary that was compiled from those same source files (as opposed to a binary that is complementary to the source files). My initial thought would be that this usecase does not actually exist. I can imagine shipping a binary might be needed for a closed-source component, but then you won't ship sources, obviously. If you can supply the source files for a library, then there is no need for the binary, you can just compile the sources. The only thing could be that you ship a binary as an optimization, but I suspect that it would be way better to just handle that using the caching we already have, which can already handle recompilation on changes automatically (and if you really want to supply a precompiled library for speed, you can just distribute a second copy of the library with just the precompiled files). Or am I missing a usecase now?

As for ldflags applying those always (regardless of whether there is a binary component) probably leads to least surprise. Also, I think there is no reason why something like that would be needed only for precompiled binaries, source files might just as well depend on a system library (though typically there are not so many system libraries in an Arduino environment).

LowPowerLab commented 4 years ago

@facchinm Just found this already open issue after realizing that 1.8.12 broke precompiled libraries that contain mixed code and binaries. Especially interesting find after going through the readme to see that this version "Improve precompiled libraries handling" (-:

I have a library that I am making, it is a wrapper around a closed source library, I need to add my own code to the sources. So I need to be able to ship a library that contains both my code wrappers and the closed source binaries.

For now I am forced to go back to 1.8.11. But would be nice to have this resolved. Thanks

facchinm commented 4 years ago

Hi @LowPowerLab , sorry for the inconvenience :disappointed: Can you link your library so we can add it to the CI testing to avoid this kind of problem next time? Thanks

cmaglie commented 4 years ago

The only thing could be that you ship a binary as an optimization, but I suspect that it would be way better to just handle that using the caching we already have, which can already handle recompilation on changes automatically (and if you really want to supply a precompiled library for speed, you can just distribute a second copy of the library with just the precompiled files). Or am I missing a usecase now?

Some libraries takes a very long time to compile, so long that even once per session begins to be frustrating, basically the precompiled feature has been added for those, we didn't think about the other use-case (adding an open source wrapper around a closed source lib and re-release the whole thing as a new library).

Personally I like the option 2: releasing the closed source lib as a separate lib (so it will be possibly reused by someone else?)

BTW I see the pragmaticity of the option 1, so for me it's ok to do it (and make IDE 1.8.12 incompatible with those few libraries, I guess people will survive this).

LowPowerLab commented 4 years ago

Hi @facchinm thanks for your response. We would certainly prefer to have full code but the core library is closed source by manufacturer with very little documentation. Unfortunately right now the wrapper library is still in ongoing development for the foreseeable future and not really ready to be released to public. I wanted to be able to have the end user update from an older IDE (1.8.7) to the latest one, but this broke the library, so 1.8.11 is now required instead to continue development. I can appreciate the ability to have support for library dependencies but the problem in my case with having 2 libraries (and I imagine this will be the same for others), is going to add more support, and customers will not really like having 2 libraries for the same functionality. I have to explain to them why it worked previously and now it doesn't and they need yet another library. Do you see what I mean?

If #1 option you mentioned (if I understand correctly) to use precompiled=false but with ldflags to link the .a/.so libraries and allow the mixed library, that would also be OK. My main goal is to allow the whole content in 1 library and not have to split it. You mentioned this would might require a tiny patch to work in 1.8.12 - I can build Arduino 1.8.12 myself to release to end user until patched version is released - is that patch something you could share so I can solve this now?

facchinm commented 4 years ago

@LowPowerLab patch submitted https://github.com/arduino/arduino-cli/pull/611 . If you could test with your library I'd be very grateful.

To compile the builder the easiest way right now is

git clone git@github.com:arduino/arduino-cli.git
# cd arduino-cli && checkout the right commit
git clone git@github.com:arduino/arduino-builder.git
cd arduino-builder
# add "replace github.com/arduino/arduino-cli => ../arduino-cli" (no quotes) before "require (" in go.mod
go build

or I can provide binaries if you tell me your OS

LowPowerLab commented 4 years ago

@facchinm Thanks, I am not sure when I can get to it and figure out how to make the builder, my OS is W10. From your patch comments, a mixed library (precompiled & wrapper/additional code) should be defined without precompiled and by using ldflags?

facchinm commented 4 years ago

@LowPowerLab exactly. In attachment the builder to replace the official one arduino-builder.zip Let me know if it works!

LowPowerLab commented 4 years ago

@facchinm Hmmm, maybe I'm doing something wrong. I tried 1.8.12 and also latest 1.8.13 build, replaced the arduino-builder.exe with your patched version, removed the precompiled=true in my library.properties, and left the ldflags as before pointing to the precompiled libs. I get the same compile errors as before:

undefined reference to '___function_in_precompiled_lib'

facchinm commented 4 years ago

@LowPowerLab may I get a preview of the library so I can test it myself? Feel free to mail it to m.facchin AT arduino.cc if you don't want to release publicly.