This is an archive of the discontinued LLVM Phabricator instance.

[libc++][AIX] Add OS version to target triple
ClosedPublic

Authored by Jake-Egan on May 2 2023, 9:17 AM.

Details

Reviewers
daltenty
Mordante
Group Reviewers
Restricted Project
Restricted Project
Restricted Project
Commits
rG4fc70197919c: [libc++][AIX] Add OS version to target triple
Summary

This will allow for configuring tests according to AIX version.

Diff Detail

Event Timeline

Jake-Egan created this revision.May 2 2023, 9:17 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 2 2023, 9:17 AM
Herald added a subscriber: arichardson. · View Herald Transcript
Jake-Egan requested review of this revision.May 2 2023, 9:17 AM
Jake-Egan edited the summary of this revision. (Show Details)May 2 2023, 9:23 AM
Jake-Egan added reviewers: Restricted Project, Restricted Project, Restricted Project.May 2 2023, 9:25 AM
ldionne added inline comments.
libcxx/utils/ci/run-buildbot
709

Normally, the target triple is figured out automatically in the CMake code. I think it's LLVM_DEFAULT_TARGET_TRIPLE. Is there a reason why that triple isn't inferred properly? If it were, I think this would not be necessary.

philnik set the repository for this revision to rG LLVM Github Monorepo.May 10 2023, 12:51 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptMay 10 2023, 12:51 PM
Herald added 1 blocking reviewer(s): Restricted Project. · View Herald Transcript
Herald added 1 blocking reviewer(s): Restricted Project. · View Herald Transcript
Jake-Egan planned changes to this revision.May 19 2023, 6:35 AM
Jake-Egan updated this revision to Diff 554375.Aug 29 2023, 8:50 AM

Get full AIX version

Jake-Egan added inline comments.Aug 29 2023, 9:03 AM
libcxx/utils/ci/run-buildbot
709

We considered adding the AIX version to the triple in the driver, but we don't have a good way to retrieve the AIX version in the driver right now. So, we prefer to do it in the test-suite to avoid any potential problems.

Mordante requested changes to this revision.Aug 29 2023, 10:19 AM
Mordante added a subscriber: Mordante.

Get full AIX version

Note that the AIX build is not available at the moment. I'll mark this as needs work so it can be rebased once the builder is available again.

This revision now requires changes to proceed.Aug 29 2023, 10:19 AM

It looks like this PR isn't configured right to run the libc++ CI

This seems to be the libc++ run https://buildkite.com/llvm-project/libcxx-ci/builds/29326

I'm not sure why it also runs other CIs, but that might be due to recent experiments. Maybe rebasing will solve the issue.

libcxx/utils/ci/run-buildbot
709

Can you add some comment to explain this?

Jake-Egan updated this revision to Diff 556032.Sep 6 2023, 7:31 AM

Added a comment and rebase

Jake-Egan marked an inline comment as done.Sep 6 2023, 7:31 AM
daltenty added inline comments.Oct 17 2023, 10:13 AM
libcxx/utils/ci/run-buildbot
711

Hmm, I wonder if this script is really the right place for this. If you don't go through this script to do the build, or the environment like this var isn't properly set when you run it, you won't get a correct test run result.

I think it would be better if this was implemented somewhere inside the lit config somehow.

Jake-Egan planned changes to this revision.Oct 17 2023, 11:26 AM
Jake-Egan added inline comments.
libcxx/utils/ci/run-buildbot
711

I will look into this

Jake-Egan edited the summary of this revision. (Show Details)

Use the lit config instead

Jake-Egan updated this revision to Diff 557841.Oct 23 2023, 6:56 AM

Trigger CI

daltenty added inline comments.Oct 23 2023, 1:26 PM
libcxx/test/configs/ibm-libc++-shared.cfg.in
10 ↗(On Diff #557849)

You need some kind of guard so we don't run this block on non-AIX platforms

Jake-Egan updated this revision to Diff 557865.Oct 24 2023, 7:25 AM
  • Guard for AIX
Jake-Egan marked an inline comment as done.Oct 24 2023, 7:25 AM
daltenty accepted this revision.Oct 24 2023, 8:26 AM
daltenty added inline comments.
libcxx/test/configs/ibm-libc++-shared.cfg.in
7 ↗(On Diff #557865)

Maybe you can make this a utility function inside llvm/utils/lit/lit/util.py to avoid repeating the code?

(There are similarish util functions for macos for example)

Jake-Egan updated this revision to Diff 557918.Oct 27 2023, 9:49 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 27 2023, 9:49 AM
Jake-Egan marked an inline comment as done.Oct 27 2023, 9:49 AM
daltenty added inline comments.Oct 30 2023, 8:09 AM
libcxx/test/configs/ibm-libc++-shared.cfg.in
7 ↗(On Diff #557865)

I'm confused, why not move this whole complicated function? (You've only moved the target check)

Jake-Egan updated this revision to Diff 557944.Oct 31 2023, 7:22 AM
Jake-Egan marked an inline comment as done.
daltenty accepted this revision.Oct 31 2023, 8:22 AM

LGTM, thanks!

Mordante accepted this revision.Oct 31 2023, 12:47 PM

LGTM modulo one nit.

llvm/utils/lit/lit/util.py
13 ↗(On Diff #557944)

Nit: Please keep the imports in alphabetic order.

Jake-Egan updated this revision to Diff 557951.Oct 31 2023, 1:16 PM
Jake-Egan marked an inline comment as done.
This revision was not accepted when it landed; it landed in state Needs Review.Oct 31 2023, 1:30 PM
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptOct 31 2023, 1:30 PM
Herald added 1 blocking reviewer(s): Restricted Project. · View Herald Transcript