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

[Ace] Support -fPIC compiler flag on Linux #22864

Merged
merged 8 commits into from
Feb 8, 2022

Conversation

pierrebizz
Copy link
Contributor

Add fpic feature to enable the -fPIC flag in order to be able to link ACE/TAO in a dynamic library

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

    ,

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

@Hoikas
Copy link
Contributor

Hoikas commented Jan 30, 2022

My understanding is that vcpkg prefers to always pass -fPIC, so this should not be a feature, rather just a bugfix.

@cenit
Copy link
Contributor

cenit commented Jan 31, 2022

My understanding is that vcpkg prefers to always pass -fPIC, so this should not be a feature, rather just a bugfix.

yes, i think this is the rule here. Please remove the feature and apply the flag always

@pierrebizz
Copy link
Contributor Author

Very well; is there a risk that it will break the builds of people already using this config without the fPIC flag ?

@Neumann-A
Copy link
Contributor

This is a case for z_vcpkg_get_cmake_vars and not manually adding flags.

@pierrebizz
Copy link
Contributor Author

Can someone more intelligent than me explain what exactly failed in the x64_windows_static build ?

@cenit
Copy link
Contributor

cenit commented Jan 31, 2022

Error: vcpkg has crashed; no additional details are available.
The source line is D:\a\_work\1\s\src\vcpkg\binarycaching.cpp(554)

not related to your PR.
vcpkg ci is somehow broken, HTTP remotes are pointing to non-existing address (err 403)

@pierrebizz
Copy link
Contributor Author

Do I have to do something to retrigger ?

@cenit
Copy link
Contributor

cenit commented Jan 31, 2022

you can merge with master if your branch is not already fully up to date, or you can just wait for a maintainer with the power to launch an /azp run command

@@ -105,7 +105,7 @@ if(VCPKG_TARGET_IS_WINDOWS)
elseif(VCPKG_TARGET_IS_LINUX)
set(SOLUTION_TYPE gnuace)
set(config_h_contents "#include \"ace/config-linux.h\"\n")
file(WRITE "${ACE_ROOT}/include/makeinclude/platform_macros.GNU" "include $(ACE_ROOT)/include/makeinclude/platform_linux.GNU")
file(WRITE "${ACE_ROOT}/include/makeinclude/platform_macros.GNU" "CCFLAGS += -fPIC\ninclude $(ACE_ROOT)/include/makeinclude/platform_linux.GNU")
Copy link
Contributor

Choose a reason for hiding this comment

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

At the moment you build ACE/TAO as shared library we do add -fPIC already to the compiler flags, no need to do that manually. As far as I know vcpkg does a static build, at that moment -fPIC is not used

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, I read in the discussion that vcpkg always wants to use -fPIC

@Cheney-W Cheney-W added the category:port-bug The issue is with a library, which is something the port should already support label Feb 7, 2022
@Cheney-W
Copy link
Contributor

Cheney-W commented Feb 7, 2022

@pierrebizz Could you please solve the conflict?

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

This is a new experimental fast check for PR issues. Please let us know if this bot is helpful!

After committing all other changes, the version database must be updated
git add -u && git commit
git checkout d2b22891a13123accac084b5feabfd0914f4e243 -- versions
./vcpkg x-add-version --all
Diff
diff --git a/versions/a-/ace.json b/versions/a-/ace.json
index d30da3e..b9dd64e 100644
--- a/versions/a-/ace.json
+++ b/versions/a-/ace.json
@@ -1,7 +1,7 @@
 {
   "versions": [
     {
-      "git-tree": "016c4bb6add08e6e2b82254adb14e0d49a298e48",
+      "git-tree": "d20fc0a9eac1744ff9fb5c5427eade58234e35c9",
       "version": "7.0.6",
       "port-version": 3
     },

@Cheney-W Cheney-W changed the title Ace - add fpic feature to support -fPIC compiler flag on Linux [Ace] Support -fPIC compiler flag on Linux Feb 8, 2022
@Cheney-W Cheney-W added info:reviewed Pull Request changes follow basic guidelines and removed requires:author-response labels Feb 8, 2022
@ras0219-msft
Copy link
Contributor

@Neumann-A's point is correct, the best long term solution is to use vcpkg_cmake_get_vars() from the vcpkg-cmake helper and not to hardcode flags.

However, without someone signed up to do that work, I don't want to block this reasonable incremental improvement. @pierrebizz I'd encourage you to consider submitting a PR that uses the above function to get the full set of CMake settings, which would greatly improve compatibility in more ways than just -fPIC.

Thanks again!

@ras0219-msft ras0219-msft merged commit 5fe063f into microsoft:master Feb 8, 2022
Jimmy-Hu added a commit to Jimmy-Hu/vcpkg that referenced this pull request Feb 8, 2022
[Ace] Support -fPIC compiler flag on Linux (microsoft#22864)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:port-bug The issue is with a library, which is something the port should already support info:reviewed Pull Request changes follow basic guidelines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants