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

ci: Add support for dist-loongarch64-linux #110519

Merged
merged 1 commit into from
May 23, 2023
Merged

ci: Add support for dist-loongarch64-linux #110519

merged 1 commit into from
May 23, 2023

Conversation

heiher
Copy link
Contributor

@heiher heiher commented Apr 19, 2023

We are preparing to promote loongarch64-unknown-linux-gnu to Tier 2, and one of the tasks is to add CI support. We are currently in the process of upgrading the dependencies for the build tools, and before this is completed, we would like to request comments. Thanks

Progress

Tier 2 with host tools MCP: rust-lang/compiler-team#518

@rustbot
Copy link
Collaborator

rustbot commented Apr 19, 2023

r? @Mark-Simulacrum

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added A-testsuite Area: The testsuite used to check the correctness of rustc S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue. labels Apr 19, 2023
@rust-log-analyzer

This comment has been minimized.

@heiher heiher marked this pull request as ready for review April 26, 2023 03:36
@heiher
Copy link
Contributor Author

heiher commented Apr 26, 2023

@bors try

@bors
Copy link
Contributor

bors commented Apr 26, 2023

@heiher: 🔑 Insufficient privileges: not in try users

@heiher
Copy link
Contributor Author

heiher commented Apr 29, 2023

@cuviper
Copy link
Member

cuviper commented May 2, 2023

Note that try only runs dist-x86_64-linux unless you (temporarily) edit the CI config for that job.

My crosstool refactoring in #110865 was just approved, so it would be great if you followed suit here. I see that you're using a git snapshot for loongarch support, but apart from that you should be able to work like the others.

@heiher
Copy link
Contributor Author

heiher commented May 3, 2023

Hello @cuviper, Thanks for your comments.

Note that try only runs dist-x86_64-linux unless you (temporarily) edit the CI config for that job.

You're right, I did make a temporary change to the CI configuration for that job.

My crosstool refactoring in #110865 was just approved, so it would be great if you followed suit here. I see that you're using a git snapshot for loongarch support, but apart from that you should be able to work like the others.

Great job! Thank you for pushing the update for crosstool-ng. However, support for LoongArch is expected to be available only in the next release version (1.26). Therefore, it may be a good idea for us to use the git snapshot version for now and make the change once version 1.26 is released. This way, we shouldn't miss out on Rust 1.71.

I am grateful for your insightful comments and suggestions. ❤️

@heiher heiher marked this pull request as draft May 3, 2023 01:19
@cuviper
Copy link
Member

cuviper commented May 3, 2023

@heiher Yes, it's fine that you need to use a pre-release version of crosstool-ng. I meant that you should try to follow the rest of the refactoring that I implemented: committing a defconfig rather than a full config, and using the new common scripts/crosstool-ng-build.sh instead of individual scripts like your build-toolchains.sh.

@bors
Copy link
Contributor

bors commented May 3, 2023

