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] Fix handling of link flags #22798

Merged
merged 7 commits into from
Feb 16, 2022

Conversation

daschuer
Copy link
Contributor

This fixes the handling of link flags for Autotools ports, a prerequisite for #22516

  • What does your PR fix?

  • don't set LD_LIBRARY_PATH in vcpkg_configure_make like in vcpkg_build_make
  • Fix extra space when linking to manual-link
  • only link against manual-link folder if exists
  • only link against manual-link folder if exists
  • Only set the lib path if exists
  • Which triplets are supported/not supported? Have you updated the CI baseline?

    No change

  • 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?

    not required

@Cheney-W Cheney-W added the category:vcpkg-bug The issue is with the vcpkg system (including helper scripts in `scripts/cmake/`) label Jan 27, 2022
@JackBoosY JackBoosY added category:tool-update The issue is with build tool or build script, which requires update or should be executed correctly and removed category:vcpkg-bug The issue is with the vcpkg system (including helper scripts in `scripts/cmake/`) labels Jan 27, 2022
@JackBoosY
Copy link
Contributor

cc @Neumann-A for review this changes.

@JackBoosY JackBoosY added the info:reviewed Pull Request changes follow basic guidelines label Feb 7, 2022
@JackBoosY JackBoosY added requires:author-response and removed info:reviewed Pull Request changes follow basic guidelines labels Feb 8, 2022
@ras0219-msft
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@JackBoosY JackBoosY added depends:different-pr This PR or Issue depends on a PR which has been filed and removed requires:author-response labels Feb 11, 2022
@JackBoosY
Copy link
Contributor

colmap:x64-osx regression will be fixed in another PR.

@JackBoosY JackBoosY removed the depends:different-pr This PR or Issue depends on a PR which has been filed label Feb 14, 2022
@JackBoosY
Copy link
Contributor

@Neumann-A Any other suggestions?

@Neumann-A
Copy link
Contributor

i already left all my comments and the bugs are fixed. so no?

@JackBoosY JackBoosY added the info:reviewed Pull Request changes follow basic guidelines label Feb 16, 2022
@BillyONeal
Copy link
Member

Thanks for fixing what looks like it was a very tricky ball of wax to dig through! I can't believe the VCPKG_DETECTED_STATIC_LINKER_FLAGS_ typo was in there :(.

I hate that this is messing with %_LINK_% which is generally a user setting rather than a build tools setting, but that's what this was doing before.

Comment on lines +150 to 155
if(EXISTS "${Z_VCPKG_INSTALLED}${path_suffix}/lib")
string(APPEND LDFLAGS_${cmake_buildtype} " -L${Z_VCPKG_INSTALLED}${path_suffix}/lib")
endif()
if(EXISTS "${Z_VCPKG_INSTALLED}${path_suffix}/lib/manual-link")
string(APPEND LDFLAGS_${cmake_buildtype} " -L${Z_VCPKG_INSTALLED}${path_suffix}/lib/manual-link")
endif()
Copy link
Member

Choose a reason for hiding this comment

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

This is clearly an improvement vs. the status quo so I'm merging it, however, shouldn't this be:

Suggested change
if(EXISTS "${Z_VCPKG_INSTALLED}${path_suffix}/lib")
string(APPEND LDFLAGS_${cmake_buildtype} " -L${Z_VCPKG_INSTALLED}${path_suffix}/lib")
endif()
if(EXISTS "${Z_VCPKG_INSTALLED}${path_suffix}/lib/manual-link")
string(APPEND LDFLAGS_${cmake_buildtype} " -L${Z_VCPKG_INSTALLED}${path_suffix}/lib/manual-link")
endif()
if(EXISTS "${Z_VCPKG_INSTALLED}${path_suffix}/lib")
string(APPEND LDFLAGS_${cmake_buildtype} " -L\"${Z_VCPKG_INSTALLED}${path_suffix}/lib\"")
endif()
if(EXISTS "${Z_VCPKG_INSTALLED}${path_suffix}/lib/manual-link")
string(APPEND LDFLAGS_${cmake_buildtype} " -L\"${Z_VCPKG_INSTALLED}${path_suffix}/lib/manual-link"")
endif()

or something like that? (I'm not sure what syntax is is strictly speaking correct here but I suspect we should be defending against spaces)

Copy link
Member

Choose a reason for hiding this comment

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

It looks like the diff is confused. All I did was add quotes, changing:

"-L${Z_VCPKG_INSTALLED}${path_suffix}/lib/manual-link"

to

"-L\"${Z_VCPKG_INSTALLED}${path_suffix}/lib/manual-link\""

@BillyONeal BillyONeal merged commit 016788b into microsoft:master Feb 16, 2022
@daschuer daschuer deleted the manual-link 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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants