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

[vcpkg_configure_make] Set the --host parameter in case of Linux #22331

Merged
merged 1 commit into from
Feb 9, 2022

Conversation

daschuer
Copy link
Contributor

@daschuer daschuer commented Jan 4, 2022

This allows to crosscompile on Linux for all autotools ports.
It set the tool prefix determined by the gcc path, via th --host parameter.

  • What does your PR fix?

Fixes building libgcrypt[core]:arm-linux and libgpg-error[core]:arm-linux on an x64 build machine.
Without this patch the x64 version of the libs is installed int the arm-linux folder

  • Which triplets are supported/not supported? Have you updated the CI baseline?

No changed

  • Does your PR follow the maintainer guide?

    Yes

  • If you have added/updated a port: Have you run ./vcpkg x-add-version --all and committed the result?

@JackBoosY JackBoosY self-assigned this Jan 4, 2022
@JackBoosY JackBoosY added the category:tool-update The issue is with build tool or build script, which requires update or should be executed correctly label Jan 4, 2022
@JackBoosY
Copy link
Contributor

cc @Neumann-A

@dg0yt
Copy link
Contributor

dg0yt commented Jan 4, 2022

I see that this is modeled after the macOS section. But it seems to me what is really needed is not compiler name matching but checking VCPKG_CROSSCOMPILING.

@daschuer
Copy link
Contributor Author

daschuer commented Jan 4, 2022

VCPKG_CROSSCOMPILING is set if the host and the target triplet are different.
In case you use a custom compiler for host AND target, we also want the custom compiler passed to the autotools ports. So I think the current solution is correct.

Let's keep this patch minimal and not dive into a refactoring of the whole vcpkg_configure_make.

@dg0yt
Copy link
Contributor

dg0yt commented Jan 4, 2022

It is already hard to refactor because of too many minimal patches...

@daschuer
Copy link
Contributor Author

daschuer commented Jan 4, 2022

What is your suggestion?

@daschuer
Copy link
Contributor Author

daschuer commented Jan 9, 2022

@dg0yt The current solution is IMHO good. What do you wish to have improved?

@JackBoosY
Copy link
Contributor

Ping @dg0yt

@dg0yt
Copy link
Contributor

dg0yt commented Jan 10, 2022

TBH I gave up on expecting a clear line in the development of vcpkg_configure_make and didn't want to comment any further. Better commit as is than waste of CI time.

But again:

  • I expect cross compiling to be enabled when VCPKG_CROSSCOMPILING is true.
  • I expect autotools-host triplet determination to be a separate function, not to be part of the main spaghetti code.

@Neumann-A
Copy link
Contributor

I think the first thing to do here is setup the env variables for programs like CC and stuff for linux/osx (just like windows does).
After that the actual value for host might be set (it probably can be kind of arbitrary then since it only controls weather or not configure assumes a cross build or not. )
The question here is only how CC_FOR_BUILD should be set? (in vcpkg terms it needs to be deactivated.)

Copy link
Contributor

@strega-nil-ms strega-nil-ms left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, does this work when you're using clang?

scripts/cmake/vcpkg_configure_make.cmake Outdated Show resolved Hide resolved
@daschuer
Copy link
Contributor Author

Also, does this work when you're using clang?

No, in clang you control the compilation target by command line parameters and not by a prefix:

clang --target=arm-linux-gnueabihf

@JackBoosY
Copy link
Contributor

Depends on #22534

@daschuer
Copy link
Contributor Author

Rebased to include #22534

I consider this as done.

In the current state this is a fix for a pending issue that effects me, mirroring the solution from other build systems. The discussed changes of the whole solution for all targets towards a streamlined conception is out of scope of this PR.

# Linux - cross-compiling support
if(VCPKG_TARGET_IS_LINUX)
if (requires_autoconfig AND NOT arg_BUILD_TRIPLET OR arg_DETERMINE_BUILD_TRIPLET)
if(VCPKG_DETECTED_CMAKE_C_COMPILER MATCHES "([^\/]*)-gcc$" AND CMAKE_MATCH_1)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a comment explaining what this regex is doing?

  • Why is it specific to gcc?
  • Why we know that prefix is going to be the right value for --host (compare to elsewhere in this file where we use triplet-derived variables)
  • Is there any reason we shouldn't just always set --host?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • can we derive the --host parameter from VCPKG_TARGET_ARCHITECTURE, somewhat like we do in the macOS case?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment is in place.
Using VCPKG_TARGET_ARCHITECTURE alone does not work, because the ABI setting is not known here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

alright, thanks @daschuer; I'm happy with it then, but I want to discuss it at the meeting with everyone.

@JackBoosY
Copy link
Contributor

Ping for response.

@strega-nil-ms strega-nil-ms added info:reviewed Pull Request changes follow basic guidelines requires:discussion labels Feb 4, 2022
@Neumann-A
Copy link
Contributor

Note:
This is not the correct fix. The correct fix is to set CC etc. explicitly instead of having configure figure it out! (This is currently only done on windows!).
--host should always be set if VCPKG_CROSSCOMPILING is true! (It can be set to arbitrary values for that if CC is explicitly set.)

@JackBoosY
Copy link
Contributor

I agree with @Neumann-A

@daschuer
Copy link
Contributor Author

daschuer commented Feb 5, 2022

I am not sure if the outlined solution is more "correct", because it pretends to be a native build on a foreign host. At least it is explicitly mentioned as an anti-pattern in the autoconf manual:
https://www.gnu.org/software/autoconf/manual/autoconf-2.65/html_node/Specifying-Target-Triplets.html

Note that if you do not specify --host, configure fails if it can't run the code generated by the specified compiler. For example, configuring as follows fails:
./configure CC=m68k-coff-gcc

For now I think this PR is in a merge able state, tested on CI and solving an actual issue. I have no interest to hack around the recommendations of the manual. So I suggest to merge this as it is and postpone the suggested changes to another PR form another contributor.

@Neumann-A
Copy link
Contributor

because it pretends to be a native build on a foreign host

Your logic is inverted. That is only the case if the user lied to vcpkg and said host-triplet=target-triplet. As said --host should be passed depending on VCPKG_CROSSCOMPILING.

For now I think this PR is in a merge able state, tested on CI and solving an actual issue

Disagree. The actual issue is that vcpkg_configure_make is not setting up the compiler from the cmake toolchain for !windows. Having configure figuring it out by a magic prefix is just a hack.

@daschuer
Copy link
Contributor Author

daschuer commented Feb 6, 2022

Not using the host compiler without a prefix is a kind of crosscompile for autotools. In addition, autotools do not fully support changing the compiler via command line options. It requires to edit the files. For instance you cannot use clang, see:
https://stackoverflow.com/questions/37644113/how-to-compile-project-with-clang-and-the-option-std-c11-using-autotools
This is, why all our targets win/osx/linux are using the gcc compiler!

In configure you find this code:

ac_tool_prefix=
test -n "$host_alias" && ac_tool_prefix=$host_alias-

so you see, that what you consider as hack is just the counterpart of generating the ac_tool_prefix from cmake, which is highly desired.

You need to acknowledge that there is a bunch of tools autotools makes use of with this prefix:

arm-linux-gnueabihf-addr2line     arm-linux-gnueabihf-gcov
arm-linux-gnueabihf-ar            arm-linux-gnueabihf-gcov-9
arm-linux-gnueabihf-as            arm-linux-gnueabihf-gcov-dump
arm-linux-gnueabihf-c++filt      
arm-linux-gnueabihf-cpp           arm-linux-gnueabihf-gcov-tool
arm-linux-gnueabihf-dwp           arm-linux-gnueabihf-gprof
arm-linux-gnueabihf-elfedit       arm-linux-gnueabihf-ld
arm-linux-gnueabihf-g++           arm-linux-gnueabihf-ld.bfd       arm-linux-gnueabihf-ld.gold
arm-linux-gnueabihf-gcc           arm-linux-gnueabihf-nm         arm-linux-gnueabihf-objcopy
arm-linux-gnueabihf-gcc-ar        arm-linux-gnueabihf-objdump     arm-linux-gnueabihf-ranlib
arm-linux-gnueabihf-gcc-nm        arm-linux-gnueabihf-readelf   arm-linux-gnueabihf-size
arm-linux-gnueabihf-gcc-ranlib    arm-linux-gnueabihf-strings  arm-linux-gnueabihf-strip

@Neumann-A
Copy link
Contributor

compiler via command line options.

I am speaking of env variables like CC.
The required changes are e.g. in
https://github.com/microsoft/vcpkg/pull/22254/files#diff-b92b07d5c0c681687d71463c6a368fae98e9ffedb241e2aadbde3aac688899a9

It requires to edit the files

No it doesn't require it. There are ac_cv_ variables which can be passed to circumvent the configure check. (which is why #22473 exists)

This is, why all our targets win/osx/linux are using the gcc compiler!

No its not. Windows does use cl in vcpkg by setting CC and stuff..... please check the script. The same is just missing for !Windows.

counterpart of generating the ac_tool_prefix from cmake

I am not generating ac_tool_prefix.... I am just setting CC and passing some arbitrary --host if VCPKG_CROSSCOMPILG is true.

Also from #22331 (comment)):

Also, does this work when you're using clang?
No, in clang you control the compilation target by command line

This requires CC to be set to clang. Otherwise configure will just look for gcc

You need to acknowledge that there is a bunch of tools autotools makes use of with this prefix

And? I am just asking to specify them explicitly instead of implicitly. In CMake you already have a toolchain where everything is explicitly set.

@daschuer
Copy link
Contributor Author

daschuer commented Feb 6, 2022

I have no interest to argue this any longer or put more work into it . And I really don't mind how the issue is solved.

The solution here is a copy of the OSX solution, it just works and does no break anything. So It is up to you accept this or provide a better solution. Since this is only a 15 lines patch with limited scope you may also consider to merge this and revert this by a refactoring of the whole file this also included OSX.

@ras0219-msft ras0219-msft added the requires:vcpkg-team-review This PR or issue requires someone on the vcpkg team to take a further look. label Feb 8, 2022
@ras0219-msft ras0219-msft merged commit d15470b into microsoft:master Feb 9, 2022
@ras0219-msft
Copy link
Contributor

After deliberation, we think this is an acceptable incremental improvement pending a full rewrite of the vcpkg_configure_make/vcpkg_install_make as helper ports. All of @Neumann-A's points are valid and need addressing in the long term, but until that rewrite happens this PR does marginally improve the situation.

I will also note that users can directly specify the autotools host triplet in their vcpkg triplet file via VCPKG_MAKE_BUILD_TRIPLET, which will override this detection and will work for any compiler.

@Neumann-A
Copy link
Contributor

@ras0219-msft I still think this shouldn't have been merged. As you mentioned VCPKG_MAKE_BUILD_TRIPLET would have been an supported workflow for the issues. As such this PR does not marginally improve the situation (since there is already a valid workflow) and the rewrite of the scripts will now more probably break users. Furthermore, you are now forcing everybody to rebuild their dependencies with this merge since this should have been marked as a world-rebuild PR.

@daschuer daschuer deleted the autotools_crosscompile branch January 5, 2023 07:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:tool-update The issue is with build tool or build script, which requires update or should be executed correctly info:reviewed Pull Request changes follow basic guidelines requires:discussion requires:vcpkg-team-review This PR or issue requires someone on the vcpkg team to take a further look.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants