keyboardio / Kaleidoscope-Bundle-Keyboardio

A Kaleidoscope distribution for the Keyboardio Model 01 and other keyboards.
Other
17 stars 24 forks source link

Fix link order problems with inter-library/inter-plugin dependencies #17

Closed noseglasses closed 5 years ago

noseglasses commented 5 years ago

This is a second attempt to fix Arduino's build system in a way that prevents library link/include order problems (a first unsuccessfull attempt was https://github.com/keyboardio/Kaleidoscope-Bundle-Keyboardio/pull/15).

It is submitted as draft PR as it relies on https://github.com/keyboardio/Kaleidoscope-Build-Tools/pull/6 that would need to be merged before this PR could be merged.

Moreover, even though this PR is expected to be fully portable by using Java to delete a file during the Arduino build system execution, this feature is up to now only tested under Ubuntu Linux. It would be nice if Mac and Windows users (@gedankenexperimenter, @obra ?) could give it a try on their build platforms to confirm that the proposed changes work on all major platforms supported by Kaleidoscope.

This PR has been confirmed to work with kaleidoscope-builder (command-line builds) on the following OS.


Before this change, when Arduino was linking the elf binary, it added all objects and archives (libraries/plugins) to a long linker command line. This would sometimes cause issues due to the linker's gargabe collection feature. When one library A referenced a symbol from a library B but library A appeared after B in the linker command line, the referenced symbol was in some cases garbage collected as to the linker it appeared to be unreferenced.

To understand this better it is important to know that the linker processes objects and archives one by one in the order of their appearance at the command line. For every object or archive encountered it runs garbage collection individually. That's why symbols might get discarded although an object or archives that is encountered later on while processing the linker command line might have referenced it.

To fix this problem this commit divides up the linker process into two steps.

First, all objects and archives are combined in a large .a archive. Then, this archive is passed to the original linker command line, thereby replacing the original set of objects and archives.

The final link now sees all symbols together. Thus, it is able to determine the true interdependencies and do a proper garbage collection without accidentally generating unresolved symbols.

With this fix, the order of appearance of include directives in the sketch file that formerly had an influence on the link order (as Arduino passes library archives to the linker in the order of the appearance of the matching includes in the sketch), can now be savely chosen arbitrarily.

This commit switches to a version of submodule build-tools that comes with a small java program that allows to portably remove the joined archive file that is used as a temporary build artifact.

Signed-off-by: Florian Fleissner florian.fleissner@inpartik.de

gedankenexperimenter commented 5 years ago

I don't have any Windows machines, but I'll give it a try on macOS sometime today.

noseglasses commented 5 years ago

Thanks, @gedankenexperimenter. I naively assumed that Arduino is self-contained, including Java on all major platforms. Too bad :-( Now I will see if there's any chance that the Arduino build system exposes the java command as some sort of variable. If not, I am at the end of my wits.

noseglasses commented 5 years ago

@gedankenexperimenter, just for my understanding. Does that mean that Arduino when being installed on macOS requires an additional Java package to be installed. Or does the system come with a pre-installed Java?

noseglasses commented 5 years ago

I just found that Arduino supports property overrides for specific OS. This would probably allow us to define the Java path and executable name for each platform individually.

@gedankenexperimenter, is there some sort of default path to Java on macOS? Or can it be found via the PATH variable? The latter would mean that we could invoke Java on macOS without an absolute path.

noseglasses commented 5 years ago

Just checked the approach using Arduino's OS specific property overrides. it works nicely and would solve our problem on Linux and Windows.

gedankenexperimenter commented 5 years ago

Java does not come installed on macOS. I think it's probably bundled into the Arduino executable. But we shouldn't need Java with platform-specfic property overrides; we can just use rm instead (and del on Windows?).

noseglasses commented 5 years ago

Java does not come installed on macOS. I think it's probably bundled into the Arduino executable. But we shouldn't need Java with platform-specfic property overrides; we can just use rm instead (and del on Windows?).

Damn right! I added the Java based solution before I stumbled upon the platform specific tools option. Will fix it in a minute...

noseglasses commented 5 years ago

Java does not come installed on macOS. I think it's probably bundled into the Arduino executable.

it seems to rely on a Java that does not come with an executable but with a dylib (libjava.dylib). Thus, calling Java for other purposes e.g. to enhance the crappy Arduino-build system is not easily possible.

noseglasses commented 5 years ago

Allright, using rm and del things become much simpler. @gedankenexperimenter, would you please do me the favor and give it another try?

gedankenexperimenter commented 5 years ago

I can confirm that this works on macOS now.

gedankenexperimenter commented 5 years ago

I can also confirm that this does not trigger #16.

noseglasses commented 5 years ago

i found a solution for Windows that uses powershell to remove the temporary joined archive file.

Unfortunately I am unable to test this in the Windows Arduino GUi. With MSys and kaleidoscope-builder it works fine.

noseglasses commented 5 years ago

IMHO, as the Arduino-GUI uses arduino-builder under the hood as its build tool, this is supposed to work as well with the GUI.

obra commented 5 years ago

Awesome. I'm good to try this and see how folks do.