Skip to content
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

Library dependency resolution relies on lib_deps=... order #3598

Closed
mcspr opened this issue Jul 16, 2020 · 9 comments
Closed

Library dependency resolution relies on lib_deps=... order #3598

mcspr opened this issue Jul 16, 2020 · 9 comments
Assignees
Labels
bug ldf Library Dependency Finder
Milestone

Comments

@mcspr
Copy link
Contributor

mcspr commented Jul 16, 2020

Configuration

Operating system:

Fedora Server 32

PlatformIO Version (platformio --version):

PlatformIO, version 4.4.0a5 (a2efd7f)

Description of problem

Library specified in lib_deps, but also specified in other dependent library's library.json requirements gets detected twice after scanning for dependencies and also gets linked twice.

Project in question uses ESPAsyncTCP and async-mqtt-client. Depending on which one is specified first in the lib_deps, different versions of ESPAsyncTCP are installed.

Compilation issue comes from ESPAsyncTCP having slightly different signatures of onError(), where int8_t type is replaced with err_t. Main application uses the old version, while async-mqtt-client uses the 2nd b/c of the initial dependency scan, so linking with both libraries isn't possible.

Steps to Reproduce

With project structure as described below

  1. pio run

Actual Results

See https://github.com/mcspr/pio-poc-libdeps-order/runs/875651792?check_suite_focus=true#step:5:1 (5th step, "Run without installing libs manually")

When ESPAsyncTCP comes before async-mqtt-client, it is installed first and async-mqtt-client dependency gets satisfied.
When async-mqtt-client comes before, PIO registry version gets installed first and lib_deps version second.

Having two libraries at the same time causes build process to try to include both at the same time.

Expected Results

(?) Not really sure about this one. Based on previous version of PIO, it worked fine with the stable release

If problems with PlatformIO Build System:

The content of platformio.ini:

[env]
platform = espressif8266
board = d1_mini
framework = arduino

[env:ok]
lib_deps =
    https://github.com/me-no-dev/ESPAsyncTCP.git#7e9ed22
    https://github.com/marvinroger/async-mqtt-client#v0.8.1

[env:fail]
lib_deps =
    https://github.com/marvinroger/async-mqtt-client#v0.8.1
    https://github.com/me-no-dev/ESPAsyncTCP.git#7e9ed22

Source file to reproduce issue:

// main.cpp
#include "main.h"

#include <cstdint>

AsyncMqttClient mqtt;
AsyncClient client;

void setup() {

    Serial.begin(115200);

    WiFi.begin("SSID", "PASS");

    mqtt.connect();
    mqtt.publish("test", 0, false, "test");

    // <<<
    // it is important that we try to use incompatible function from a version different from the one that gets linked with mqtt client
    client.onError([](void*, AsyncClient* c, int8_t err) {
        Serial.printf(PSTR("Error %s (%d) on client %p\n"), c->errorToString(err), err, c);
    });
    // >>>
    client.connect("foo.bar", 80);

}

void loop() {
}
// main.h
#pragma once

#include <Arduino.h>
#include <ESP8266WiFi.h>
#include <Hash.h>
#include <ESPAsyncTCP.h>
#include <AsyncMqttClient.h>

Additional info

  • not sure if header or cpp matters when dealing with ldf, but keeping deps in header for the sake of the experiment
  • installer.py is a helper script that was assumed as the culprit initially, but as it turns out issue is easily reproducible even without it
@mcspr
Copy link
Contributor Author

mcspr commented Jul 16, 2020

Seems to be related to the #2115

mcspr added a commit to mcspr/espurna that referenced this issue Jul 16, 2020
mcspr added a commit to xoseperez/espurna that referenced this issue Jul 16, 2020
@ivankravets ivankravets added the ldf Library Dependency Finder label Aug 23, 2020
@ivankravets ivankravets added this to the 4.4.1 milestone Aug 23, 2020
@ivankravets ivankravets removed this from the 5.0.1 milestone Sep 10, 2020
@ivankravets
Copy link
Member

The library order is matters. LDF firstly adds to the build stage all deps which you manually declared and in exact order.

Could you try the latest 5.0.1-dev? I tried to reproduce this issue and both envs produce the same error. See screenshot:

Screen Shot 2020-09-10 at 12 49 31

mcspr added a commit to mcspr/pio-poc-libdeps-order that referenced this issue Sep 10, 2020
@mcspr
Copy link
Contributor Author

mcspr commented Sep 10, 2020

I pushed another build, see https://github.com/mcspr/pio-poc-libdeps-order/runs/1095869624

pio system info
rm -rf .pio/build
rm -rf .pio/libdeps
pio run -e $1

ok env works, fail does not

As a sidenote about the screenshot error, is there a conflict with main.h ? All library includes happen there, perhaps it does not pick up src/main.h? (easily testable by inserting #warning "foo" which will show stderr message if it is really included)

@ivankravets
Copy link
Member

Could you reproduce this issue with the latest development branch?

@ivankravets ivankravets modified the milestones: Backlog, 5.3.0 Apr 2, 2022
@mcspr
Copy link
Contributor Author

mcspr commented Apr 5, 2022

env:fail still breaks when running the CI workflow from the repo I created. it runs pio upgrade --dev, so it is the latest version published
https://github.com/mcspr/pio-poc-libdeps-order/runs/5806038304?check_suite_focus=true
(.zip'ed logs for posterity)

@ivankravets
Copy link
Member

Is this is a bug of ESP8266 integration? PlatformIO Core processes dependencies in order as you declare them in lib_deps. I don't see any issues with PlatformIO Core. See https://github.com/platformio/platformio-core/blob/develop/platformio/builder/tools/piolib.py#L952

@mcspr
Copy link
Contributor Author

mcspr commented Apr 7, 2022

edit: My proposal was to remove the confusion in the builder and override the library.json dependency with a matching name with the one in lib_deps. If we have both libraries at the same time, we can't guarantee things will work.

There are different types in the resulting .o files. In one header it is declared as u8, in the other it is int (aka what C & C++ think simple enums are). Depending on which header files the library or user code used to build, it would think of different types, resulting in the link issue when we use the old library instead of the newest one.

It is also technically a versioning issue with the ESPAsyncTCP, it should've bumped itself to 1.3.0 to avoid this breaking change :/

@ivankravets ivankravets self-assigned this Apr 8, 2022
@ivankravets
Copy link
Member

Thanks for the report! Please re-test with pio upgrade --dev.

@mcspr
Copy link
Contributor Author

mcspr commented Apr 11, 2022

Thanks for the report! Please re-test with pio upgrade --dev.

PoC repo works now, thanks!
https://github.com/mcspr/pio-poc-libdeps-order/runs/5971294721?check_suite_focus=true

Dependency Graph
|-- AsyncMqttClient @ 0.8.1+sha.ddbf4d1
|   |-- ESPAsyncTCP @ 1.2.0+sha.7e9ed22
|-- ESPAsyncTCP @ 1.2.0+sha.7e9ed22
|-- ESP8266WiFi @ 1.0
|-- Hash @ 1.0

This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug ldf Library Dependency Finder
Projects
None yet
Development

No branches or pull requests

2 participants