make ignore-git true by default when channel is dev

[JeremySorensen]

Fixes#43771
(Handle git info in rustbuild differently)

[rust-highfive]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @brson (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

[JeremySorensen]

This is my first PR, so please tell me what I did wrong!
I wanted to point out that I noticed that the default channel is "dev" when nothing is specified, so I changed git-ignore to true by default to be consistent. (Both in the code and in the config.toml.example file.)

[Mark-Simulacrum]

Thanks! If you add "Fixes #43771" to the description (remove issue) GitHub will automatically close it once this merges.

@bors r+

[bors]

📌 Commit c3b27d5 has been approved by Mark-Simulacrum

[ishitatsuyuki]

I have no idea why you're doing this, as almost any action will cause a full rebuild due to the staged nature.

This removes the version number from my dev channel build, and I will have to change the channel to nightly as the ./configure script doesn't have option related to git.

Oh, and by the way, dev is the default with git, and stable is the default without git. I think a build should contain version number out of box. If you need to disable for timestamp reasons, just do in your own config.toml.

[bors]

⌛ Testing commit c3b27d5 with merge b7afe1fdef0a39be24a9fee3bd782e370e2409f6...

[ishitatsuyuki]

Oh, I see, there was a discussion for this decision. Nevermind my comments.

[bors]

💔 Test failed - status-travis

[kennytm]

@bors: retry

distcheck failed, feels spurious.

[00:07:21] downloading https://static.rust-lang.org/dist/2017-07-18/cargo-beta-x86_64-unknown-linux-gnu.tar.gz
######################################################################## 100.0%
[00:07:22] extracting /checkout/obj/build/tmp/distcheck/build/cache/2017-07-18/cargo-beta-x86_64-unknown-linux-gnu.tar.gz
[00:07:22] error: failed to load source for a dependency on `serde_derive`
[00:07:22] [00:07:22] Caused by:
[00:07:22] Unable to update registry https://github.com/rust-lang/crates.io-index
[00:07:22] [00:07:22] Caused by:
[00:07:22] failed to update replaced source `registry https://github.com/rust-lang/crates.io-index`
[00:07:22] [00:07:22] Caused by:
[00:07:22] failed to read root of directory source: /checkout/obj/build/tmp/distcheck/src/vendor
[00:07:22] [00:07:22] Caused by:
[00:07:22] No such file or directory (os error 2)
[00:07:22] failed to run: /checkout/obj/build/tmp/distcheck/build/x86_64-unknown-linux-gnu/stage0/bin/cargo build --manifest-path /checkout/obj/build/tmp/distcheck/src/bootstrap/Cargo.toml --locked --frozen
[00:07:22] Build completed unsuccessfully in 0:00:09
[00:07:22] Makefile:54: recipe for target 'check' failed

[bors]

⌛ Testing commit c3b27d5 with merge 2c5f1bc088de952ace86da838b963711357e5880...

[bors]

💔 Test failed - status-travis

[TimNN]

distcheck failed again, I don't think this is spurious. Looking at the patch, I'm not sure if it entirely correct, although I'm not currently that familiar with the inner working of rustbuild:

This patch changes the default of ignore-git to true and then later updates the default based on channel, but only if config.toml contains a [rust] section. Given that the builders still use config.mk I believe that ignore-git will now be true on all builders, which then causes the above failure.

[TimNN]

This should probably be "if left commented out".

[JeremySorensen]

yes you are right

[JeremySorensen]

Given that the builders still use config.mk I believe that ignore-git will now be true on all builders, which then causes the above failure.

Oh darn... OK well I think the fix is simple, just revert the initial value of ignore-git back to false, and only keep the logic for setting the default if there is a config.toml file.
But that raises a question, are there some tests that:

1. Use a config.toml file
2. Do NOT specify an explicit value for ignore-git
3. Require ignore-git to be false in order to pass?

If all of those are true, for any test, then that(those) test(s) will fail. So if we want to keep this patch, we would need to find all such tests and explicitly set ignore-git to false.

[JeremySorensen]

sorry I was trying to figure out how to update a pull request, I didn't realize that pushing to my fork was enough to cause stuff to run. I will follow contributing.md more closely and see if I can figure out what is going on with the tests.

[JeremySorensen]

OK I am getting to the point where I need some mentoring here. When I ran the tests (on my machine), a whole block of them failed with the same error. Here is an excerpt of the results:

 Check compiletest suite=run-make mode=run-make (x86_64-pc-windows-msvc -> x86_64-pc-windows-msvc)
Building rustdoc for stage2 (x86_64-pc-windows-msvc)
running 159 tests
test [run-make] run-make\allow-non-lint-warnings-cmdline ... FAILED
test [run-make] run-make\alloc-extern-crates ... FAILED
...
test [run-make] run-make\windows-subsystem ... FAILED
failures:
---- [run-make] run-make\allow-non-lint-warnings-cmdline stdout ----
thread '[run-make] run-make\allow-non-lint-warnings-cmdline' panicked at 'failed to spawn `make`: Error { repr: Os { code: 2, message: "The system cannot find the file specified." } }', src\libcore\result.rs:860:4
note: Run with `RUST_BACKTRACE=1` for a backtrace.
---- [run-make] run-make\alloc-extern-crates stdout ----
thread '[run-make] run-make\alloc-extern-crates' panicked at 'failed to spawn `make`: Error { repr: Os { code: 2, message: "The system cannot find the file specified." } }', src\libcore\result.rs:860:4
...
---- [run-make] run-make\windows-subsystem stdout ----
thread '[run-make] run-make\windows-subsystem' panicked at 'failed to spawn `make`: Error { repr: Os { code: 2, message: "The system cannot find the file specified." } }', src\libcore\result.rs:860:4
failures:
[run-make] run-make\a-b-a-linker-guard
[run-make] run-make\alloc-extern-crates
...
[run-make] run-make\windows-subsystem
test result: FAILED. 0 passed; 159 failed; 0 ignored; 0 measured; 0 filtered out

But the thing is I get this even after I revert back to the original config.rs. So the only thing that is changed is config.toml, but everything builds fine its the tests that fail. So I am not sure why these tests are failing but it doesn't seem related to my code.

Also I am not sure why the travis-ci failed, it kind of looks like it got cancelled, if somebody cancelled it because of my comment indicating that I had kicked it off inadvertently then that is fine, but I don't know how to tell that.

At this point the only change to the code is one line:
config.ignore_git = config.channel == "dev";
which goes right before this existing line:
set(&mut config.ignore_git, rust.ignore_git);
So I think that is right, if the travis-ci was cancelled, can restart it and rerun the tests on the server to see which fail?
Also how can I get those tests to pass? Should I just do a clean clone of my fork and start over?

[Mark-Simulacrum]

It looks like you might not have make installed. Maybe try running that and see if it exists? Travis passed, and I think the code is correct, so:

@bors r+

[bors]

📌 Commit bdb7901 has been approved by Mark-Simulacrum

[kennytm]

Unable to update registry, spurious. But @JeremySorensen wants to hold off a bit?

[alexcrichton]

@JeremySorensen oh bors is our integration bot, when we "r+" a PR it goes into a queue of PRs to be tested, and then the queue is drained one at a time. You've also now seen what happens when tests fail! (spuriously)

The comment looks fine by me though, but @JeremySorensen did you want to reword it?

[JeremySorensen]

@alexcrichton I think the expected behavior is that if the user doesn't copy config.toml.example over, OR if they do but don't change it, then the channel will be "dev" and ignore_git will be true correct? If so, then yes the comment looks correct to me. (I thought I remembered it was still set to false in the example, but I was wrong.)

[alexcrichton]

Ok!

I think, though, that the failure here is not a spurious failure. I believe what you'll want to do is add an option to the configure script for this one builder. You can find the builder's configuration in src/ci/docker/x86_64-gnu-distcheck and the env var should be something like RUST_CONFIGURE_ARGS. The argument you want to add is --set build.ignore-git=true

Note that --set hasn't been tested much before, so there may be a bug or two with that...

[alexcrichton]

Er sorry, you want ignore-git=false b/c we want that builder to read git information!

[alexcrichton]

@bors: r+

[bors]

📌 Commit d24ee23 has been approved by alexcrichton

[arielb1]

configure: processing command line
[00:00:27] configure: [00:00:27] configure: rust.debug-assertions := True
[00:00:27] configure: build.ignore-git := false
[00:00:27] configure: build.submodules := False
[00:00:27] configure: llvm.assertions := True
[00:00:27] configure: build.locked-deps := True
[00:00:27] configure: llvm.ccache := sccache
[00:00:27] configure: build.openssl-static := True
[00:00:27] configure: build.build := x86_64-unknown-linux-gnu
[00:00:27] configure: build.configure-args := ['--build=x86_64-unknown-linux-gnu', '--set', ...
[00:00:27] Traceback (most recent call last):
[00:00:27] File "/checkout/src/bootstrap/configure.py", line 378, in <module>
[00:00:27] configure_section(sections[section_key], section_config)
[00:00:27] File "/checkout/src/bootstrap/configure.py", line 367, in configure_section
[00:00:27] raise RuntimeError("failed to find config line for {}".format(key))
[00:00:27] RuntimeError: failed to find config line for ignore-git

@bors r-

[alexcrichton]

I think maybe rust.ignore-git? @JeremySorensen I'm shooting the dark here so it'd be best if you could verify locally, which I believe you can do via ./src/ci/docker/run.sh x86_64-gnu-distcheck

[JeremySorensen]

I ran it as is and got the same error (as expected), then I changed the line in Dockerfile to
ENV RUST_CONFIGURE_ARGS --build=x86_64-unknown-linux-gnu --set rust.ignore-git=false
now I get this:

failed to parse TOML configuration 'config.toml': invalid type: string "false", expected a boolean for key rust.ignore-git

I checked, the false is not in quotes in the Docker file, so I guess it is getting escaped incorrectly, or not getting parsed.

[alexcrichton]

I think we may need to add a special case to this line to just convert "true" to True to get properly set as a boolean perhaps?

[JeremySorensen]

@alexcrichton Ha! I just did that! my code looks like this:

forkeyinknown_args:
# The `set` option is special and can be passed a bunch of timesifkey=='set':
foroption, valueinknown_args[key]:
keyval=value.split('=', 1)
iflen(keyval) ==1orkeyval[1] =="true":
value=Trueelifkeyval[1] =="false":
value=Falseelse:
value=keyval[1]
set(keyval[0], value)
continue

We actually need the false case here
Should I commit that for this PR as well?
edit: just noticed we seem to use single quotes for strings in Python, I can fix that before I commit.
edit 2: nevermind we use double quotes in lots of other places, not sure why 'set' is in single quotes

[alexcrichton]

Looks good to me!

[JeremySorensen]

I ran the tests again and it looks like it got the ignore-git=false, but I am having a bunch of seemingly unrelated errors. I don't know anything about Docker (I had to install it to make the command work) but I wonder if it is because I ran the test several times and killed it in progress a couple times, or if somehow I really broke everything. Should I push the changes to the Docker file and configure.py?

I didn't redirect the output of the command to a file and it rolled off the end so I don't know if I got everything but here are at least some of the errors:

get_toml (bootstrap.RustBuild)
Doctest: bootstrap.RustBuild.get_toml ... ok
program_config (bootstrap.RustBuild)
Doctest: bootstrap.RustBuild.program_config ... FAIL
rustc_stamp (bootstrap.RustBuild)
Doctest: bootstrap.RustBuild.rustc_stamp ... ok
...
Return True when the stamp file does not exists ... ok
======================================================================
FAIL: program_config (bootstrap.RustBuild)
Doctest: bootstrap.RustBuild.program_config
----------------------------------------------------------------------
Traceback (most recent call last):
File "/usr/lib/python2.7/doctest.py", line 2226, in runTest
raise self.failureException(self.format_failure(new.getvalue()))
AssertionError: Failed doctest test for bootstrap.RustBuild.program_config
File "/checkout/src/bootstrap/bootstrap.py", line 519, in program_config
----------------------------------------------------------------------
File "/checkout/src/bootstrap/bootstrap.py", line 527, in bootstrap.RustBuild.program_config
Failed example:
cargo_path.rstrip(".exe") == os.path.join("/tmp/rust",
"bin", "cargo")
Expected:
True
Got:
False
----------------------------------------------------------------------
Ran 14 tests in 0.068s
FAILED (failures=1)
invalid checksum:
found: 334d016f755cd6dc58c53a86e183882f8ec14f52fb05345887c8a5edd42c87b7
expected: 64ec88ca00b268e5ba1a35678a1b5316d212f4f366b2477232534a8aeca37f3c
+ python2.7 ../x.py test distcheck
Finished dev [unoptimized] target(s) in 0.0 secs
Distcheck
...
failures:
net::tcp::tests::clone_accept_concurrent
net::tcp::tests::clone_accept_smoke
net::tcp::tests::clone_while_reading
net::tcp::tests::close_read_wakes_up
net::tcp::tests::close_readwrite_smoke
net::tcp::tests::connect_loopback
net::tcp::tests::double_bind
net::tcp::tests::fast_rebind
net::tcp::tests::multiple_connect_interleaved_greedy_schedule
net::tcp::tests::multiple_connect_interleaved_lazy_schedule
net::tcp::tests::multiple_connect_serial
net::tcp::tests::partial_read
net::tcp::tests::peek
net::tcp::tests::read_eof
net::tcp::tests::shutdown_smoke
net::tcp::tests::smoke_test
net::tcp::tests::socket_and_peer_name
net::tcp::tests::tcp_clone_smoke
net::tcp::tests::tcp_clone_two_read
net::tcp::tests::tcp_clone_two_write
net::tcp::tests::write_close
net::udp::tests::connect_send_peek_recv
net::udp::tests::peek_from
net::udp::tests::set_nonblocking
net::udp::tests::socket_name_ip4
net::udp::tests::socket_smoke_test_ip4
net::udp::tests::udp_clone_smoke
net::udp::tests::udp_clone_two_read
net::udp::tests::udp_clone_two_write
test result: FAILED. 773 passed; 29 failed; 0 ignored; 0 measured; 0 filtered out

[alexcrichton]

Yeah that all looks fine to me, thanks for investigating though! I'm not sure what's up with the python looking failure, but the docker failures I believe are all related to IPv6 in the container. I get them locally as well and I'm not sure how to fix them :(

[alexcrichton]

@bors: r+

Thanks!

[bors]

📌 Commit 4f591a4 has been approved by alexcrichton

[bors]

⌛ Testing commit 4f591a4 with merge 51a54b6...

[bors]

☀️ Test successful - status-appveyor, status-travis
Approved by: alexcrichton
Pushing 51a54b6 to master...