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

Fix Diff-Check job for subtree sync PRs #5953

Conversation

ytmimi
Copy link
Contributor

@ytmimi ytmimi commented Oct 28, 2023

rustfmt currently has a runtime dependency on the sysroot. So when we build a standalone rustfmt binary we need to set LD_LIBRARY_PATH so each rustfmt binary knows where to find it's dependencies.

When running our Diff-Check job to test PRs for breaking changes it's often the case that both the master rustfmt binary and the feature branch binary have the same runtime dependencies so we only need to set LD_LIBRARY_PATH once.

However, when running the diff-check job against a subtree sync PR that assumption doesn't hold. The subtree sync PR bumps the required toolchain used to build rustfmt and therefore the binary that gets built for the subtree sync PR has a different runtime dependency than the master rustfmt binary.

Now we set LD_LIBRARY_PATH twice to account for this potential difference.

These changes mostly improve logging out the cargo version and version
of the two rustfmt binaries that are compiled. Some other minor logging
changes were made as well to add some whitespace to improve visual
clarity when looking at the logs in the GitHub Actions console.
rustfmt currently has a runtime dependency on the sysroot. So when we
build a standalone rustfmt binary we need to set `LD_LIBRARY_PATH` so
each rustfmt binary knows where to find it's dependencies.

When running our Diff-Check job to test PRs for breaking changes it's
often the case that both the master rustfmt binary and the feature
branch binary have the same runtime dependencies so we only need to
set `LD_LIBRARY_PATH` once.

However, when running the diff-check job against a subtree sync PR that
assumption doesn't hold. The subtree sync PR bumps the required
toolchain used to build rustfmt and therefore the binary that gets built
for the subtree sync PR has a different runtime dependency than the
master rustfmt binary.

Now we set `LD_LIBRARY_PATH` twice to account for this potential
difference.
@ytmimi
Copy link
Contributor Author

ytmimi commented Oct 28, 2023

I Ran a Diff-Check job on my fork of rustfmt using the fix_diff_check_job_for_subtree_sync_prs branch. Here's what the additional log output looks like:

compiling with cargo 1.75.0-nightly (8eb8acbb1 2023-10-17)
Building rustfmt from src
Switched to a new branch 'fix_diff_check_job_for_subtree_sync_prs'
branch 'fix_diff_check_job_for_subtree_sync_prs' set up to track 'feature/fix_diff_check_job_for_subtree_sync_prs'.
Building feature rustfmt from src
Runtime dependencies for rustfmt -- LD_LIBRARY_PATH: /home/runner/.rustup/toolchains/nightly-2023-10-22-x86_64-unknown-linux-gnu/lib:/home/runner/.rustup/toolchains/nightly-2023-10-22-x86_64-unknown-linux-gnu/lib:
RUSFMT_BIN rustfmt 1.7.0-nightly (041f113 2023-10-23)
FEATURE_BIN rustfmt 1.7.0-nightly (eeac8b9 2023-10-28)

Copy link
Member

@calebcartwright calebcartwright left a comment

Choose a reason for hiding this comment

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

👍

@calebcartwright calebcartwright merged commit 37489e4 into rust-lang:master Nov 2, 2023
27 checks passed
@ytmimi ytmimi deleted the fix_diff_check_job_for_subtree_sync_prs branch November 2, 2023 18:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants