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.

Sign up for GitHub

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

Jump to bottom

Fix #7715, in rpath_flags use host OS instead of build OS to determine -rpath separator #7716

Merged
merged 10 commits into from Jan 14, 2021
Merged

Fix #7715, in rpath_flags use host OS instead of build OS to determine -rpath separator #7716

merged 10 commits into from Jan 14, 2021

Conversation

Erlkoenig90
Copy link
Contributor

@Erlkoenig90 Erlkoenig90 commented Sep 17, 2020 •
edited by memsharded

Fix issue #7715.

Changelog: Bugfix: Change the rpath_flags flag to always use the comma separator instead of "=", because the current behaviour causes linker error messages when attempting to cross-compile to Mac OS, and the comma separator is accepted everywhere.
Docs: Omit

  • Refer to the issue that supports this Pull Request.
  • If the issue has missing info, explain the purpose/use case/pain/need that covers this Pull Request.
  • I've read the Contributing guide.
  • I've followed the PEP8 style guides for Python code.
  • I've opened another PR in the Conan docs repo to the develop branch, documenting this one.

Note: By default this PR will skip the slower tests and will use a limited set of python versions. Check here how to increase the testing level by writing some tags in the current PR body text.

@Erlkoenig90
Fix conan-io#7715, in rpath_flags use host OS instead of build OS to …
a729522 
…determine the separator for the ld -rpath flag which is neccessary for cross-compiling to Mac OS
@Erlkoenig90
For conan-io#7715, don't pass os_host to rpath_flags but extract from…
7ee24c8 
… settings, pass os_build again and only set rpath_separator to , if compiling for Mac OS from Linux or Mac OS via GCC_LIKE
@solvingj
Copy link
Contributor

This looks correct to me. @memsharded vote to add this to 1.31 milestone because makefile generator uses this function and will be updated in 1.31.

@Erlkoenig90 Erlkoenig90 mentioned this pull request Jan 13, 2021
Closed
@Erlkoenig90
Copy link
Contributor Author

@solvingj Hi, can we get this merged or is further work required? The bug is a showstopper for cross compiling Linux -> Mac OS X, and the fix would influence only this one case, leaving everything else working as-is.

@memsharded
Copy link
Member

Hi @Erlkoenig90

