Skip to content

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

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

Closed
sierraatomic opened this issue Feb 19, 2020 · 11 comments · Fixed by arduino/arduino-cli#611
Closed
Labels
conclusion: resolved Issue was resolved topic: code Related to content of the project itself type: imperfection Perceived defect in any part of project

Comments

@sierraatomic
Copy link

sierraatomic commented Feb 19, 2020

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:

  • library compilation bails out if the precompiled object is found.
@facchinm
Copy link
Member

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
Copy link
Collaborator

@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
Copy link

@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
Copy link
Member

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

@cmaglie
Copy link
Member

cmaglie commented Feb 25, 2020

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
Copy link

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 added a commit to facchinm/arduino-cli that referenced this issue Mar 10, 2020
This patch restores some functionalities broken by arduino#512 .
It introduces a BREAKING CHANGE to "precompiled" field

If merged, "precompiled" means that the library ONLY contains a precompiled library and its headers.
No file containing implementation will be compiled if the library is specified as such.

Fixes arduino/arduino-builder#353

Tested with
* arduino#512 (comment) (Nano33BLE and generic MKR board)
* https://github.com/BoschSensortec/BSEC-Arduino-library (works after removing precompiled=true directive in library.properties)
* https://github.com/vidor-libraries/USBBlaster (MKRVidor, fully precompiled)
@facchinm
Copy link
Member

@LowPowerLab patch submitted arduino/arduino-cli#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 [email protected]:arduino/arduino-cli.git
# cd arduino-cli && checkout the right commit
git clone [email protected]: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
Copy link

@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
Copy link
Member

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

@LowPowerLab
Copy link

@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
Copy link
Member

@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.

facchinm added a commit to facchinm/arduino-cli that referenced this issue Mar 24, 2020
This patch restores some functionalities broken by arduino#512 .
It introduces a BREAKING CHANGE to "precompiled" field

If merged, "precompiled" means that the library ONLY contains a precompiled library and its headers.
No file containing implementation will be compiled if the library is specified as such.

Fixes arduino/arduino-builder#353

Tested with
* arduino#512 (comment) (Nano33BLE and generic MKR board)
* https://github.com/BoschSensortec/BSEC-Arduino-library (works after removing precompiled=true directive in library.properties)
* https://github.com/vidor-libraries/USBBlaster (MKRVidor, fully precompiled)
facchinm added a commit to facchinm/arduino-cli that referenced this issue Apr 20, 2020
This patch restores some functionalities broken by arduino#512 .
It introduces a BREAKING CHANGE to "precompiled" field

If merged, "precompiled" means that the library ONLY contains a precompiled library and its headers.
No file containing implementation will be compiled if the library is specified as such.

Fixes arduino/arduino-builder#353

Tested with
* arduino#512 (comment) (Nano33BLE and generic MKR board)
* https://github.com/BoschSensortec/BSEC-Arduino-library (works after removing precompiled=true directive in library.properties)
* https://github.com/vidor-libraries/USBBlaster (MKRVidor, fully precompiled)
cmaglie pushed a commit to facchinm/arduino-cli that referenced this issue May 4, 2020
This patch restores some functionalities broken by arduino#512 .
It introduces a BREAKING CHANGE to "precompiled" field

If merged, "precompiled" means that the library ONLY contains a precompiled library and its headers.
No file containing implementation will be compiled if the library is specified as such.

Fixes arduino/arduino-builder#353

Tested with
* arduino#512 (comment) (Nano33BLE and generic MKR board)
* https://github.com/BoschSensortec/BSEC-Arduino-library (works after removing precompiled=true directive in library.properties)
* https://github.com/vidor-libraries/USBBlaster (MKRVidor, fully precompiled)
cmaglie pushed a commit to facchinm/arduino-cli that referenced this issue May 4, 2020
This patch restores some functionalities broken by arduino#512 .
It introduces a BREAKING CHANGE to "precompiled" field

If merged, "precompiled" means that the library ONLY contains a precompiled library and its headers.
No file containing implementation will be compiled if the library is specified as such.

Fixes arduino/arduino-builder#353

Tested with
* arduino#512 (comment) (Nano33BLE and generic MKR board)
* https://github.com/BoschSensortec/BSEC-Arduino-library (works after removing precompiled=true directive in library.properties)
* https://github.com/vidor-libraries/USBBlaster (MKRVidor, fully precompiled)
cmaglie added a commit to arduino/arduino-cli that referenced this issue May 8, 2020
* Fix mixed code precompiled libraries

This patch restores some functionalities broken by #512 .
It introduces a BREAKING CHANGE to "precompiled" field

If merged, "precompiled" means that the library ONLY contains a precompiled library and its headers.
No file containing implementation will be compiled if the library is specified as such.

Fixes arduino/arduino-builder#353

Tested with
* #512 (comment) (Nano33BLE and generic MKR board)
* https://github.com/BoschSensortec/BSEC-Arduino-library (works after removing precompiled=true directive in library.properties)
* https://github.com/vidor-libraries/USBBlaster (MKRVidor, fully precompiled)

* Added test-build for Arduino_TensorFlowLite

* Added another test for Bosch Sensor lib

* Fallback search for precompiled libraries in non-fpu specific directories

* variable renamed

* Moved fixLDFLAG inside compileLibraries

* Inlined FixLDflags

* Using more paths helpers to simplify code

* Added support for "precompiled=full" in library.properties

This flag allow the deployment of a pure precompiled library that ships
also the source code of the precompiled part.
This allows to possibly compile-from-source as a fallback if the library
does not provide a precompiled build for the current architecture.

The "precompiled=true" instead has been ported back to the old behaviour
(for libraries that ships a precompiled binary and support source code
that wraps/uses the precompiled part).

* Updated tests

* Fixed test compile-build on very long paths on Windows

* Removed constants.BUILD_PROPERTIES_COMPILER_LIBRARIES_LDFLAGS

* Add space before "-L" gcc flag to allow multiple precompiled libs to be included

Co-authored-by: Cristian Maglie <[email protected]>
@rsora rsora added regression type: imperfection Perceived defect in any part of project labels Sep 22, 2021
@per1234 per1234 added conclusion: resolved Issue was resolved topic: code Related to content of the project itself labels Sep 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
conclusion: resolved Issue was resolved topic: code Related to content of the project itself type: imperfection Perceived defect in any part of project
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants