conan-io / conan

Conan - The open-source C and C++ package manager
https://conan.io
MIT License
8.17k stars 974 forks source link

[bug] Gnu Triplet for profiles with custom os are wrong/should be able to be overriden from profile #8290

Open madebr opened 3 years ago

madebr commented 3 years ago

Hello,

I'm currently creating conan recipes for key packages for the Nintendo Switch homebrew toolchain. For this, I have added os=Nintendo Switch to my settings.yml.

The problem I'm facing is that conan uses os=Nintendo Switch for generating the gnu triplets. When building mpg123/1.26.4, the following arguments are passed to the autotools configure script.

...  '--datarootdir=${prefix}/share' --build=x86_64-linux-gnu --host=aarch64-nintendo switch

Besides the fact that the space in Nintendo Switch is not escaped, the triplet is unknown.

configure: WARNING: you should use --build, --host, --target
checking build system type... x86_64-pc-linux-gnu
checking host system type... Invalid configuration `aarch64-nintendo': system `nintendo' not recognized
configure: error: /bin/sh source_subfolder/build/config.sub aarch64-nintendo failed

It would be nice to have a way in the profile to override the host triplet. e.g. adding the following section to the host profile.

[overrides]
gnu-triplet=aarch64-none-elf

The used command to build mpg123 is:

conan install mpg123/1.26.4@ -pr:h nintendo-switch -pr:b default64 --build missing

The used host profile (nintendo-switch) is:

[settings]
os=Nintendo Switch
arch=armv8
compiler=gcc
compiler.version=10.2
compiler.libcxx=libstdc++11
build_type=Release
[build_requires]
devkitA64/38
[env]

The used build profile (default64) is:

[settings]
os=Linux
arch=x86_64
compiler=gcc
compiler.version=9
compiler.libcxx=libstdc++11
build_type=Release
[options]
[build_requires]
[env]
CC=gcc
CXX=g++
AS=gcc
ASM=gcc
CFLAGS=-m64
CXXFLAGS=-m64
ASFLAGS=-m64
ASMFLAGS=-m64
LDFLAGS=-m64

/cc @jgsogo as you're the goto guy for cross build affairs.

jgsogo commented 3 years ago

Hi, @madebr

Yes, we totally need a way to override it, because Conan cannot know about all the triplets and we cannot keep adding ifs for more and more OSs to generate those triplets (my opinion)... and we cannot test that all of them actually work in our CI.

I see two alternate paths: we could add a way to extend Conan, or we could externalize this logic to packages providing build-requires (same way as android-ndk uses CONAN_CMAKE_EXECUTABLE to inject some behavior before running CMake, this devkitA64/38 could use a CONAN_AUTOTOOL_EXECUTABLE to inject behavior and translate/generate the triplet).

The first approach is better, as it would work with system installed tools as well, but it requires some development and architectural design and decisions: probably something not to consider before Conan v2.0.

Alternatives with the environment variable to modify the executable or adding this new OS to settings.yml and Conan codebase can be done right now. Which one do you think can fit more scenarios?

cc/ @memsharded

madebr commented 3 years ago

Hello @jgsogo !

Yes, we totally need a way to override it, because Conan cannot know about all the triplets and we cannot keep adding ifs for more and more OSs to generate those triplets (my opinion)... and we cannot test that all of them actually work in our CI.

I totally agree. The number of operating systems (M) and number of packages (N) is M x N. It's untenable to have to update every recipe for every new operating system. So some abstraction would be nice.

Conan needs more hook(s) and override(s). I will open new issues as they arise, so we don't mix things.

I see two alternate paths: we could add a way to extend Conan, or we could externalize this logic to packages providing build-requires (same way as android-ndk uses CONAN_CMAKE_EXECUTABLE to inject some behavior before running CMake, this devkitA64/38 could use a CONAN_AUTOTOOL_EXECUTABLE to inject behavior and translate/generate the triplet).

I would compare the desired feature with CONAN_CMAKE_SYSTEM_PROCESSOR and CONAN_CMAKE_SYSTEM_NAME (conan documentation). The autotools executable is correct. It's just the --host=... (and maybe --build=... triplet that is troublesome.

Some care should be taken though: this requires access to the environment variables of the host profile. See https://github.com/conan-io/conan/issues/7737.

The first approach is better, as it would work with system installed tools as well, but it requires some development and architectural design and decisions: probably something not to consider before Conan v2.0.

This is clearly the better solution and I'm in no hurry :smile: .

Alternatives with the environment variable to modify the executable or adding this new OS to settings.yml and Conan codebase can be done right now. Which one do you think can fit more scenarios?

Don't add it to settings.yml. It won't magically fix my problems as it would require every recipe using autotools to add special handling for Nintendo Switch. I have also created a conan package for a ps2 toolchain, and it would quickly become unmaintainable if everybody came here to add his favorite (homebrew) console.

(https://github.com/conan-io/conan/issues/8261 is loosely related to this issue)

jgsogo commented 3 years ago

Yes, I would say that there are two possible paths: environment variables and Conan extensibility.


Environment variables.

We really want to get rid of environment variables because they make the build not reproducible (we are not going to capture them in the lockfiles), but given the current implementation of the Android NDK and some requirements from other packages that provide toolchains we need a way to inject/modify things in our calls to the build-system.

Using specific-purpose variables like CONAN_CMAKE_SYSTEM_PROCESSOR, CONAN_CMAKE_SYSTEM_NAME,... is not scalable, we would need a new variable for each necessity... but only one like CONAN_CMAKE_EXECUTABLE can fit most of the needs right now (exactly the same reasoning for Autotools or any other tool used by Conan):


Implementing some level of extensibility or pluggable behavior to Conan: IMHO this is the way to go and the way we should add support to all compilers. Find here a link to different (standard) ways of extending a Python package.

madebr commented 3 years ago

Thanks for explaining me why there is desire to move away from the conan environment variables.

As autotools are not executables, but shell scripts, a CONAN_AUTOTOOLS_EXECUTABLE argument is not wanted. CMake is very standardized, autotools is very diverse. Not every configure script is created by GNU autotools. So always adding --host=... would make some packages fail. AutotoolsBuildEnvironment should remain in control of adding arguments to the shell script.

Implementing some level of extensibility or pluggable behavior to Conan: IMHO this is the way to go and the way we should add support to all compilers. Find here a link to different (standard) ways of extending a Python package.

There is indeed a lot of similarity to my desire of adding profile specific hooks and supporting pypi packages. The only difference is the location: build_requirement vs pypi package.

jgsogo commented 3 years ago

There is some work in the context of the new toolchains that can add some light to this issue:

These are ongoing efforts, but probably this issue fits there. Plugins can be overkill for this feature.

We need to wait, we need to see how the toolchains evolve and how we can pass some key-value pairs with config values to them.

madebr commented 3 years ago

I think this problem is similar to my problem with the meson toolchain #8312. It would be nice to override values from profile/build_requirement.

It would be nice to have a framework to do this uniformly, such that not every toolchain must re-implement this.

jgsogo commented 3 years ago

The new proposed conf mechanism (https://github.com/conan-io/conan/pull/8266) can be enough to do this kind of overrides... assuming that the build-helper/toolchain takes its value into account. We cannot go crazy and start reading everything from outside, but it should be flexible enough: the Nintendo Switch setting and the override will be contained in the same profile file 🎉

If merged, we should consider this issue a feature for some of the toolchains

madebr commented 3 years ago

Sounds like a good idea. I like the paper trail aspect of it instead of the magical environment variables.

madebr commented 3 years ago

Half-yearly reminder that this feature-request/bug exists :wink: .

I just checked the new conan.tools.gnu.AutoToolsToolchain and it has the same problem: https://github.com/conan-io/conan/blob/c3fc425871149b12a1d12153bed90a06a3d20132/conan/tools/gnu/autotoolstoolchain.py#L49-L52