Seems a test is still broken: test_compiler_args, because of a changed output (https://ci.conan.io/blue/organizations/jenkins/ConanTestSuite/detail/PR-7716/10/pipeline/) Are we completely sure that this will not be breaking for other users? could you please have a look to the test? Thanks!

@memsharded
Copy link
Member

Thanks for having a look and fixing the test.

I am trying to understand why it is not always "," as separator, it seems that all linkers might be happy with it.
@SSE4 did this code, any suggestion here?

@memsharded memsharded requested a review from SSE4 January 14, 2021 16:09
@Erlkoenig90
Copy link
Contributor Author

Thanks, I modified the test. If you use Clang to compile on Mac OS for Linux, you'd use LLD or GNU LD, both of which use the -rpath= syntax, so IMHO the test was wrong. Only when using Apple's LD64 for (cross-)compiling for Mac OS (on Linux or Mac OS) is the -rpath, syntax needed.

@solvingj
Copy link
Contributor

Great info @Erlkoenig90 , thanks for the insight and contribution!

@SSE4
Copy link
Contributor

SSE4 commented Jan 14, 2021

as I can say, GNU LD accepts -Wl,-rpath,dir syntax.
CMake also uses that:

./Modules/Platform/GNU.cmake:5:set(CMAKE_SHARED_LIBRARY_RUNTIME_C_FLAG "-Wl,-rpath,")

@memsharded
Copy link
Member

Perfect, many thanks. Lets use comma always then. Already did the changes, will merge when CI is green.

@memsharded memsharded added this to the 1.33 milestone Jan 14, 2021
@memsharded memsharded merged commit a924fd4 into conan-io:develop Jan 14, 2021
@Erlkoenig90
Copy link
Contributor Author

Awesome, thanks!

memsharded added a commit to memsharded/conan that referenced this pull request Jan 18, 2021
@Erlkoenig90 @memsharded @SSE4
Fix conan-io#7715, in rpath_flags use host OS instead of build OS to …
ef7053c 
…determine -rpath separator (conan-io#7716)

* Fix conan-io#7715, in rpath_flags use host OS instead of build OS to determine the separator for the ld -rpath flag which is neccessary for cross-compiling to Mac OS

* For conan-io#7715, don't pass os_host to rpath_flags but extract from settings, pass os_build again and only set rpath_separator to , if compiling for Mac OS from Linux or Mac OS via GCC_LIKE

* Fix test_compiler_args for cross-compiling Mac OS X -> Linux

* Update conans/client/build/compiler_flags.py

Co-authored-by: SSE4 <tomskside@gmail.com>

* Update conans/client/build/compiler_flags.py

Co-authored-by: SSE4 <tomskside@gmail.com>

* Update conans/test/unittests/client/generators/compiler_args_test.py

* Update conans/client/build/compiler_flags.py

* Use -rpath, in all tests for all platforms

* fixing tests

Co-authored-by: James <james@conan.io>
Co-authored-by: SSE4 <tomskside@gmail.com>
memsharded added a commit that referenced this pull request Jan 18, 2021
@memsharded @czoido
@Erlkoenig90 @SSE4 @danimtb @jgsogo
Feature/msvc msbuild (#8238)
040a556 
* working in msvc new settings

* preliminary support for msvc compiler

* fix migration

* adding msvc for msbuild toolchain

* changing to version 19.1, 19.11, etc

* working on static/dynamic runtime

* extracting toolchain checkers to use in CMake

* removed unused import

* adding checks

* adding check

* first proposal

* msvc proposal for runtime

* fix migration test

* fixing tests

* binary compatibility

* command conan new generates files with new toolchains

* removed conanfile

* removed files

* msbuild working for msvc

* fixing tests

* fixing tests

* refactors

* Update conan/tools/microsoft/toolchain.py

Co-authored-by: Carlos Zoido <mrgalleta@gmail.com>

* Update conan/tools/microsoft/toolchain.py

Co-authored-by: Carlos Zoido <mrgalleta@gmail.com>

* Fix #7715, in rpath_flags use host OS instead of build OS to determine -rpath separator (#7716)

* Fix #7715, in rpath_flags use host OS instead of build OS to determine the separator for the ld -rpath flag which is neccessary for cross-compiling to Mac OS

* For #7715, don't pass os_host to rpath_flags but extract from settings, pass os_build again and only set rpath_separator to , if compiling for Mac OS from Linux or Mac OS via GCC_LIKE

* Fix test_compiler_args for cross-compiling Mac OS X -> Linux

* Update conans/client/build/compiler_flags.py

Co-authored-by: SSE4 <tomskside@gmail.com>

* Update conans/client/build/compiler_flags.py

Co-authored-by: SSE4 <tomskside@gmail.com>

* Update conans/test/unittests/client/generators/compiler_args_test.py

* Update conans/client/build/compiler_flags.py

* Use -rpath, in all tests for all platforms

* fixing tests

Co-authored-by: James <james@conan.io>
Co-authored-by: SSE4 <tomskside@gmail.com>

* modernizing tests (#8340)

* extracting some common code to base class (#8341)

* modernizing tests (#8345)

* Fix help message in command remove --outdated (#8350)

* [test] Add test creating a target without namespaces (CMake) (#8338)

* use target without namespace

* add marks

* use target from the build-module

* remove line

* build-module per generator

* revert changes in build_info

* make it explicit it is PATH

* even without build-type, compiler.runtime is different for Release/Debug

* use x64 for Windows generator

* update tests (#8354)

* added integration test for msbuild

Co-authored-by: Carlos Zoido <mrgalleta@gmail.com>
Co-authored-by: Niklas Gürtler <Erlkoenig90@users.noreply.github.com>
Co-authored-by: SSE4 <tomskside@gmail.com>
Co-authored-by: Daniel <danimanzaneque@gmail.com>
Co-authored-by: Javier G. Sogo <jgsogo@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SSE4 SSE4 SSE4 left review comments

@memsharded memsharded memsharded left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
1.33
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@Erlkoenig90 @solvingj @memsharded @SSE4