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

x.py test try to access network when the compiler builds using vendored crates #90764

Closed
aplanas opened this issue Nov 10, 2021 · 9 comments · Fixed by #97513
Closed

x.py test try to access network when the compiler builds using vendored crates #90764

aplanas opened this issue Nov 10, 2021 · 9 comments · Fixed by #97513
Labels
C-bug Category: This is a bug. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap)

Comments

@aplanas
Copy link
Contributor

aplanas commented Nov 10, 2021

We are building the compiler and tools using the vendored crates properly (via x.py build) in systems with no network access. But when running the tests (x.py test) it tries to access the network.

Setting properly the envs vars that x.py set, this can be reproduced with:

# /home/abuild/rpmbuild/BUILD/rustc-1.56.1-src/build/bootstrap/debug/bootstrap test

Building stage0 tool tidy (x86_64-unknown-linux-gnu)
    Finished release [optimized] target(s) in 0.24s
tidy check
Checking which error codes lack tests...
* 627 error codes
* highest error code: E0785
Found 501 error codes
Found 0 error codes with no tests
Done!
thread '<unnamed>' panicked at 'cmd.exec() failed with Error during execution of `cargo metadata`:     Updating git repository `https://github.com/bjorn3/rust-ar.git`
warning: spurious network error (2 tries remaining): failed to resolve address for github.com: Temporary failure in name resolution; class=Net (12)
warning: spurious network error (1 tries remaining): failed to resolve address for github.com: Temporary failure in name resolution; class=Net (12)
error: failed to get `ar` as a dependency of package `rustc_codegen_cranelift v0.1.0 (/home/abuild/rpmbuild/BUILD/rustc-1.56.1-src/compiler/rustc_codegen_cranelift)`

Caused by:
  failed to load source for dependency `ar`

Caused by:
  Unable to update https://github.com/bjorn3/rust-ar.git?branch=do_not_remove_cg_clif_ranlib#de9ab0e5

Caused by:
  failed to fetch into: /home/abuild/.cargo/git/db/rust-ar-9b35aff8ad678e06

Caused by:
  network failure seems to have happened
  if a proxy or similar is necessary `net.git-fetch-with-cli` may help here
  https://doc.rust-lang.org/cargo/reference/config.html#netgit-fetch-with-cli

Caused by:
  failed to resolve address for github.com: Temporary failure in name resolution; class=Net (12)
', src/tools/tidy/src/deps.rs:293:20
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
thread 'main' panicked at 'called `Result::unwrap()` on an `Err` value: Any { .. }', src/tools/tidy/src/main.rs:77:9


command did not execute successfully: "/home/abuild/rpmbuild/BUILD/rustc-1.56.1-src/build/x86_64-unknown-linux-gnu/stage0-tools-bin/rust-tidy" "/home/abuild/rpmbuild/BUILD/rustc-1.56.1-src" "/home/abuild/rpmbuild/BUILD/rust-1.56.1-x86_64-unknown-linux-gnu/usr/bin/cargo" "/home/abuild/rpmbuild/BUILD/rustc-1.56.1-src/build" "8"
expected success, got: exit status: 101

Seems that the call is be present in rust-tidy (in deps.rs). IIUC should use the --offline parameter, and I do not think that is checking the presence of .cargo/config per the ar error (that is present in the vendor directory)

@aplanas aplanas added the C-bug Category: This is a bug. label Nov 10, 2021
@ehuss
Copy link
Contributor

ehuss commented Nov 10, 2021

Hm, that's strange! Unfortunately I am unable to reproduce. Can you provide exact instructions starting with downloading the source file? The following worked for me:

wget https://static.rust-lang.org/dist/rustc-1.56.1-src.tar.gz
tar -xzf rustc-1.56.1-src.tar.gz
cd rustc-1.56.1-src
./configure --set rust.channel=stable --set build.vendor=true
# Download bootstrap
./x.py help
# Disconnect network here
./x.py test src/tools/tidy

You may want to check that you are running the command from the correct directory (so that it can see .cargo/config).

@ehuss ehuss added the T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) label Nov 10, 2021
@aplanas
Copy link
Contributor Author

aplanas commented Nov 10, 2021

Oh! ... that is good news as maybe I am doing something wrong. The full spec is here[1], but the tl;dr is like this:

./configure \
  --host=%{_host} \
  --build=%{_build} \
  --prefix=%{_prefix} \
  --bindir=%{_bindir} \
  --sysconfdir=%{_sysconfdir} \
  --datadir=%{_datadir} \
  --localstatedir=%{_localstatedir} \
  --mandir=%{_mandir} \
  --infodir=%{_infodir} \
  --set rust.deny-warnings=false \
  --disable-option-checking \
  --build=%{rust_triple} --host=%{rust_triple} --target=%{rust_triple} \
  --enable-local-rust \
  --local-rust-root=%{rust_root} \
  --libdir=%{common_libdir} \
  --docdir=%{_docdir}/rust \
  --disable-codegen-tests \
  --enable-optimize \
  --disable-docs \
  --disable-compiler-docs \
  --enable-verbose-tests \
  --disable-jemalloc \
  --disable-rpath \
  --enable-vendor \
  --enable-extended \
  --tools="cargo" \
  --release-channel="stable"

cat > .env.sh <<\EOF
export RUSTFLAGS="%{rustflags}"
export DESTDIR=%{buildroot}
export LIBSSH2_SYS_USE_PKG_CONFIG=1
export CARGO_FEATURE_VENDORED=1
# END EXPORTS
EOF

. ./.env.sh

./x.py build -v

./x.py install

./x.py test

I will try manually per your example in a different place.

--
[1] https://build.opensuse.org/package/view_file/devel:languages:rust/rust1.56/rust1.56.spec?expand=1

@aplanas
Copy link
Contributor Author

aplanas commented Nov 10, 2021

A minimal config that have the issue is this:

./configure \
  --enable-local-rust \
  --local-rust-root=/home/abuild/rpmbuild/BUILD/rust-1.56.1-x86_64-unknown-linux-gnu/usr \
  --llvm-root=/usr \
  --disable-codegen-tests \
  --enable-vendor \
  --release-channel=stable

./x.py --help

./x.py test

IIUC the differences from the working example are: I am using a provided local rustc and the system LLVM.

@ehuss
Copy link
Contributor

ehuss commented Nov 10, 2021

Oh, I see the issue, I was a bit mistaken before (I had the git repo cached already).

cc @bjorn3

The problem is that the cranelift workspace has several git dependencies. Those are not captured here.

A simple reproduction is to run the following with vendoring enabled:

CARGO_HOME=chome cargo tree --manifest-path compiler/rustc_codegen_cranelift/Cargo.toml --offline

As a short-term workaround, you can add the following to .cargo/config:

[source."https://github.com/bjorn3/rust-ar.git"]
git = "https://github.com/bjorn3/rust-ar.git"
branch = "do_not_remove_cg_clif_ranlib"
replace-with = "vendored-sources"

[source."https://github.com/bytecodealliance/wasmtime.git"]
git = "https://github.com/bytecodealliance/wasmtime.git"
replace-with = "vendored-sources"

I guess one option is to add those lines to bootstrap.py, but I feel like that is somewhat fragile and likely to break again or get out of sync. Alternatively, perhaps the cranelift check in tidy could be disabled in certain scenarios (like being vendored), but that can be difficult to detect.

Another option is to try to capture the vendor config to disk. I think I like that option the most, but will be harder since cargo vendor is done in two places (and we'd need to figure out a good place to store the config for distribution).

@Mark-Simulacrum
Copy link
Member

Generally it's our intent to remove those git dependencies as we move forward and not really introduce them in the future (I think it was probably an oversight/mistake to do so), so I don't think we need to design full-fledged solutions but having temporary patches would be good. It might be that the src tarball should replace cranelift with a stub (essentially empty) project to avoid this particular case, for example.

@bjorn3
Copy link
Member

bjorn3 commented Nov 10, 2021

I already replaced cranelift git dependencies with crates.io dependencies at https://github.com/bjorn3/rustc_codegen_cranelift (https://github.com/bjorn3/rustc_codegen_cranelift/pull/1201), which will fix part of the problem once I do another subtree sync. Unfortunately I am stuck on my own fork of rust-ar for now as upstream doesn't support writing a symbol table: https://github.com/bjorn3/rustc_codegen_cranelift/issues/1202

@aplanas
Copy link
Contributor Author

aplanas commented Nov 11, 2021

Thanks a lot for catching the bug so fast and so accurately! I am adding the workaround in the spec to enable tests against the system LLVM, this will help us.

IMHO technically the bug has been solved after @bjorn3 comment. What can be the outcome here before closing it? Wait until it reach the repo? Provide a test that validate before each release that no dependency comes from git?

@bjorn3
Copy link
Member

bjorn3 commented Nov 11, 2021

It is not fully fixed yet unfortunately. Rust-ar is still used as git dependency.

aplanas added a commit to aplanas/rust that referenced this issue Nov 14, 2021
In some situations we should want on influence into the .cargo/config
when we use vendored source.  One example is rust-lang#90764, when we want to
workaround some references to crates forked and living in git, that are
missing in the vendor/ directory.

This commit will create the .cargo/config file only when the .cargo/
directory needs to be created.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Nov 18, 2021
…mulacrum

bootstap: create .cargo/config only if not present

In some situations we should want on influence into the .cargo/config
when we use vendored source.  One example is rust-lang#90764, when we want to
workaround some references to crates forked and living in git, that are
missing in the vendor/ directory.

This commit will create the .cargo/config file only when the .cargo/
directory needs to be created.
JohnTitor added a commit to JohnTitor/rust that referenced this issue Nov 19, 2021
…mulacrum

bootstap: create .cargo/config only if not present

In some situations we should want on influence into the .cargo/config
when we use vendored source.  One example is rust-lang#90764, when we want to
workaround some references to crates forked and living in git, that are
missing in the vendor/ directory.

This commit will create the .cargo/config file only when the .cargo/
directory needs to be created.
@jyn514
Copy link
Member

jyn514 commented Jun 22, 2022

This is fixed in #97513, which saves the output of cargo vendor to disk rather than trying to predict it ahead of time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: This is a bug. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants