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

Start using gix in bootstrap #110369

Closed
wants to merge 1 commit into from
Closed

Conversation

mati865
Copy link
Contributor

@mati865 mati865 commented Apr 15, 2023

I don't know if this approach is acceptable so opening PR with a single change to get feedback. If this is accepted I can work on converting more uses of git cli.
FYI this crate is usable as an alternative backend for Cargo: rust-lang/cargo#11813

Fixes #105696

@rustbot
Copy link
Collaborator

rustbot commented Apr 15, 2023

r? @albertlarsan68

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

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) labels Apr 15, 2023
@rust-log-analyzer
Copy link
Collaborator

The job mingw-check-tidy failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)
Prepare all required actions
Getting action download info
Download action repository 'actions/checkout@v3' (SHA:8e5e7e5ab8b370d6c329ec480221332ada57f0ab)
Download action repository 'rust-lang/simpleinfra@master' (SHA:75291a2cdf0dd5ba60860701274f9500703102da)
Complete job name: PR - mingw-check-tidy
git config --global core.autocrlf false
shell: /usr/bin/bash --noprofile --norc -e -o pipefail {0}
env:
  CI_JOB_NAME: mingw-check-tidy
---
Building wheels for collected packages: reuse
  Building wheel for reuse (pyproject.toml): started
  Building wheel for reuse (pyproject.toml): finished with status 'done'
  Created wheel for reuse: filename=reuse-1.1.0-cp310-cp310-manylinux_2_35_x86_64.whl size=180119 sha256=9fa76c45f3193f307e02ea67d1a48cfe7ef2b854a074b766938a88e046bc7887
  Stored in directory: /tmp/pip-ephem-wheel-cache-p5pruett/wheels/c2/3c/b9/1120c2ab4bd82694f7e6f0537dc5b9a085c13e2c69a8d0c76d
Installing collected packages: boolean-py, binaryornot, setuptools, reuse, python-debian, markupsafe, license-expression, jinja2, chardet
  Attempting uninstall: setuptools
    Found existing installation: setuptools 59.6.0
    Not uninstalling setuptools at /usr/lib/python3/dist-packages, outside environment /usr
---
Successfully built 17190f37c7d6
Successfully tagged rust-ci:latest
Built container sha256:17190f37c7d6319945a148437689fddd51efaad229cad84f6f535863997f5a11
Uploading finished image to https://ci-caches.rust-lang.org/docker/5a83b1174027be8472df586f36f1b4e77c39a20882cc84831c17b85c3cc5731b4e272c71c37dd18e09cacea39bf89c1b3226a304723983ec1270656c20627ec9
upload failed: - to s3://rust-lang-ci-sccache2/docker/5a83b1174027be8472df586f36f1b4e77c39a20882cc84831c17b85c3cc5731b4e272c71c37dd18e09cacea39bf89c1b3226a304723983ec1270656c20627ec9 Unable to locate credentials
[CI_JOB_NAME=mingw-check-tidy]
[CI_JOB_NAME=mingw-check-tidy]
---
   Compiling getrandom v0.2.9
   Compiling unicode-normalization v0.1.22
   Compiling gix-sec v0.6.2
   Compiling filetime v0.2.16
   Compiling uluru v3.0.0
   Compiling nix v0.26.2
   Compiling aho-corasick v0.7.18
   Compiling syn v1.0.102
   Compiling imara-diff v0.1.5
---
    Finished release [optimized] target(s) in 14.34s
fmt check
tidy check
tidy: Skipping binary file check, read-only filesystem
tidy error: invalid license `Apache-2.0` in `bytesize 1.2.0 (registry+https://github.com/rust-lang/crates.io-index)`
tidy error: invalid license `CC0-1.0 OR MIT-0` in `dunce 1.0.3 (registry+https://github.com/rust-lang/crates.io-index)`
tidy error: invalid license `Apache-2.0` in `imara-diff 0.1.5 (registry+https://github.com/rust-lang/crates.io-index)`
tidy error: invalid license `BSD-3-Clause` in `instant 0.1.12 (registry+https://github.com/rust-lang/crates.io-index)`
tidy error: invalid license `BSD-3-Clause` in `sha1_smol 1.0.0 (registry+https://github.com/rust-lang/crates.io-index)`
tidy error: invalid license `MPL-2.0` in `uluru 3.0.0 (registry+https://github.com/rust-lang/crates.io-index)`
tidy error: invalid license `Apache-2.0` in `unicode-bom 1.1.4 (registry+https://github.com/rust-lang/crates.io-index)`
some tidy checks failed

@Noratrieb
Copy link
Member

Can you quickly test the compile times of bootstrap now vs before?

@mati865
Copy link
Contributor Author

mati865 commented Apr 15, 2023

Running cargo b after cargo clean on this branch takes 17s while on master cargo b after cargo clean takes 10s.

@Mark-Simulacrum
Copy link
Member

I feel like this needs a stronger motivation, particularly with such a hit to compile times. Do we know what's going wrong in #105696? I think anyone working within a git checkout should have a working git, and if we have cases where the git isn't quite working then maybe the better path is to support ignoring git's existence.

@mati865
Copy link
Contributor Author

mati865 commented Apr 15, 2023

I felt it could be nice to get rid of calling git cli and call the API instead, the mentioned issue was the reason why I looked at it but not the main motivation for this change. If this change in this direction is not welcome (even if I manage to reduce build time regression) I will just close this PR.

In #105696 the problem is when running with Cygwin (or MSYS2 in this case because git is built with Cygwin libraries) git it returns unix style paths which are unknown to 99% of the native (built with mingw-w64 or MSVC, not built with Cygwin library). Rust is within those 99% so it gives an error because of the wrong path:

# Git for Windows (native)
$ git rev-parse --show-toplevel
D:/Projekty/rust

# Cygwin/MSYS2 (with Cygwin libs)
$ git rev-parse --show-toplevel
/d/Projekty/rust

@Mark-Simulacrum
Copy link
Member

In general, if the build time regression is minimal and results are equivalent (including git CLI interop for things like submodule checkouts) then I don't mind moving to gix, especially if it improves compatibility.

@mati865
Copy link
Contributor Author

mati865 commented Apr 16, 2023

Okay, thanks. I'll see if I can get reduce build time first.

@mati865 mati865 marked this pull request as draft April 16, 2023 08:05
@mati865
Copy link
Contributor Author

mati865 commented Apr 16, 2023

@rustbot author

@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 Apr 16, 2023
@mati865
Copy link
Contributor Author

mati865 commented Apr 16, 2023

Looks like build time regression cannot get any lower than 14-15s (from 10s) on my machine so closing.

@mati865 mati865 closed this Apr 16, 2023
@mati865 mati865 deleted the gix_in_bootstrap branch April 16, 2023 08:52
@Noratrieb
Copy link
Member

Once #99989 is implemented build time will become less important and it could be picked up again

@jyn514
Copy link
Member

jyn514 commented Apr 16, 2023

#99989 actually would be fairly easy to implement I think, since it goes through the existing bootstrap.py file. Not sure if that's something you're interested in working on.

@mati865
Copy link
Contributor Author

mati865 commented Apr 16, 2023

Thanks but I already have to touch Python at my job more often than I'd like to, so I'll pass.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. 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.

Build of GNU toolchain 1.66.0 on Windows fails on rustbuild config.rs when git is installed in MSYS
7 participants