☔ The latest upstream changes (presumably #110865) made this pull request unmergeable. Please resolve the merge conflicts.

@heiher
Copy link
Contributor Author

heiher commented May 4, 2023

@cuviper It has been rebased to the mainline and passed the tests. Does it look good to you please?

@heiher heiher marked this pull request as ready for review May 4, 2023 00:02
Copy link
Contributor

@xen0n xen0n 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 much easier to review now, thanks for the rebasing! Only one question from me below.

- Operating System > Linux kernel version = 5.19.16
- Binary utilities > Version of binutils = 2.40
- C-library > glibc version = 2.36
- C compiler > gcc version = 12.2.0
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we able to raise this to GCC 13.1.0 now that it's released?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Certainly, although GCC 13 has been released, the upgrade for crosstool-ng is still in progress. We would like to catch up with the Rust 1.71 release, and since LoongArch CI is not dependent on GCC 13, I propose that we upgrade after crosstool-ng is ready, possibly through a new PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If crosstool-ng is ready earlier than expected, I would be happy to include it in this PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah yes. I forgot we privately talked about this a while ago.

Copy link
Member

@cuviper cuviper left a comment

Choose a reason for hiding this comment

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

I'm going to leave the formal review to Mark. I'm not sure if the target should be built in CI at all until it has approval per the target tier policy.

CT_DEBUG_GDB=y
CT_GDB_V_12=y
# CT_GDB_CROSS_PYTHON is not set
# CT_GDB_GDBSERVER is not set
Copy link
Member

Choose a reason for hiding this comment

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

We don't need GDB, so all 4 of these can be removed.

# CT_GDB_CROSS_PYTHON is not set
# CT_GDB_GDBSERVER is not set
CT_ISL_V_0_24=y
CT_ZSTD_V_1_5_2=y
Copy link
Member

Choose a reason for hiding this comment

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

On all other targets, I left out versions for things like ISL and ZSTD so they could just use their defaults. Are there particular needs here?

CT_CONFIG_VERSION="4"
CT_EXPERIMENTAL=y
CT_PREFIX_DIR="/x-tools/${CT_TARGET}"
CT_LOG_ALL=y
Copy link
Member

Choose a reason for hiding this comment

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

We don't really need logging, but I guess it doesn't hurt either way.

Please do add the mirror config like the other targets though:

CT_USE_MIRROR=y
CT_MIRROR_BASE_URL="https://ci-mirrors.rust-lang.org/rustc"

(We don't have everything mirrored, but AIUI this will let our mirror take precedence when we do add things.)

@heiher
Copy link
Contributor Author

heiher commented May 5, 2023

I'm going to leave the formal review to Mark. I'm not sure if the target should be built in CI at all until it has approval per the target tier policy.

Thanks for your feedback. We are moving forward, and there is a resolved MCP that for LoongArch to Tier2: rust-lang/compiler-team#518

@heiher
Copy link
Contributor Author

heiher commented May 6, 2023

@rustbot author

cargo build breaks by libc constants are missing.

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 6, 2023
@Mark-Simulacrum
Copy link
Member

Can you temporarily enable the builder here for try builds or in PR CI? That will let us test how long this takes (after Docker is cached) and make a decision in T-infra on whether we currently have capacity to add the builder.

#!/bin/sh
set -ex

git clone --depth=1 https://github.com/crosstool-ng/crosstool-ng
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be pinned to a fixed commit, to make sure we don't pick up a broken commit.

@heiher
Copy link
Contributor Author

heiher commented May 7, 2023

@Mark-Simulacrum Certainly, Thank you.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@Mark-Simulacrum Mark-Simulacrum removed the S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). label May 22, 2023
@Mark-Simulacrum
Copy link
Member

Infra discussed and agreed to move forward with adding the builder.

@bors r+

@bors
Copy link
Contributor

bors commented May 22, 2023

📌 Commit 5f173e9 has been approved by Mark-Simulacrum

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 22, 2023
@bors
Copy link
Contributor

bors commented May 22, 2023

⌛ Testing commit 5f173e9 with merge 4f80088941583158b9f62e8d6a60b94a087329d9...

@bors
Copy link
Contributor

bors commented May 23, 2023

💥 Test timed out

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels May 23, 2023
@xen0n
Copy link
Contributor

xen0n commented May 23, 2023

The dist-x86_64-apple build seems to be flaking out.

@rust-log-analyzer
Copy link
Collaborator

A job failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

@Dylan-DPC Dylan-DPC added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 23, 2023
@heiher
Copy link
Contributor Author

heiher commented May 23, 2023

@bors retry

This PR doesn't seem bad, retry again.

@bors
Copy link
Contributor

bors commented May 23, 2023

@heiher: 🔑 Insufficient privileges: not in try users

@workingjubilee
Copy link
Member

@bors retry

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 23, 2023
@bors
Copy link
Contributor

bors commented May 23, 2023

⌛ Testing commit 5f173e9 with merge cda5bec...

@@ -22,6 +22,7 @@ apt-get update && DEBIAN_FRONTEND=noninteractive apt-get install -y --no-install
patch \
pkg-config \
python3 \
rsync \
Copy link
Contributor

Choose a reason for hiding this comment

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

rsync used somewhere? Can't see any matches here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is used by crosstool-ng git version.

@bors
Copy link
Contributor

bors commented May 23, 2023

☀️ Test successful - checks-actions
Approved by: Mark-Simulacrum
Pushing cda5bec to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label May 23, 2023
@bors bors merged commit cda5bec into rust-lang:master May 23, 2023
@rustbot rustbot added this to the 1.71.0 milestone May 23, 2023
@heiher heiher deleted the ci branch May 23, 2023 10:33
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (cda5bec): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
2.9% [2.8%, 3.0%] 2
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-1.6% [-1.6%, -1.6%] 1
All ❌✅ (primary) - - 0

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 644.363s -> 644.28s (-0.01%)

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jun 10, 2023
ci: Upgrade loongarch64-linux-gnu GCC to 13.1.0

This PR upgrades GCC to 13.1.0 for the `loongarch64-unknown-linux-gnu` target. This upgrade was suggested in a previous review discussion: rust-lang#110519 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-testsuite Area: The testsuite used to check the correctness of rustc merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.