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

[Release] Don't build during test-release.sh Phase 3 install #82001

Merged
merged 1 commit into from
Feb 20, 2024

Conversation

rorth
Copy link
Collaborator

@rorth rorth commented Feb 16, 2024

As described in test-release.sh ninja install does builds in Phase 3, considerable parts of Phase 3 of a test-release.sh build are run by ninja install, ignoring both $Verbose and the parallelism set via -j NUM.

This patches fixes this by not specifying any explicit build target for Phase 3, thus running the full build as usual.

Tested on sparc64-unknown-linux-gnu.

As described in [test-release.sh ninja install does builds in Phase
3](llvm#80999), considerable parts
of Phase 3 of a `test-release.sh` build are run by `ninja install`,
ignoring both `$Verbose` and the parallelism set via `-j NUM`.

This patches fixes this by not specifying any explicit build target for
Phase 3, thus running the full build as usual.

Tested on `sparc64-unknown-linux-gnu`.
@rorth rorth requested a review from tstellar February 16, 2024 15:43
if [ "$Phase" -eq "3" ]; then
# Build everything at once, with the proper parallelism and verbosity,
# in Phase 3.
BuildTarget=
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does this actually change anything? Isn't this variable already unset?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

IIUC, BuildTarget is global, so stays unchanged from previous Phases unless reset. That's what I see in the build logs when comparing rc1 (with unchanged test-release.sh) and rc2 (with locally patched test-release.sh):

rc1:

 Phase 3: Building llvmCore
[...]
# Compiling llvm 18.1.0-rc1 Release
# ninja -j 8 -v
[1/4140
[...]
# Installing llvm 18.1.0-rc1 Release
# ninja install
[1/4093] Creating export file for LLVMHello
[2/4093] Building CXX object lib/MCA/CMakeFiles/LLVMMCA.dir/HardwareUnits/HardwareUnit.cpp.o

vs. rc2:

# Phase 3: Building llvmCore
[...]
# Compiling llvm 18.1.0-rc2 Release
# ninja -j 12 -v
[1/8225]
[...]
# Installing llvm 18.1.0-rc2 Release
# ninja install
[0/8] Performing build step for 'builtins'

i.e. with the patched script, in Phase 3 way more targets are actually built (with proper -v and -jN) in the build phase and nothing's built during ninja install, as it should be (and where the default parallelism does no harm).

@rorth rorth merged commit f6ac598 into llvm:main Feb 20, 2024
5 checks passed
@tstellar tstellar added this to the LLVM 18.X Release milestone Feb 20, 2024
@tstellar
Copy link
Collaborator

/cherry-pick f6ac598

llvmbot pushed a commit to llvmbot/llvm-project that referenced this pull request Feb 20, 2024
)

As described in [test-release.sh ninja install does builds in Phase
3](llvm#80999), considerable
parts of Phase 3 of a `test-release.sh` build are run by `ninja
install`, ignoring both `$Verbose` and the parallelism set via `-j NUM`.

This patches fixes this by not specifying any explicit build target for
Phase 3, thus running the full build as usual.

Tested on `sparc64-unknown-linux-gnu`.

(cherry picked from commit f6ac598)
@llvmbot
Copy link

llvmbot commented Feb 20, 2024

/pull-request #82420

llvmbot pushed a commit to llvmbot/llvm-project that referenced this pull request Feb 21, 2024
)

As described in [test-release.sh ninja install does builds in Phase
3](llvm#80999), considerable
parts of Phase 3 of a `test-release.sh` build are run by `ninja
install`, ignoring both `$Verbose` and the parallelism set via `-j NUM`.

This patches fixes this by not specifying any explicit build target for
Phase 3, thus running the full build as usual.

Tested on `sparc64-unknown-linux-gnu`.

(cherry picked from commit f6ac598)
@pointhex pointhex mentioned this pull request May 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

3 participants