-
Notifications
You must be signed in to change notification settings - Fork 438
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
[BUILD] Patches for building on AIX #3127
Conversation
✅ Deploy Preview for opentelemetry-cpp-api-docs canceled.
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3127 +/- ##
==========================================
+ Coverage 87.12% 87.83% +0.71%
==========================================
Files 200 195 -5
Lines 6109 6136 +27
==========================================
+ Hits 5322 5389 +67
+ Misses 787 747 -40 |
No, one is ALIAS and the other is ALIASED
________________________________
From: Lalit Kumar Bhasin ***@***.***>
Sent: Wednesday, November 6, 2024 5:36 PM
To: open-telemetry/opentelemetry-cpp ***@***.***>
Cc: Chris Ward ***@***.***>; Mention ***@***.***>
Subject: [EXTERNAL] Re: [open-telemetry/opentelemetry-cpp] Patches for building on AIX (PR #3127)
@ lalitb commented on this pull request. In cmake/tools. cmake: > @@ -160,6 +160,10 @@ function(project_build_tools_patch_default_imported_config) if(IS_ALIAS_TARGET) continue() endif() + get_target_property(IS_ALIASED_TARGET ${TARGET_NAME}
@lalitb commented on this pull request.
________________________________
In cmake/tools.cmake<#3127 (comment)>:
@@ -160,6 +160,10 @@ function(project_build_tools_patch_default_imported_config)
if(IS_ALIAS_TARGET)
continue()
endif()
+ get_target_property(IS_ALIASED_TARGET ${TARGET_NAME} ALIASED_TARGET)
+ if (IS_ALIASED_TARGET)
+ continue()
+ endif()
This if condition appears redundant, as it checks the same ALIASED_TARGET property again
—
Reply to this email directly, view it on GitHub<#3127 (review)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AC5CNDOZ25LKHC3UX5JAB3DZ7JHR5AVCNFSM6AAAAABRIPLQZSVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDIMJZGAYTGNRUGI>.
You are receiving this because you were mentioned.Message ID: ***@***.***>
Unless otherwise stated above:
IBM United Kingdom Limited
Registered in England and Wales with number 741598
Registered office: Building C, IBM Hursley Office, Hursley Park Road, Winchester, Hampshire SO21 2JN
|
Is the CI failure something that I have caused ? Can someone tell me more about what is broken, and give me a test case to reproduce the fault? |
@tjcw The CI failure is for the easyCLA you need to sign: The PR can be merged only after that, and once all comments are resolved. |
OK. I am currently waiting for the person I think is IBM's CLA coordinator to turn round the email. |
I just got notified of a CI failure with this. It looks to me like a configuration or permission failure rather than a problem with my code. Can someone check please? |
The following build failed in CI: This looks unrelated, restarting this build. [edit] Somehow, this test fails and spins forever: |
* Special considerations for HostArchValues | ||
* | ||
* The sys/systemcfg.h header on AIX is known to define an IA64 macro, | ||
* which collides with HostArchValues::IA64. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This work around is no longer necessary, after:
Change naming to use camel case, to avoid collisions with macros.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will change the PR to withdraw this workaround
Does my PR make this test loop forever, or does this occur without my PR ? |
This is much likely an issue in the github worker itself, this happens some times (the github runner was upgraded lately). I will investigate it separately. [edit] The only change in this area that can affect grpc is file |
I have backed out the change to cmake/opentelemetry-proto.cmake ; I can try applying it as a separate PR if necessary. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks for the contribution
[BUILD] Patches for building on AIX (open-telemetry#3127)
Fixes #3126
Changes
Build scripts and code tweaks to enable build on AIX
For significant contributions please make sure you have completed the following items:
CHANGELOG.md
updated for non-trivial changes